+cc Ville
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
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@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)
>
>
> > +
> > #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
