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

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

On 15/03/2019 18:40, Lucas De Marchi wrote:
> On Fri, Mar 15, 2019 at 05:31:05PM +0000, Tvrtko Ursulin wrote:
>>
>> 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@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)],
>>>
>>> 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.
> 
> I think I missed a "(" and thought that would be (32 / 32) - 3
> 
> anyway, you are printing confusing information here since you only print
> one u32.

Confusing how and what is the second u32? You mean when array gets 
expanded in the future? Here the actual idea (see next version) is to 
only print the 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__)); \
>>>
>>>
>>> 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.
> 
> I challenge that. My butt feeling here is pretty strong that's not
> true.

I hope you meant gut feeling. Anyways, I said I wasn't 100% sure of the 
details, right? Having exercised my memory lanes I think the biggest 
negative effect of the move to u64 was in conjunction with this 
subplatform idea. This is due the "if (mask & platform) && (mask & 
subplatform)" expanding to 2 u64 constants per check. I have a remedy 
for that, but lets not get bogged down and code size which is not the 
main consideration here. Idea was to consolidate and simplify whilst 
preserving the two beneficial effects which I already explain.

>> Benefit of the u32 array approach is that it avoids that even on 
>> 64-bit builds.
> 
> my point is on 64-bit builds... It may suffer a little on m32.

Which is why I hinted the u32 array approach does not suffer on either. 
No increase on 64-bit, no real increase on 32-bit.

Regards,

Tvrtko

> 
> Lucas De Marchi
> 
>> 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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx