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
