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