Re: [PATCH] drm/i915: Introduce concept of a sub-platform

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/03/2019 14:09, Ville Syrjälä wrote:
> On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Concept of a sub-platform already exist in our code (like ULX and ULT
>> platform variants and similar),implemented via the macros which check a
>> list of device ids to determine a match.
>>
>> With this patch we consolidate device ids checking into a single function
>> called during early driver load.
>>
>> A few low bits in the platform mask are reserved for sub-platform
>> identification and defined as a per-platform namespace.
>>
>> At the same time it future proofs the platform_mask handling by preparing
>> the code for easy extending, and tidies the very verbose WARN strings
>> generated when IS_PLATFORM macros are embedded into a WARN type
>> statements.
>>
>> The approach is also beneficial to driver size, with an combined shrink of
>> code and strings of around 1.7 kiB.
>>
>> v2: Fixed IS_SUBPLATFORM. Updated commit msg.
>> v3: Chris was right, there is an ordering problem.
>>
>> v4:
>>   * Catch-up with new sub-platforms.
>>   * Rebase for RUNTIME_INFO.
>>   * Drop subplatform mask union tricks and convert platform_mask to an
>>     array for extensibility.
>>
>> 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>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Jose Souza <jose.souza@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c          |   7 +-
>>   drivers/gpu/drm/i915/i915_drv.h          | 110 +++++++++++++++--------
>>   drivers/gpu/drm/i915/i915_pci.c          |   2 +-
>>   drivers/gpu/drm/i915/intel_device_info.c |  79 ++++++++++++++++
>>   drivers/gpu/drm/i915/intel_device_info.h |  28 +++++-
>>   5 files changed, 179 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 0d743907e7bc..3218350cd225 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -863,6 +863,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
>>   	if (i915_inject_load_failure())
>>   		return -ENODEV;
>>   
>> +	intel_device_info_subplatform_init(dev_priv);
>> +
>>   	spin_lock_init(&dev_priv->irq_lock);
>>   	spin_lock_init(&dev_priv->gpu_error.lock);
>>   	mutex_init(&dev_priv->backlight_lock);
>> @@ -1752,10 +1754,11 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv)
>>   	if (drm_debug & DRM_UT_DRIVER) {
>>   		struct drm_printer p = drm_debug_printer("i915 device info:");
>>   
>> -		drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s gen=%i\n",
>> +		drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s (%x) gen=%i\n",
>>   			   INTEL_DEVID(dev_priv),
>>   			   INTEL_REVID(dev_priv),
>>   			   intel_platform_name(INTEL_INFO(dev_priv)->platform),
>> +			   RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
>>   			   INTEL_GEN(dev_priv));
>>   
>>   		intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>> @@ -1798,8 +1801,6 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   	memcpy(device_info, match_info, sizeof(*device_info));
>>   	RUNTIME_INFO(i915)->device_id = pdev->device;
>>   
>> -	BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
>> -		     BITS_PER_TYPE(device_info->platform_mask));
>>   	BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask));
>>   
>>   	return i915;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index dccb6006aabf..34282cf66cb0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2281,7 +2281,46 @@ static inline unsigned int i915_sg_segment_size(void)
>>   #define IS_REVID(p, since, until) \
>>   	(INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
>>   
>> -#define IS_PLATFORM(dev_priv, p) (INTEL_INFO(dev_priv)->platform_mask & BIT(p))
>> +#define __IS_PLATFORM(dev_priv, p) \
>> +({ \
>> +	const unsigned int pbits__ = \
>> +		BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>> +		INTEL_SUBPLATFORM_BITS; \
>> +	const unsigned int pi__ = (p) / pbits__; \
>> +	const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
>> +\
>> +	BUILD_BUG_ON(!__builtin_constant_p(p)); \
>> +\
>> +	(RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
>> +})
>> +
>> +#define __IS_SUBPLATFORM(dev_priv, p, s) \
>> +({ \
>> +	const unsigned int pbits__ = \
>> +		BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>> +		INTEL_SUBPLATFORM_BITS; \
>> +	const unsigned int pi__ = (p) / pbits__; \
>> +	const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
>> +\
>> +	BUILD_BUG_ON(!__builtin_constant_p(p)); \
>> +	BUILD_BUG_ON(!__builtin_constant_p(s)); \
>> +	BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
>> +\
>> +	(RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
>> +})
>> +
>> +static inline bool
>> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
>> +{
>> +	return __IS_PLATFORM(i915, p);
>> +}
>> +
>> +static inline bool
>> +IS_SUBPLATFORM(const struct drm_i915_private *i915,
>> +	       enum intel_platform p, unsigned int s)
>> +{
>> +	return __IS_SUBPLATFORM(i915, p, s);
>> +}
>>   
>>   #define IS_I830(dev_priv)	IS_PLATFORM(dev_priv, INTEL_I830)
>>   #define IS_I845G(dev_priv)	IS_PLATFORM(dev_priv, INTEL_I845G)
>> @@ -2296,11 +2335,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, INTEL_SUBPLATFORM_PINEVIEW_G)
>> +#define IS_PINEVIEW_M(dev_priv)	\
>> +	IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_M)
>>   #define IS_PINEVIEW(dev_priv)	IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
> 
> I have a feeling we should just use the normal IS_MOBILE() thing
> here. But untangling that mess might be a bit of work.
> 
>>   #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, INTEL_SUBPLATFORM_IRONLAKE_M)
> 
> This is clearly just IS_MOBILE().

Ok.

> Or we just keep the current pci id checks. Frankly I see no benefit in
> abstracting these few exceptions.

Are you referring to Ironlake only or in general?

> 
>>   #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)
>> @@ -2318,42 +2361,31 @@ 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, INTEL_SUBPLATFORM_ULT)
>> +#define IS_BDW_ULX(dev_priv) \
>> +	IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, INTEL_SUBPLATFORM_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, INTEL_SUBPLATFORM_ULT)
>>   #define IS_HSW_GT3(dev_priv)	(IS_HASWELL(dev_priv) && \
>>   				 INTEL_INFO(dev_priv)->gt == 3)
>>   #define IS_HSW_GT1(dev_priv)	(IS_HASWELL(dev_priv) && \
>>   				 INTEL_INFO(dev_priv)->gt == 1)
>>   /* 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, INTEL_SUBPLATFORM_ULX)
>> +#define IS_SKL_ULT(dev_priv) \
>> +	IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULT)
>> +#define IS_SKL_ULX(dev_priv) \
>> +	IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULX)
>> +#define IS_KBL_ULT(dev_priv) \
>> +	IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULT)
>> +#define IS_KBL_ULX(dev_priv) \
>> +	IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULX)
>> +#define IS_AML_ULX(dev_priv) \
>> +	IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_AML_ULX)
> 
> Why is AML_ULX different from normal ULX?

As far as I could it is a different devid which we officially call 
Amberlake, but did not add it as separate platform.

Given how this macro is used, which is always in conjuction with 
IS_KBL_ULX, I considered just removing it and adding the devid to 
KBL_ULX, but did not want to interferre with other people's ideas.

> 
>>   #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) && \
>> @@ -2364,16 +2396,16 @@ 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, INTEL_SUBPLATFORM_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_ICL_WITH_PORT_F(dev_priv)   (IS_ICELAKE(dev_priv) && \
>> -					INTEL_DEVID(dev_priv) != 0x8A51)
>> +#define IS_CNL_WITH_PORT_F(dev_priv) \
>> +	IS_SUBPLATFORM(dev_priv, INTEL_CANNONLAKE, INTEL_SUBPLATFORM_PORTF)
>> +#define IS_ICL_WITH_PORT_F(dev_priv) \
>> +	IS_SUBPLATFORM(dev_priv, INTEL_ICELAKE, INTEL_SUBPLATFORM_PORTF)
> 
> God that PORTF thing is ugly. I would suggest we just add port_mask
> or something along those lines into the device info.
> 
> Then we could perhaps even avoid adding more and more branches to
> intel_setup_outputs(). Although I'm not sure we need anything for that
> since we'll look at the VBT anyway (which *maybe* doesn't advertize
> ports that aren't present?).

No idea, sounds like a question for you display folks to decide 
separately of this proposal probably?

> 
>>   
>>   #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>> index 3cf697e8f1fa..2b042618e3ca 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -32,7 +32,7 @@
>>   #include "i915_globals.h"
>>   #include "i915_selftest.h"
>>   
>> -#define PLATFORM(x) .platform = (x), .platform_mask = BIT(x)
>> +#define PLATFORM(x) .platform = (x)
>>   #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
>>   
>>   #define I845_PIPE_OFFSETS \
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
>> index aac19b1c419c..b8a7e17cb443 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -707,6 +707,85 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
>>   	return 0;
>>   }
>>   
>> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
>> +{
>> +	const unsigned int pbits =
>> +		BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
>> +		INTEL_SUBPLATFORM_BITS;
>> +	const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
>> +	const unsigned int pb =
>> +		INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
>> +	struct intel_runtime_info *info = RUNTIME_INFO(i915);
>> +	u16 devid = INTEL_DEVID(i915);
>> +
>> +	BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
>> +		     pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
>> +
>> +	info->platform_mask[pi] = BIT(pb);
>> +
>> +	if (IS_PINEVIEW(i915)) {
>> +		if (devid == 0xa001)
>> +			info->platform_mask[pi] |=
>> +				BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
>> +		else if (devid == 0xa011)
>> +			info->platform_mask[pi] |=
>> +				BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
>> +	} else if (IS_IRONLAKE(i915)) {
>> +		if (devid == 0x0046)
>> +			info->platform_mask[pi] |=
>> +				BIT(INTEL_SUBPLATFORM_IRONLAKE_M);
>> +	} else if (IS_HASWELL(i915)) {
>> +		if ((devid & 0xFF00) == 0x0A00)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> +		/* ULX machines are also considered ULT. */
>> +		if (devid == 0x0A0E || devid == 0x0A1E)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
>> +	} else if (IS_BROADWELL(i915)) {
>> +		if ((devid & 0xf) == 0x6 ||
>> +		    (devid & 0xf) == 0xb ||
>> +		    (devid & 0xf) == 0xe)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> +		/* ULX machines are also considered ULT. */
>> +		if ((devid & 0xf) == 0xe)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
>> +	} else if (IS_SKYLAKE(i915)) {
>> +		if (devid == 0x1906 ||
>> +		    devid == 0x1913 ||
>> +		    devid == 0x1916 ||
>> +		    devid == 0x1921 ||
>> +		    devid == 0x1926)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> +		else if (devid == 0x190E ||
>> +			 devid == 0x1915 ||
>> +			 devid == 0x191E)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
>> +	} else if (IS_KABYLAKE(i915)) {
>> +		if (devid == 0x5906 ||
>> +		    devid == 0x5913 ||
>> +		    devid == 0x5916 ||
>> +		    devid == 0x5921 ||
>> +		    devid  == 0x5926)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> +		else if (devid == 0x590E ||
>> +			 devid == 0x5915 ||
>> +			 devid == 0x591E)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
>> +		else if (devid == 0x591C ||
>> +			 devid == 0x87C0)
>> +			info->platform_mask[pi] |=
>> +				BIT(INTEL_SUBPLATFORM_AML_ULX);
>> +	} else if (IS_COFFEELAKE(i915)) {
>> +		if ((devid & 0x00F0) == 0x00A0)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> +	} else if (IS_CANNONLAKE(i915)) {
>> +		if ((devid & 0x0004) == 0x0004)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
>> +	} else if (IS_ICELAKE(i915)) {
>> +		if (devid != 0x8A51)
>> +			info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
>> +	}
>> +}
>> +
>>   /**
>>    * intel_device_info_runtime_init - initialize runtime info
>>    * @dev_priv: the i915 device
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
>> index 047d10bdd455..b03fbd2e451a 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>> @@ -44,7 +44,7 @@ enum intel_platform {
>>   	INTEL_I915G,
>>   	INTEL_I915GM,
>>   	INTEL_I945G,
>> -	INTEL_I945GM,
>> +	INTEL_I945GM = 8,
>>   	INTEL_G33,
>>   	INTEL_PINEVIEW,
>>   	/* gen4 */
>> @@ -55,7 +55,7 @@ enum intel_platform {
>>   	/* gen5 */
>>   	INTEL_IRONLAKE,
>>   	/* gen6 */
>> -	INTEL_SANDYBRIDGE,
>> +	INTEL_SANDYBRIDGE = 16,
>>   	/* gen7 */
>>   	INTEL_IVYBRIDGE,
>>   	INTEL_VALLEYVIEW,
>> @@ -66,7 +66,7 @@ enum intel_platform {
>>   	/* gen9 */
>>   	INTEL_SKYLAKE,
>>   	INTEL_BROXTON,
>> -	INTEL_KABYLAKE,
>> +	INTEL_KABYLAKE = 24,
> 
> Too much magic. Looks rather fragile.

Imagine these hunks gone. This is not required for this patch at all, or 
for anything for that matter. What do you think is magic and fragile?

Regards,

Tvrtko

> 
>>   	INTEL_GEMINILAKE,
>>   	INTEL_COFFEELAKE,
>>   	/* gen10 */
>> @@ -76,6 +76,24 @@ enum intel_platform {
>>   	INTEL_MAX_PLATFORMS
>>   };
>>   
>> +/*
>> + * 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 platforms.
>> + */
>> +
>> +#define INTEL_SUBPLATFORM_BITS (3)
>> +
>> +#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
>> +
>> +#define INTEL_SUBPLATFORM_PINEVIEW_G (0)
>> +#define INTEL_SUBPLATFORM_PINEVIEW_M (1)
>> +
>> +#define INTEL_SUBPLATFORM_ULT	  (0)
>> +#define INTEL_SUBPLATFORM_ULX	  (1)
>> +#define INTEL_SUBPLATFORM_AML_ULX (2)
>> +
>> +#define INTEL_SUBPLATFORM_PORTF (0)
>> +
>>   enum intel_ppgtt {
>>   	INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE,
>>   	INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING,
>> @@ -160,7 +178,6 @@ struct intel_device_info {
>>   	intel_engine_mask_t engine_mask; /* Engines supported by the HW */
>>   
>>   	enum intel_platform platform;
>> -	u32 platform_mask;
>>   
>>   	enum intel_ppgtt ppgtt;
>>   	unsigned int page_sizes; /* page sizes supported by the HW */
>> @@ -195,6 +212,8 @@ struct intel_device_info {
>>   };
>>   
>>   struct intel_runtime_info {
>> +	u32 platform_mask[1];
>> +
>>   	u16 device_id;
>>   
>>   	u8 num_sprites[I915_MAX_PIPES];
>> @@ -269,6 +288,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_subplatform_init(struct drm_i915_private *dev_priv);
>>   void intel_device_info_runtime_init(struct drm_i915_private *dev_priv);
>>   void intel_device_info_dump_flags(const struct intel_device_info *info,
>>   				  struct drm_printer *p);
>> -- 
>> 2.19.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx