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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

+cc Ville

On Tue, Oct 30, 2018 at 11:52:30AM +0200, Jani Nikula wrote:
> On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> 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

Couldn't we define

cherryview platform with

DISPLAY_GEN(7) /* 7.5 */

and also a

#define IS_DISPLAY_GEN7_LP(dev_priv) IS_DISPLAY_GEN7(dev_priv) && IS_LP(dev_priv)

this would cover most of IS_CHERRYVIEW || IS_VALLEYVIEW code.

> GMCH display at all.

For GMCH I believe with this gen display range in place
we could use IS_DISPLAY(dev_priv, 2, 4) for most cases.

For the intersection with VLV/CHV I would define a
info.has_cxsr_wm

This would be a real feature based ting instead of "gmch_display"

> We'll still continue
> to have the more feature oriented HAS_GMCH_DISPLAY,

maybe very little cases would be:
IS_DISPLAY(dev_priv, 2, 4) || IS_DISPLAY_GEN7_LP(dev_priv)

so we could kill GMCH entirely.

> HAS_DDI, and
> HAS_PCH_SPLIT.

HAS_PCH_SPLIT makes sense in my opinion because of the south display
stuff that is on PCH. That one I would keep.

> Haswell display is still better represented by HAS_DDI
> than gen because it's 7.5.

Maybe this is also a place to define Haswell with:

DISPLAY_GEN(7) /* 7.5 */

And kill all HAS_DDI in favor of Display ranges as well.

>
> 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 agree it is not clear yet it's worth the hassle.

I believe that the conversion can be slowly with only places that
we are changing to have IS_DISPLAY_GEN(dev_priv) >= 7 we change
the entire block to use DISPLAY_GEN instead of IS_GEN or platform
codenames.

> 
> I'll want to hear more opinions before merging.

Yeap, that's the idea here ;)

My ultimate goal is to stop the current mixed cases where we have

if INTEL_GEN > 11
else PLATFORM_CODENAME

and also expand the places where we have IS_ICELAKE
to be INTEL_GEN > 11

So adding the next gen gets easier and without bureaucracy
just gen++...

Thanks a lot for the comments,
Rodrigo.

> 
> 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@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  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)
> 
> 
> > +
> >  #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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx