Re: [RFC 1/4] drm/i915: Add Display Gen info.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 30, 2018 at 11:52:30AM +0200, Jani Nikula wrote:
> On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> > Introduce Display Gen. The goal is to use this to minimize
> > the amount of platform codename checks we have nowdays on
> > display code.
> >
> > The introduction of a new platform should be just
> > gen >= current.
> 
> So the patches 1-3 look nice for GLK. The thing that bugs me here is
> that this doesn't help VLV/CHV GMCH display at all. We'll still continue
> to have the more feature oriented HAS_GMCH_DISPLAY, HAS_DDI, and
> HAS_PCH_SPLIT. Haswell display is still better represented by HAS_DDI
> than gen because it's 7.5.
> 
> Patch 4 means continued pedantic review about not mixing up IS_GEN and
> IS_DISPLAY_GEN. If we aren't strict about the separation, then what's
> the point? It's not immediately obvious that it's worth the hassle. Only
> time will tell.

I think the real point of having a display_gen in the device_info struct is to
group together features that would otherwise be too fine-grained. That would
lead to growing the device_info a lot for little benefit.

But, as you say even inside a single display gen we have small differences or some
features that are not specific do a display gen. Just like we have from gem gen.
What we maybe need is a way to represent the .5 thing, but I'm not sure it's worth...
IMO we should approximate it to the closest one and differentiate the specific places
with the more fine-grained feature checks.

> 
> I'll want to hear more opinions before merging.
> 
> One note inline below.
> 
> 
> BR,
> Jani.
> 
> 
> >
> > Just a gen++ without exposing any new feature or ip.
> > so this would minimize the amount of patches needed
> > for a bring-up specially holding them on internal branches.
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h          | 28 ++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/i915_pci.c          |  5 ++++-
> >  drivers/gpu/drm/i915/intel_device_info.h |  2 ++
> >  3 files changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c9e5bab6861b..3242229688e3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2349,8 +2349,9 @@ intel_info(const struct drm_i915_private *dev_priv)
> >  #define INTEL_INFO(dev_priv)	intel_info((dev_priv))
> >  #define DRIVER_CAPS(dev_priv)	(&(dev_priv)->caps)
> >  
> > -#define INTEL_GEN(dev_priv)	((dev_priv)->info.gen)
> > -#define INTEL_DEVID(dev_priv)	((dev_priv)->info.device_id)
> > +#define INTEL_GEN(dev_priv)		((dev_priv)->info.gen)
> > +#define INTEL_DISPLAY_GEN(dev_priv)	((dev_priv)->info.display_gen)
> > +#define INTEL_DEVID(dev_priv)		((dev_priv)->info.device_id)
> >  
> >  #define REVID_FOREVER		0xff
> >  #define INTEL_REVID(dev_priv)	((dev_priv)->drm.pdev->revision)
> > @@ -2363,6 +2364,8 @@ intel_info(const struct drm_i915_private *dev_priv)
> >  /* Returns true if Gen is in inclusive range [Start, End] */
> >  #define IS_GEN(dev_priv, s, e) \
> >  	(!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e))))
> > +#define IS_DISPLAY_GEN(dev_priv, s, e) \
> > +	(!!((dev_priv)->info.display_gen_mask & INTEL_GEN_MASK((s), (e))))
> >  
> >  /*
> >   * Return true if revision is in range [since,until] inclusive.
> > @@ -2532,6 +2535,27 @@ intel_info(const struct drm_i915_private *dev_priv)
> >  #define IS_GEN10(dev_priv)	(!!((dev_priv)->info.gen_mask & BIT(9)))
> >  #define IS_GEN11(dev_priv)	(!!((dev_priv)->info.gen_mask & BIT(10)))
> >  
> > +#define IS_DISPLAY_GEN2(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(2)))
> > +#define IS_DISPLAY_GEN3(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(3)))
> > +#define IS_DISPLAY_GEN4(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(4)))
> > +#define IS_DISPLAY_GEN5(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(5)))
> > +#define IS_DISPLAY_GEN6(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(6)))
> > +#define IS_DISPLAY_GEN7(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(7)))
> > +#define IS_DISPLAY_GEN8(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(8)))
> > +#define IS_DISPLAY_GEN9(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(9)))
> > +#define IS_DISPLAY_GEN10(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(10)))
> > +#define IS_DISPLAY_GEN11(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
> > +					    & BIT(11)))
> 
> I know this is the same pattern as in IS_GEN<N> above, but shouldn't the
> compiler end up with the same result if these were simply:
> 
> #define IS_DISPLAY_GEN2(dev_priv) IS_DISPLAY_GEN(dev_priv, 2, 2)


humn... maybe this is too magic, but it works for me and I didn't add any additional
macro to the kernel to implement it :)

[CI, DON'T TEST THIS] diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
[CI, DON'T TEST THIS] index 554627dc623c..02a8b51fd733 100644
[CI, DON'T TEST THIS] --- a/drivers/gpu/drm/i915/i915_drv.h
[CI, DON'T TEST THIS] +++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2385,9 +2385,15 @@ intel_info(const struct drm_i915_private *dev_priv)
 	GENMASK((e) - 1, (s) - 1))
 
 /* Returns true if Gen is in inclusive range [Start, End] */
-#define IS_GEN(dev_priv, s, e) \
+#define _IS_GEN_ARG2(dev_priv, s, e) \
 	(!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e))))
 
+#define _IS_GEN_ARG1(dev_priv, g) \
+	(!!((dev_priv)->info.gen_mask & BIT((g) - 1)))
+
+#define IS_GEN(dev_priv, ...) \
+	CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__)
+
 /*
  * Return true if revision is in range [since,until] inclusive.
  *


So we could use IS_GEN(dev_priv, 2) as well as IS_GEN(dev_priv, 2, 4), which IMO is very clear.
The same would apply for IS_DISPLAY_GEN() version. And if they generate the same code, we could
just change the expansion to repeat the argument.


Lucas De Marchi

> 
> 
> > +
> >  #define IS_LP(dev_priv)	(INTEL_INFO(dev_priv)->is_lp)
> >  #define IS_GEN9_LP(dev_priv)	(IS_GEN9(dev_priv) && IS_LP(dev_priv))
> >  #define IS_GEN9_BC(dev_priv)	(IS_GEN9(dev_priv) && !IS_LP(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index 44e745921ac1..fb8caf846c02 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -30,7 +30,10 @@
> >  #include "i915_selftest.h"
> >  
> >  #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x)
> > -#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
> > +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1), \
> > +		.display_gen = (x), .display_gen_mask = BIT((x))
> > +/* Unless explicitly stated otherwise Display gen inherits platform gen */
> > +#define DISPLAY_GEN(x) .display_gen = (x), .display_gen_mask = BIT((x))
> >  
> >  #define GEN_DEFAULT_PIPEOFFSETS \
> >  	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> > index b4c2c4eae78b..9f31f29186a8 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -151,8 +151,10 @@ typedef u8 intel_ring_mask_t;
> >  struct intel_device_info {
> >  	u16 device_id;
> >  	u16 gen_mask;
> > +	u16 display_gen_mask;
> >  
> >  	u8 gen;
> > +	u8 display_gen;
> >  	u8 gt; /* GT number, 0 if undefined */
> >  	u8 num_rings;
> >  	intel_ring_mask_t ring_mask; /* Rings supported by the HW */
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux