[Intel-gfx] [PATCH v2] drm/i915: use unsigned long for platform_mask
Lucas De Marchi
lucas.demarchi at intel.com
Wed Apr 3 19:45:11 UTC 2019
On Wed, Apr 03, 2019 at 10:25:33AM +0100, Tvrtko Ursulin wrote:
>
>On 03/04/2019 09:15, Lucas De Marchi wrote:
>>On Tue, Apr 2, 2019 at 11:58 PM Tvrtko Ursulin
>><tvrtko.ursulin at 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.
I tried some options, to reproduce it again, but I couldn't. Interesting
that I remember running pahole on the result .ko and getting the result
I was expecting.
I thought about looking into my .bash_history, but it already rotated.
>
>>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?
afaik, no. Because the struct is already not defined at this point. So
the usual trick of getting the size without an insntance doesn't work.
I.e. sizeof(((struct bla *)0)->member[0]) works outside of bla, but not
while we are defining it.
Well.. it can also happen of someone changing the type here and
forgetting to change "mask" in other function (see v1). I think since
they are in the same line it's easy to spot the mistake. But no strong
opinion. We already have 64 - 6 bits of platform bits that's probably
sufficient for a long time.
Lucas De Marchi
>
>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 at 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 at lists.freedesktop.org
>>>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
More information about the Intel-gfx
mailing list