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

[Tvrtko Ursulin]

On 15/03/2019 17:12, Lucas De Marchi wrote:
> On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at 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 at intel.com>
>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Cc: Jose Souza <jose.souza at 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)],
> 
> bug here, INTEL_SUBPLATFORM_BITS should be outside of []. Bad things
> will happen for platform=32 /o\

? [32 / (32 - 3)] = [1], for which there is a BUILD_BUG_ON with a 
comment saying to increase size of array.

> 
>>                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__)); \
> 
> 
> Ugh. That double dword fiddling is way too ugly. IMO it is not buying us
> anything. Just use a u64 rather than the double dword. Your approach may
> have a small benefit on ARCH=i386, but has the burden of carrying all
> this forward.  The diff below (only build-tested) is on top of yours,
> which is basically equivalent to "move to u64 and then add the
> subplatform part".
> 
>    text    data     bss     dec     hex filename
> 1834620   40454    4176 1879250  1cacd2 drivers/gpu/drm/i915/i915.o.yours
> 1834710   40454    4176 1879340  1cad2c drivers/gpu/drm/i915/i915.o

The cost of going u64 would be higher than what you saw if bits above 
were actually used I think. But would have to check the output to be 
sure. It was at least a year ago I think I last played with this.

Benefit of the u32 array approach is that it avoids that even on 64-bit 
builds.

As it stands v5 of my patch has minimal positive effect on code size 
(sub 1k). Maybe a bit better in non-debug builds. But the main point is 
about the devid checking consolidation.

It is of course open to discussion.

Regards,

Tvrtko