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@xxxxxxxxx>
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@xxxxxxxxx>
Suggested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Cc: Jose Souza <jose.souza@xxxxxxxxx>
---
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.
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.
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.
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
