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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux