Re: [PATCH v2] drm/i915: use unsigned long for platform_mask

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

On 03/04/2019 09:15, Lucas De Marchi wrote:
> On Tue, Apr 2, 2019 at 11:58 PM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 03/04/2019 02:46, Lucas De Marchi wrote:
>>> No reason to stick to u32 for platform mask if we can just use more bits
>>> on 64 bit platforms.
>>>
>>> $ size drivers/gpu/drm/i915/i915.ko*
>>>      text         data     bss     dec     hex filename
>>> 1884779         41334    5408 1931521  1d7901 drivers/gpu/drm/i915/i915.ko
>>> 1886693         41358    5408 1933459  1d8093 drivers/gpu/drm/i915/i915.ko.old
>>
>> How did you get such a large difference, and decrease even? Could you
>> check in the code what is happening? Because I get an increase with this
>> patch:
>>
>>      text    data     bss     dec     hex filename
>> 1905314   43903    7424 1956641  1ddb21 i915.ko.orig
>> 1905796   43903    7424 1957123  1ddd03 i915.ko.patch
> 
> the only explanation I really have is that my measurement was bogus.
> Some possible explanations...
> 1) I compared a i386 to a x86-64 build; 2) somehow a config changed
> between the builds;
> 3) when preparing the patch I rebased on upstream between the builds.
> 
> Checking (1), no... that's in the ~400k range. So no idea, sorry.

I was worried you'd say you compiler just behaves differently. To 
eliminate this option it would still be good to double check if you can 
find the time.

> So I think the only useful thing in this patch is to make the array to
> grow automatically. Or maybe not even that?

I really liked that and then started thinking that it can still sneak up 
a mistake if one changes the type of the member and forgets to change 
the type in size calculation BITS_PER_TYPE. So I ended up a little less 
sure. Could the calculation self-reference the struct member?

Regards,

Tvrtko

> 
> Lucas De Marchi
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Now on 64 bits we have only one long as opposed to 2 u32:
>>>
>>> $ pahole -C intel_runtime_info drivers/gpu/drm/i915/i915.ko
>>> struct intel_runtime_info {
>>>        long unsigned int          platform_mask[1];     /*     0     8 */
>>> ...
>>> }
>>>
>>> On 32 bits we still have the same thing as before:
>>> $ size drivers/gpu/drm/i915/i915.ko*
>>>      text         data     bss     dec     hex filename
>>> 1489839         32485    2816 1525140  174594 drivers/gpu/drm/i915/i915.ko
>>> 1489839         32485    2816 1525140  174594 drivers/gpu/drm/i915/i915.ko.old
>>>
>>> Besides reducing the code on x86-64 now the array size is automatically
>>> calculated and we don't have to worry about extending it anymore.
>>>
>>> v2: fix sparse and checkpatch warnings
>>>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_drv.h          | 6 +-----
>>>    drivers/gpu/drm/i915/intel_device_info.h | 7 +++----
>>>    2 files changed, 4 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 0ab4826921f7..9fe765ffe878 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2309,10 +2309,6 @@ __platform_mask_index(const struct intel_runtime_info *info,
>>>        const unsigned int pbits =
>>>                BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS;
>>>
>>> -     /* Expand the platform_mask array if this fails. */
>>> -     BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
>>> -                  pbits * ARRAY_SIZE(info->platform_mask));
>>> -
>>>        return p / pbits;
>>>    }
>>>
>>> @@ -2354,7 +2350,7 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>>>        const unsigned int pi = __platform_mask_index(info, p);
>>>        const unsigned int pb = __platform_mask_bit(info, p);
>>>        const unsigned int msb = BITS_PER_TYPE(info->platform_mask[0]) - 1;
>>> -     const u32 mask = info->platform_mask[pi];
>>> +     const unsigned long mask = info->platform_mask[pi];
>>>
>>>        BUILD_BUG_ON(!__builtin_constant_p(p));
>>>        BUILD_BUG_ON(!__builtin_constant_p(s));
>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
>>> index 0e579f158016..2f5ca2b6f094 100644
>>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>>> @@ -214,11 +214,10 @@ struct intel_runtime_info {
>>>         * Platform mask is used for optimizing or-ed IS_PLATFORM calls into
>>>         * into single runtime conditionals, and also to provide groundwork
>>>         * for future per platform, or per SKU build optimizations.
>>> -      *
>>> -      * Array can be extended when necessary if the corresponding
>>> -      * BUILD_BUG_ON is hit.
>>>         */
>>> -     u32 platform_mask[2];
>>> +     unsigned long platform_mask[DIV_ROUND_UP(INTEL_MAX_PLATFORMS,
>>> +                                              BITS_PER_TYPE(unsigned long)
>>> +                                              - INTEL_SUBPLATFORM_BITS)];
>>>
>>>        u16 device_id;
>>>
>>>
>>
>> _______________________________________________
>> 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