On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi at 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 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'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 at intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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