diff mbox series

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

Message ID 20181112171242.7640-7-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series mkwrite_device_info removal | expand

Commit Message

Tvrtko Ursulin Nov. 12, 2018, 5:12 p.m. UTC 
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.

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(-)

Comments

Chris Wilson Nov. 12, 2018, 5:29 p.m. UTC | #1 
Quoting Tvrtko Ursulin (2018-11-12 17:12:41)
> 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);

Any chance for a magic decoder ring? Quick identification of ult/ulx I
think would be very nice.
-Chris
Tvrtko Ursulin Nov. 13, 2018, 9:17 a.m. UTC | #2 
On 12/11/2018 17:29, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-11-12 17:12:41)
>> 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);
> 
> Any chance for a magic decoder ring? Quick identification of ult/ulx I
> think would be very nice.

Yes I wasn't happy with not doing that myself. So I think it is 
definitely needed.

Regards,

Tvrtko
Jani Nikula Nov. 13, 2018, 11:40 a.m. UTC | #3 
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,
Tvrtko Ursulin Nov. 13, 2018, 5:11 p.m. UTC | #4 
On 13/11/2018 11:40, Jani Nikula wrote:
> 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.

It would be much nicer, but how would you do it? Perhaps my imagination 
is just strong enough today.

Simply by splitting the id's into subplatform parts, for instance where 
we have today:

#define INTEL_BDW_GT1_IDS(info)  \
         INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
         INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
         INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
         INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
         INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
         INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */

We'd split to:

#define INTEL_BDW_GT1_ULT_IDS(info)  \
         INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
         INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \

#define INTEL_BDW_GT1_ULX_IDS(info)  \
         INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \

#define INTEL_BDW_GT1_IDS(info)  \
         INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
         INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
         INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */

Then in i915_pci.c, instead of:

	...
	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
	...

We'd have:

	...
	INTEL_BDW_GT1_ULT_IDS(&intel_broadwell_gt1_info),
	INTEL_BDW_GT1_ULX_IDS(&intel_broadwell_gt1_info),
	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
	...

And a separate table to map the id's to subplatform values.

Hmm, but we would probably need to extrac the id's from the 
INTEL_BDW1_GT_IDS like macros so they can be used in this second site 
without the info parameter. Something like the trick for device info 
flags, but can it be made to generate a macro? I think not..

Regards,

Tvrtko
Jani Nikula Nov. 13, 2018, 10:28 p.m. UTC | #5 
On Tue, 13 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 13/11/2018 11:40, Jani Nikula wrote:
>> 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.
>
> It would be much nicer, but how would you do it? Perhaps my imagination 
> is just strong enough today.

So here's an idea.

>
> Simply by splitting the id's into subplatform parts, for instance where 
> we have today:
>
> #define INTEL_BDW_GT1_IDS(info)  \
>          INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>          INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>          INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>          INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
>          INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>          INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */
>
> We'd split to:
>
> #define INTEL_BDW_GT1_ULT_IDS(info)  \
>          INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>          INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>
> #define INTEL_BDW_GT1_ULX_IDS(info)  \
>          INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \

So far so good.

>
> #define INTEL_BDW_GT1_IDS(info)  \
>          INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>          INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>          INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */

Now include INTEL_BDW_GT1_ULT_IDS(info) and INTEL_BDW_GT1_ULX_IDS(info)
to the above...

>
> Then in i915_pci.c, instead of:
>
> 	...
> 	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
> 	...
>
> We'd have:
>
> 	...
> 	INTEL_BDW_GT1_ULT_IDS(&intel_broadwell_gt1_info),
> 	INTEL_BDW_GT1_ULX_IDS(&intel_broadwell_gt1_info),
> 	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
> 	...

...so you don't need to make this change at all. But that's a minor
detail.

> And a separate table to map the id's to subplatform values.
>
> Hmm, but we would probably need to extrac the id's from the 
> INTEL_BDW1_GT_IDS like macros so they can be used in this second site 
> without the info parameter. Something like the trick for device info 
> flags, but can it be made to generate a macro? I think not..

Are we shy of macro magic? Pfft.

#undef INTEL_VGA_DEVICE
#define INTEL_VGA_DEVICE(id, info) (id)

static const u32 bdw_ult_ids[] = {
	INTEL_BDW_GT1_ULT_IDS(0),
};

static const u32 bdw_ulx_ids[] = {
	INTEL_BDW_GT1_ULX_IDS(0),
};

#undef INTEL_VGA_DEVICE

Now you can add another mapping on top with pointers to similar arrays
as above and corresponding subplatform bits. Just need to order the code
to not clobber the real INTEL_VGA_DEVICE needs.

We don't need to split the ult/ulx tables by platform either if we only
care about the subplatform ult/ulx here, just need to remember add all
ult/ulx in corresponding arrays.


