Re: [PATCH 3/3] drm/i915: Allow optimized platform checks

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

On 20/09/2017 10:56, Tvrtko Ursulin wrote:
> On 20/09/2017 10:39, Jani Nikula wrote:
>> On Wed, 20 Sep 2017, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> If we store the platform as a bitmask, and convert the
>>> IS_PLATFORM macro to use it, we allow the compiler to
>>> merge the IS_PLATFORM(a) || IS_PLATFORM(b) || ... checks
>>> into a single conditional.
>>>
>>> Even with the added BUG_ON this saves almost 1k of text:
>>>
>>>      text           data     bss     dec     hex filename
>>> -1460254          60014    3656 1523924  1740d4 
>>> drivers/gpu/drm/i915/i915.ko
>>> +1459260          60026    3656 1522942  173cfe 
>>> drivers/gpu/drm/i915/i915.ko
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
>>>   drivers/gpu/drm/i915/i915_drv.h | 3 ++-
>>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index b1f96eb1be16..c3bd4b7cb19b 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -869,6 +869,12 @@ static int i915_driver_init_early(struct 
>>> drm_i915_private *dev_priv,
>>>       memcpy(device_info, match_info, sizeof(*device_info));
>>>       device_info->device_id = dev_priv->drm.pdev->device;
>>> +    BUILD_BUG_ON(sizeof(device_info->platform_mask) * BITS_PER_BYTE <
>>> +             (INTEL_MAX_PLATFORMS - 1));
>>> +    BUG_ON(device_info->platform == 0  >>> +           device_info->platform >= INTEL_MAX_PLATFORMS);
>>> +    device_info->platform_mask = BIT(device_info->platform - 1);
>>
>> Please just lose the -1, pretty please?
>>
>>> +
>>>       BUG_ON(device_info->gen > sizeof(device_info->gen_mask) * 
>>> BITS_PER_BYTE);
>>>       device_info->gen_mask = BIT(device_info->gen - 1);
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 950aa109f8cb..81211f23326a 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -861,6 +861,7 @@ struct intel_device_info {
>>>       u8 ring_mask; /* Rings supported by the HW */
>>>       enum intel_platform platform;
>>> +    u32 platform_mask;
>>>       u32 display_mmio_offset;
>>> @@ -2984,7 +2985,7 @@ intel_info(const struct drm_i915_private 
>>> *dev_priv)
>>>   #define IS_REVID(p, since, until) \
>>>       (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
>>> -#define IS_PLATFORM(dev_priv, p) ((dev_priv)->info.platform == (p))
>>> +#define IS_PLATFORM(dev_priv, p) ((dev_priv)->info.platform_mask & 
>>> BIT((p) - 1))
>>
>> What would the result be without platform_mask and just:
>>
>> #define IS_PLATFORM(dev_priv, p) (BIT((dev_priv)->info.platform) & 
>> BIT(p))
> 
> More code I'm afraid. But the problem of 32 platforms limit makes it 
> problematic for me. Because I checked, and going to u64 for the 
> platform_mask grows the code 100-200 bytes over the starting point. We'd 
> keep the single conditional advantage but I don't know, feels like not 
> worth it in that case. It could only last for a couple years before we 
> would need to go to u64.

On the other hand we could take this, and by the time would need to 
grown the mask field, we can a) enjoy the size saving, b) by that time 
new platform code will dwarf the 100-200 bytes going to u64 will add, 
and c) 100-200 bytes might even turn to a gain again, since new or-ed 
IS_platform checks will appear. Thoughts?

Regards,

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