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

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

On 15/03/2019 13:16, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-15 12:26:33)
>> 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)],
> 
> I hope that's a one-off!

It's indeed horrible. I was thinking of adding a helper but decided to 
wait for some general acks first.

>>                             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; \
> 
> Oh, p is a compile time constant, so these can all be evaluated at
> compile time. So not a worry.
> 
>> +\
>> +       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);
>> +}
> 
> Ok, that all makes sense as a custom bitmap.

Hm head scratch.. why I needed macro and a static inline? I'll check if 
I can get away with only the latter. This fiddling was all about keeping 
WARN_ON strings readable (and short!).

> 
>> +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);
> 
> if (IS_PINEVIEW(i915)) {
> 	int subplatform = 0;
> 
> 	if (devid == 0xa001)
> 		subplatform = INTEL_SUBPLATFORM_PINEVIEW_G;
> 	else if (devid == 0xa001)
> 		subplatform = INTEL_SUBPLATFORM_PINEVIEW_M;
> 	else
> 		MISSING_CASE(devid);

With MISSING_CASE you are talking specifically for Pineview since it 
only has a total of two ids? You expect more parts to ship? :))

Otherwise the logic here is only to mention ids which are special, not 
all platform knows of. So as a general pattern MISSING_CASE wouldn't work.

> 
> 	info->platform_mask[pi] |= BIT(subplatform);
> 	WARN_ON(!IS_SUBPLATFORM(i915, INTEL_PLATFORM_PINEVIEW, subplatform));
> }
> 
> So we catch missing ids, and validate subplatform against
> SUBPLATFORM_BITS.

Not so straightforward with the ULT and ULX bunch since some of those 
are both.

What I have added locally however is this:

	GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_BITS);

	RUNTIME_INFO(i915)->platform_mask[pi] |= mask;

Moved the mask assignment to end so that I can check subplatform did not 
overflow the allocated space.

>>   /**
>>    * 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,
> 
> Looks like you are just keeping a tally, and no I have no idea how to
> add a comment to make that clear.
> 
> /* tally */
> /* tally so far */

Yeah.. I kept re-counting to see how it will look when the array is 
expanded, whether I should reserve more suplatfoms bits right now for 
cut-off to fall somewhere reasonable.

I can remove these markers, they are not that useful.

> 
>>          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)
> 
> I can't believe you haven't done i852/i855! (Or whatever that variant
> was called.)

IS_I85X? I don't see any sub-platforms there.

> Couldn't spot anything wrong, and so long as subplatform remains clear
> in the error state, hint hint, I'm happy.

Like how? With a string like platform names? Would be possible but is it 
really needed on top of devid?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx