Re: [RFC 6/7] drm/i915: Introduce subplatform concept

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 12 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Introduce subplatform mask to eliminate throughout the code devid checking
> sprinkle, mostly courtesy of IS_*_UL[TX] macros.
>
> Subplatform mask initialization is done at runtime device info init.

I kind of like the concept, and I like the centralization of devid
checks in one function, but I've always wanted to take this to one step
further: only specify device ids in i915_pciids.h, and *nowhere* else.

It's perhaps too much duplication to create a device info for all these
variants, but I think it would be possible to make the subplatform info
table driven using macros defined in i915_pciids.h.

I think Rodrigo had patches to define CNL port F in terms of num_ports,
but perhaps the subplatform approach works better for that.

BR,
Jani.


>
> v2: Fixed IS_SUBPLATFORM. Updated commit msg.
> v3: Chris was right, there is an ordering problem.
> v4: Drop mask sharing, rename title, rebase.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c          |  2 +
>  drivers/gpu/drm/i915/i915_drv.h          | 59 +++++++++------------
>  drivers/gpu/drm/i915/intel_device_info.c | 65 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_device_info.h | 18 +++++++
>  4 files changed, 108 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 4f5ddc3d2f4d..14c199438978 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1688,6 +1688,8 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	runtime_info->gen_mask = BIT(INTEL_GEN(i915) - 1);
>  	runtime_info->platform_mask = BIT(device_info->platform);
>  
> +	intel_device_info_subplatform_init(i915);
> +
>  	return i915;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 283592dd7023..4ec4a6308fe4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2377,6 +2377,10 @@ static inline unsigned int i915_sg_segment_size(void)
>  
>  #define IS_PLATFORM(dev_priv, p) \
>  	((dev_priv)->runtime_info.platform_mask & BIT(p))
> +#define IS_SUBPLATFORM(dev_priv, p, s) \
> +	(IS_PLATFORM(dev_priv, p) && \
> +	 ((dev_priv)->runtime_info.subplatform_mask & \
> +	  BIT(INTEL_SUBPLATFORM_##s)))
>  
>  #define IS_I830(dev_priv)	IS_PLATFORM(dev_priv, INTEL_I830)
>  #define IS_I845G(dev_priv)	IS_PLATFORM(dev_priv, INTEL_I845G)
> @@ -2391,11 +2395,15 @@ static inline unsigned int i915_sg_segment_size(void)
>  #define IS_G45(dev_priv)	IS_PLATFORM(dev_priv, INTEL_G45)
>  #define IS_GM45(dev_priv)	IS_PLATFORM(dev_priv, INTEL_GM45)
>  #define IS_G4X(dev_priv)	(IS_G45(dev_priv) || IS_GM45(dev_priv))
> -#define IS_PINEVIEW_G(dev_priv)	(INTEL_DEVID(dev_priv) == 0xa001)
> -#define IS_PINEVIEW_M(dev_priv)	(INTEL_DEVID(dev_priv) == 0xa011)
> +#define IS_PINEVIEW_G(dev_priv)	\
> +	IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_G)
> +#define IS_PINEVIEW_M(dev_priv)	\
> +	IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_M)
>  #define IS_PINEVIEW(dev_priv)	IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
>  #define IS_G33(dev_priv)	IS_PLATFORM(dev_priv, INTEL_G33)
> -#define IS_IRONLAKE_M(dev_priv)	(INTEL_DEVID(dev_priv) == 0x0046)
> +#define IS_IRONLAKE(dev_priv)	IS_PLATFORM(dev_priv, INTEL_IRONLAKE)
> +#define IS_IRONLAKE_M(dev_priv)	\
> +	IS_SUBPLATFORM(dev_priv, INTEL_IRONLAKE, IRONLAKE_M)
>  #define IS_IVYBRIDGE(dev_priv)	IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE)
>  #define IS_IVB_GT1(dev_priv)	(IS_IVYBRIDGE(dev_priv) && \
>  				 INTEL_INFO(dev_priv)->gt == 1)
> @@ -2413,40 +2421,20 @@ static inline unsigned int i915_sg_segment_size(void)
>  #define IS_MOBILE(dev_priv)	(INTEL_INFO(dev_priv)->is_mobile)
>  #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \
>  				    (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00)
> -#define IS_BDW_ULT(dev_priv)	(IS_BROADWELL(dev_priv) && \
> -				 ((INTEL_DEVID(dev_priv) & 0xf) == 0x6 ||	\
> -				 (INTEL_DEVID(dev_priv) & 0xf) == 0xb ||	\
> -				 (INTEL_DEVID(dev_priv) & 0xf) == 0xe))
> -/* ULX machines are also considered ULT. */
> -#define IS_BDW_ULX(dev_priv)	(IS_BROADWELL(dev_priv) && \
> -				 (INTEL_DEVID(dev_priv) & 0xf) == 0xe)
> +#define IS_BDW_ULT(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, ULT)
> +#define IS_BDW_ULX(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, ULX)
>  #define IS_BDW_GT3(dev_priv)	(IS_BROADWELL(dev_priv) && \
>  				 INTEL_INFO(dev_priv)->gt == 3)
> -#define IS_HSW_ULT(dev_priv)	(IS_HASWELL(dev_priv) && \
> -				 (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0A00)
> +#define IS_HSW_ULT(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, ULT)
>  #define IS_HSW_GT3(dev_priv)	(IS_HASWELL(dev_priv) && \
>  				 INTEL_INFO(dev_priv)->gt == 3)
>  /* ULX machines are also considered ULT. */
> -#define IS_HSW_ULX(dev_priv)	(INTEL_DEVID(dev_priv) == 0x0A0E || \
> -				 INTEL_DEVID(dev_priv) == 0x0A1E)
> -#define IS_SKL_ULT(dev_priv)	(INTEL_DEVID(dev_priv) == 0x1906 || \
> -				 INTEL_DEVID(dev_priv) == 0x1913 || \
> -				 INTEL_DEVID(dev_priv) == 0x1916 || \
> -				 INTEL_DEVID(dev_priv) == 0x1921 || \
> -				 INTEL_DEVID(dev_priv) == 0x1926)
> -#define IS_SKL_ULX(dev_priv)	(INTEL_DEVID(dev_priv) == 0x190E || \
> -				 INTEL_DEVID(dev_priv) == 0x1915 || \
> -				 INTEL_DEVID(dev_priv) == 0x191E)
> -#define IS_KBL_ULT(dev_priv)	(INTEL_DEVID(dev_priv) == 0x5906 || \
> -				 INTEL_DEVID(dev_priv) == 0x5913 || \
> -				 INTEL_DEVID(dev_priv) == 0x5916 || \
> -				 INTEL_DEVID(dev_priv) == 0x5921 || \
> -				 INTEL_DEVID(dev_priv) == 0x5926)
> -#define IS_KBL_ULX(dev_priv)	(INTEL_DEVID(dev_priv) == 0x590E || \
> -				 INTEL_DEVID(dev_priv) == 0x5915 || \
> -				 INTEL_DEVID(dev_priv) == 0x591E)
> -#define IS_AML_ULX(dev_priv)	(INTEL_DEVID(dev_priv) == 0x591C || \
> -				 INTEL_DEVID(dev_priv) == 0x87C0)
> +#define IS_HSW_ULX(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, ULX)
> +#define IS_SKL_ULT(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, ULT)
> +#define IS_SKL_ULX(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, ULX)
> +#define IS_KBL_ULT(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, ULT)
> +#define IS_KBL_ULX(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, ULX)
> +#define IS_AML_ULX(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, AML)
>  #define IS_SKL_GT2(dev_priv)	(IS_SKYLAKE(dev_priv) && \
>  				 INTEL_INFO(dev_priv)->gt == 2)
>  #define IS_SKL_GT3(dev_priv)	(IS_SKYLAKE(dev_priv) && \
> @@ -2457,14 +2445,13 @@ static inline unsigned int i915_sg_segment_size(void)
>  				 INTEL_INFO(dev_priv)->gt == 2)
>  #define IS_KBL_GT3(dev_priv)	(IS_KABYLAKE(dev_priv) && \
>  				 INTEL_INFO(dev_priv)->gt == 3)
> -#define IS_CFL_ULT(dev_priv)	(IS_COFFEELAKE(dev_priv) && \
> -				 (INTEL_DEVID(dev_priv) & 0x00F0) == 0x00A0)
> +#define IS_CFL_ULT(dev_priv)	IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, ULT)
>  #define IS_CFL_GT2(dev_priv)	(IS_COFFEELAKE(dev_priv) && \
>  				 INTEL_INFO(dev_priv)->gt == 2)
>  #define IS_CFL_GT3(dev_priv)	(IS_COFFEELAKE(dev_priv) && \
>  				 INTEL_INFO(dev_priv)->gt == 3)
> -#define IS_CNL_WITH_PORT_F(dev_priv)   (IS_CANNONLAKE(dev_priv) && \
> -					(INTEL_DEVID(dev_priv) & 0x0004) == 0x0004)
> +#define IS_CNL_WITH_PORT_F(dev_priv) \
> +	IS_SUBPLATFORM(dev_priv, INTEL_CANNONLAKE, PORTF)
>  
>  #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)
>  
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 00758d11047b..b9d08428f35b 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -118,6 +118,8 @@ intel_device_info_dump_runtime(const struct intel_runtime_device_info *info,
>  	drm_printf(p, "CS timestamp frequency: %u kHz\n",
>  		   info->cs_timestamp_frequency_khz);
>  
> +	drm_printf(p, "Subplatform mask: %x\n", info->subplatform_mask);
> +
>  	intel_runtime_device_info_dump_flags(info, p);
>  }
>  
> @@ -727,6 +729,69 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
> +{
> +	struct intel_runtime_device_info *info = &i915->runtime_info;
> +	u16 devid = INTEL_DEVID(i915);
> +
> +	if (IS_PINEVIEW(i915)) {
> +		if (devid == 0xa001)
> +			info->subplatform_mask =
> +				BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
> +		else if (devid == 0xa011)
> +			info->subplatform_mask =
> +				BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
> +	} else if (IS_IRONLAKE(i915) && devid == 0x0046) {
> +			info->subplatform_mask =
> +				BIT(INTEL_SUBPLATFORM_IRONLAKE_M);
> +	} else if (IS_HASWELL(i915)) {
> +		if ((devid & 0xFF00) == 0x0A00)
> +			info->subplatform_mask = BIT(INTEL_SUBPLATFORM_ULT);
> +		/* ULX machines are also considered ULT. */
> +		if (devid == 0x0A0E || devid == 0x0A1E)
> +			info->subplatform_mask |= BIT(INTEL_SUBPLATFORM_ULX);
> +	} else if (IS_BROADWELL(i915)) {
> +		if ((devid & 0xf) == 0x6 ||
> +		    (devid & 0xf) == 0xb ||
> +		    (devid & 0xf) == 0xe)
> +			info->subplatform_mask = BIT(INTEL_SUBPLATFORM_ULT);
> +		/* ULX machines are also considered ULT. */
> +		if ((devid & 0xf) == 0xe)
> +			info->subplatform_mask |= BIT(INTEL_SUBPLATFORM_ULX);
> +	} else if (IS_SKYLAKE(i915)) {
> +		if (devid == 0x1906 ||
> +		    devid == 0x1913 ||
> +		    devid == 0x1916 ||
> +		    devid == 0x1921 ||
> +		    devid == 0x1926)
> +			info->subplatform_mask = BIT(INTEL_SUBPLATFORM_ULT);
> +		else if (devid == 0x190E ||
> +			 devid == 0x1915 ||
> +			 devid == 0x191E)
> +			info->subplatform_mask = BIT(INTEL_SUBPLATFORM_ULX);
> +	} else if (IS_KABYLAKE(i915)) {
> +		if (devid == 0x591c ||
> +		    devid == 0x87c0)
> +			info->subplatform_mask = BIT(INTEL_SUBPLATFORM_AML);
> +		else if (devid == 0x5906 ||
> +			 devid == 0x5913 ||
> +			 devid == 0x5916 ||
> +			 devid == 0x5921 ||
> +			 devid  == 0x5926)
> +			info->subplatform_mask = BIT(INTEL_SUBPLATFORM_ULT);
> +		else if (devid == 0x590E ||
> +			 devid == 0x5915 ||
> +			 devid == 0x591E)
> +			info->subplatform_mask = BIT(INTEL_SUBPLATFORM_ULX);
> +	} else if (IS_COFFEELAKE(i915)) {
> +		if ((devid & 0x00F0) == 0x00A0)
> +			info->subplatform_mask = BIT(INTEL_SUBPLATFORM_ULT);
> +	} else if (IS_CANNONLAKE(i915)) {
> +		if ((devid & 0x0004) == 0x0004)
> +			info->subplatform_mask = BIT(INTEL_SUBPLATFORM_PORTF);
> +	}
> +}
> +
>  /**
>   * intel_device_info_runtime_init - initialize runtime info
>   * @info: intel device info struct
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index f9e577ccf775..91f163039949 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -182,10 +182,27 @@ struct intel_device_info {
>  	func(has_rc6); \
>  	func(has_rc6p); \
>  
> +/*
> + * Subplatform bits share the same namespace per parent platform. In other words
> + * it is fine for the same bit to be used on multiple parent platform.
> + */
> +#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
> +
> +#define INTEL_SUBPLATFORM_PINEVIEW_G (0)
> +#define INTEL_SUBPLATFORM_PINEVIEW_M (1)
> +
> +/* SKL/KBL */
> +#define INTEL_SUBPLATFORM_ULT (0)
> +#define INTEL_SUBPLATFORM_ULX (1)
> +#define INTEL_SUBPLATFORM_AML (2)
> +
> +#define INTEL_SUBPLATFORM_PORTF (0)
> +
>  struct intel_runtime_device_info {
>  	int gen;
>  
>  	u32 platform_mask;
> +	u32 subplatform_mask;
>  
>  	unsigned int num_rings;
>  
> @@ -270,6 +287,7 @@ static inline void sseu_set_eus(struct sseu_dev_info *sseu,
>  const char *intel_platform_name(enum intel_platform platform);
>  
>  void intel_device_info_runtime_init(struct drm_i915_private *dev_priv);
> +void intel_device_info_subplatform_init(struct drm_i915_private *dev_priv);
>  void intel_device_info_dump(struct drm_i915_private *dev_priv,
>  			    struct drm_printer *p);
>  void intel_device_info_dump_flags(const struct intel_device_info *info,

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx