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

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 29 Mar 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 29/03/2019 09:54, Jani Nikula wrote:
>> On Wed, 27 Mar 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@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.
>>>
>>> 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.
>>>
>>> v5:
>>>   * Fix subplatform check.
>>>   * Protect against forgetting to expand subplatform bits.
>>>   * Remove platform enum tallying.
>>>   * Add subplatform to error state. (Chris)
>>>   * Drop macros and just use static inlines.
>>>   * Remove redundant IRONLAKE_M. (Ville)
>>>
>>> v6:
>>>   * Split out Ironlake change.
>>>   * Optimize subplatform check.
>>>   * Use __always_inline. (Lucas)
>>>   * Add platform_mask comment. (Paulo)
>>>   * Pass stored runtime info in error capture. (Chris)
>>>
>>> v7:
>>>   * Rebased for new AML ULX device id.
>>>   * Bump platform mask array size for EHL.
>>>   * Stop mentioning device ids in intel_device_subplatform_init by using
>>>     the trick of splitting macros i915_pciids.h. (Jani)
>>>   * AML seems to be either a subplatform of KBL or CFL so express it like
>>>     that.
>>>
>>> v8:
>>>   * Use one device id table per subplatform. (Jani)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Cc: Jose Souza <jose.souza@intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v6
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.c          |   8 +-
>>>   drivers/gpu/drm/i915/i915_drv.h          | 123 ++++++++++++++++-------
>>>   drivers/gpu/drm/i915/i915_gpu_error.c    |   3 +
>>>   drivers/gpu/drm/i915/i915_pci.c          |   2 +-
>>>   drivers/gpu/drm/i915/intel_device_info.c |  93 +++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_device_info.h |  27 ++++-
>>>   6 files changed, 214 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>> index f1334f5d4ead..74734d7661e5 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -868,6 +868,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);
>>> @@ -1718,10 +1720,12 @@ 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 (subplatform=0x%x) gen=%i\n",
>>>   			   INTEL_DEVID(dev_priv),
>>>   			   INTEL_REVID(dev_priv),
>>>   			   intel_platform_name(INTEL_INFO(dev_priv)->platform),
>>> +			   intel_subplatform(RUNTIME_INFO(dev_priv),
>>> +					     INTEL_INFO(dev_priv)->platform),
>>>   			   INTEL_GEN(dev_priv));
>>>   
>>>   		intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>>> @@ -1764,8 +1768,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 9d3cab9406e1..b7d3f3a45ed9 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2298,7 +2298,67 @@ 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))
>>> +static __always_inline unsigned int
>>> +__platform_mask_index(const struct intel_runtime_info *info,
>>> +		      enum intel_platform p)
>>> +{
>>> +	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;
>>> +}
>>> +
>>> +static __always_inline unsigned int
>>> +__platform_mask_bit(const struct intel_runtime_info *info,
>>> +		    enum intel_platform p)
>>> +{
>>> +	const unsigned int pbits =
>>> +		BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS;
>>> +
>>> +	return p % pbits + INTEL_SUBPLATFORM_BITS;
>>> +}
>>> +
>>> +static inline u32
>>> +intel_subplatform(const struct intel_runtime_info *info, enum intel_platform p)
>>> +{
>>> +	const unsigned int pi = __platform_mask_index(info, p);
>>> +
>>> +	return info->platform_mask[pi] & INTEL_SUBPLATFORM_BITS;
>>> +}
>>> +
>>> +static __always_inline bool
>>> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
>>> +{
>>> +	const struct intel_runtime_info *info = RUNTIME_INFO(i915);
>>> +	const unsigned int pi = __platform_mask_index(info, p);
>>> +	const unsigned int pb = __platform_mask_bit(info, p);
>>> +
>>> +	BUILD_BUG_ON(!__builtin_constant_p(p));
>>> +
>>> +	return info->platform_mask[pi] & BIT(pb);
>>> +}
>>> +
>>> +static __always_inline bool
>>> +IS_SUBPLATFORM(const struct drm_i915_private *i915,
>>> +	       enum intel_platform p, unsigned int s)
>>> +{
>>> +	const struct intel_runtime_info *info = RUNTIME_INFO(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];
>>> +
>>> +	BUILD_BUG_ON(!__builtin_constant_p(p));
>>> +	BUILD_BUG_ON(!__builtin_constant_p(s));
>>> +	BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS);
>>> +
>>> +	/* Shift and test on the MSB position so sign flag can be used. */
>>> +	return ((mask << (msb - pb)) & (mask << (msb - s))) & BIT(msb);
>>> +}
>> 
>> Hum, I wonder if the __builtin_constant_p()'s in an inline function are
>> going to be a problem for clang.
>
> No idea.. has something been happening along these lines in the past?

The thread and two patches starting from [1] may be related.

[1] http://mid.mail-archive.com/20181016122938.18757-1-jani.nikula@intel.com

> It could be a macro but then all WARN_ON's which use IS_PLATFORM expand 
> to most unreadable mess.

I know.

>>> +static bool find_devid(u16 id, const u16 *p, unsigned int num)
>>> +{
>>> +	for (; num; num--, p++) {
>>> +		if (*p == id)
>>> +			return true;
>>> +	}
>> 
>> Why such a convoluted way of doing what's supposed to be a simple thing?
>> I had to stop at that and wonder what's going on. While this would've
>> been obvious and reviewed with a 2-second glance:
>> 
>> 	int i;
>> 
>>          for (i = 0; i < num; i++)
>>          	if (id == p[i])
>>                  	return true;
>> 
>> The alternative is zero-terminating the arrays:
>> 
>> 	for (; *p; p++)
>>          	if (id == *p)
>>                  	return true;
>> 
>
> I think mine is not that complicated. It's a standard countdown pattern, 
> no? Why add locals or null termination if not needed.

I just like to simplify the code for the humans, not for the compiler.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx