[Intel-gfx] [PATCH] drm/i915: Introduce concept of a sub-platform
Lucas De Marchi
lucas.demarchi at intel.com
Mon Mar 18 18:53:12 UTC 2019
On Mon, Mar 18, 2019 at 07:09:59AM +0000, Tvrtko Ursulin wrote:
>
>On 15/03/2019 18:40, Lucas De Marchi wrote:
>>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.
>
>Confusing how and what is the second u32? You mean when array gets
because you are printing here only one u32 and hence this number could
be more than one platform.
>expanded in the future? Here the actual idea (see next version) is to
>only print the subplatform bits.
yep, much better there.
>
>>>
>>>>
>>>>> 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.
>
>I hope you meant gut feeling. Anyways, I said I wasn't 100% sure of
yep, of course :-/
>the details, right? Having exercised my memory lanes I think the
>biggest negative effect of the move to u64 was in conjunction with
>this subplatform idea. This is due the "if (mask & platform) && (mask
>& subplatform)" expanding to 2 u64 constants per check. I have a
>remedy for that, but lets not get bogged down and code size which is
>not the main consideration here. Idea was to consolidate and simplify
>whilst preserving the two beneficial effects which I already explain.
>
>>>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.
>
>Which is why I hinted the u32 array approach does not suffer on
>either. No increase on 64-bit, no real increase on 32-bit.
I actually think the main advantage of multiple dwords is to be ready
for the next bump when it arrives. So ok. I still don't like open coded
bitmask though.
Lucas De Marchi
>
>Regards,
>
>Tvrtko
>
>>
>>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
>>
More information about the Intel-gfx
mailing list