BR,
Jani.
Tvrtko Ursulin Nov. 15, 2018, 11:03 a.m. UTC | #6 
On 13/11/2018 22:28, Jani Nikula wrote:
> On Tue, 13 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 13/11/2018 11:40, Jani Nikula wrote:
>>> 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.
>>
>> It would be much nicer, but how would you do it? Perhaps my imagination
>> is just strong enough today.
> 
> So here's an idea.
> 
>>
>> Simply by splitting the id's into subplatform parts, for instance where
>> we have today:
>>
>> #define INTEL_BDW_GT1_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>>           INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>>           INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>>           INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
>>           INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>>           INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */
>>
>> We'd split to:
>>
>> #define INTEL_BDW_GT1_ULT_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>>           INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>>
>> #define INTEL_BDW_GT1_ULX_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
> 
> So far so good.
> 
>>
>> #define INTEL_BDW_GT1_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>>           INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>>           INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */
> 
> Now include INTEL_BDW_GT1_ULT_IDS(info) and INTEL_BDW_GT1_ULX_IDS(info)
> to the above...
> 
>>
>> Then in i915_pci.c, instead of:
>>
>> 	...
>> 	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
>> 	...
>>
>> We'd have:
>>
>> 	...
>> 	INTEL_BDW_GT1_ULT_IDS(&intel_broadwell_gt1_info),
>> 	INTEL_BDW_GT1_ULX_IDS(&intel_broadwell_gt1_info),
>> 	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
>> 	...
> 
> ...so you don't need to make this change at all. But that's a minor
> detail.

Indeed, makes the change less intrusive.

>> And a separate table to map the id's to subplatform values.
>>
>> Hmm, but we would probably need to extrac the id's from the
>> INTEL_BDW1_GT_IDS like macros so they can be used in this second site
>> without the info parameter. Something like the trick for device info
>> flags, but can it be made to generate a macro? I think not..
> 
> Are we shy of macro magic? Pfft.
> 
> #undef INTEL_VGA_DEVICE
> #define INTEL_VGA_DEVICE(id, info) (id)
> 
> static const u32 bdw_ult_ids[] = {
> 	INTEL_BDW_GT1_ULT_IDS(0),
> };
> 
> static const u32 bdw_ulx_ids[] = {
> 	INTEL_BDW_GT1_ULX_IDS(0),
> };
> 
> #undef INTEL_VGA_DEVICE
> 
> Now you can add another mapping on top with pointers to similar arrays
> as above and corresponding subplatform bits. Just need to order the code
> to not clobber the real INTEL_VGA_DEVICE needs.
> 
> We don't need to split the ult/ulx tables by platform either if we only
> care about the subplatform ult/ulx here, just need to remember add all
> ult/ulx in corresponding arrays.

Nice and simple, thank you!

I am marking this for when I get round updating the series.

Regards,

Tvrtko
Tvrtko Ursulin March 25, 2019, 6 p.m. UTC | #7 
On 13/11/2018 22:28, Jani Nikula wrote:
> On Tue, 13 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 13/11/2018 11:40, Jani Nikula wrote:
>>> 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.
>>
>> It would be much nicer, but how would you do it? Perhaps my imagination
>> is just strong enough today.
> 
> So here's an idea.
> 
>>
>> Simply by splitting the id's into subplatform parts, for instance where
>> we have today:
>>
>> #define INTEL_BDW_GT1_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>>           INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>>           INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>>           INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
>>           INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>>           INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */
>>
>> We'd split to:
>>
>> #define INTEL_BDW_GT1_ULT_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>>           INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>>
>> #define INTEL_BDW_GT1_ULX_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
> 
> So far so good.
> 
>>
>> #define INTEL_BDW_GT1_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>>           INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>>           INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */
> 
> Now include INTEL_BDW_GT1_ULT_IDS(info) and INTEL_BDW_GT1_ULX_IDS(info)
> to the above...
> 
>>
>> Then in i915_pci.c, instead of:
>>
>> 	...
>> 	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
>> 	...
>>
>> We'd have:
>>
>> 	...
>> 	INTEL_BDW_GT1_ULT_IDS(&intel_broadwell_gt1_info),
>> 	INTEL_BDW_GT1_ULX_IDS(&intel_broadwell_gt1_info),
>> 	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
>> 	...
> 
> ...so you don't need to make this change at all. But that's a minor
> detail.
> 
>> And a separate table to map the id's to subplatform values.
>>
>> Hmm, but we would probably need to extrac the id's from the
>> INTEL_BDW1_GT_IDS like macros so they can be used in this second site
>> without the info parameter. Something like the trick for device info
>> flags, but can it be made to generate a macro? I think not..
> 
> Are we shy of macro magic? Pfft.
> 
> #undef INTEL_VGA_DEVICE
> #define INTEL_VGA_DEVICE(id, info) (id)
> 
> static const u32 bdw_ult_ids[] = {
> 	INTEL_BDW_GT1_ULT_IDS(0),
> };
> 
> static const u32 bdw_ulx_ids[] = {
> 	INTEL_BDW_GT1_ULX_IDS(0),
> };
> 
> #undef INTEL_VGA_DEVICE
> 
> Now you can add another mapping on top with pointers to similar arrays
> as above and corresponding subplatform bits. Just need to order the code
> to not clobber the real INTEL_VGA_DEVICE needs.
> 
> We don't need to split the ult/ulx tables by platform either if we only
> care about the subplatform ult/ulx here, just need to remember add all
> ult/ulx in corresponding arrays.

I eventually remember this thread/idea and have incorporated it into the 
latest series. Only on trybot for now, but you can have a peek if you 
want to see if it matches your expectations.

Regards,

Tvrtko
diff mbox series

Patch

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,