[Intel-gfx] [PATCH] drm/i915: Introduce concept of a sub-platform

Fri Mar 15 18:40:50 UTC 2019

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 at intel.com>
>>>
>>>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 at intel.com>
>>>Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>Cc: Jani Nikula <jani.nikula at intel.com>
>>>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>>>Cc: Jose Souza <jose.souza at intel.com>
>>>---
>>>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