Re: [PATCH] drm/i915: Use bitmask for IS_REVID checking

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

On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> Hi,
>
> On 10/05/16 14:33, Jani Nikula wrote:
>> On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID
>>> result in a maximum of one conditional jump instruction (was
>>> three before) and overall reduction in code size.
>>
>> IMO we really shouldn't bother with this. The end goal is that we remove
>> all of these when we decide we no longer care about early
>> (non-production) revisions for a platform. For Skylake I'd argue we are
>> beyond this point.
>
> Thought that simply removing everything guarded by IS_SKL_REVID might be 
> possible never crossed my mind. :) Which one is the first production 
> stepping then?

None of the ones we list and check for i.e. we should replace all of the
IS_SKL_REVID() uses with either 0 or IS_SKYLAKE(), and simplify from
there.

Maybe we should check the revid once on driver load, and whine about
early hardware. We've had such checks in the past on earlier
platforms. This might save our precious time, as we shouldn't be
debugging issues reported against pre-production hardware.

Ville here says dropping the checks is a good time to verify the
workarounds we do have in place are correct going forward.


BR,
Jani.



>
> Regards,
>
> Tvrtko.
>
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_dma.c | 11 +++++++++++
>>>   drivers/gpu/drm/i915/i915_drv.h | 24 +++++++++++++++++++-----
>>>   2 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>> index 46ac1da64a09..1ae812436827 100644
>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>> @@ -1072,6 +1072,17 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>>>   	memcpy(device_info, info, sizeof(dev_priv->info));
>>>   	device_info->device_id = dev->pdev->device;
>>>
>>> +	/* For unknown revisions mask stays at zero which is correct. */
>>> +	if (dev->pdev->revision <
>>> +	    sizeof(device_info->revision_mask) * BITS_PER_BYTE)
>>> +		device_info->revision_mask = BIT(dev->pdev->revision);
>>> +
>>> +	if (IS_SKYLAKE(dev_priv))
>>> +		device_info->skl_rev_mask = ~0;
>>> +
>>> +	if (IS_BROXTON(dev_priv))
>>> +		device_info->bxt_rev_mask = ~0;
>>> +
>>>   	spin_lock_init(&dev_priv->irq_lock);
>>>   	spin_lock_init(&dev_priv->gpu_error.lock);
>>>   	mutex_init(&dev_priv->backlight_lock);
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 26e7de415966..fbeb68e9ec96 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -757,6 +757,9 @@ struct intel_csr {
>>>   struct intel_device_info {
>>>   	u32 display_mmio_offset;
>>>   	u16 device_id;
>>> +	u8 revision_mask;
>>> +	u8 skl_rev_mask;
>>> +	u8 bxt_rev_mask;
>>>   	u8 num_pipes:3;
>>>   	u8 num_sprites[I915_MAX_PIPES];
>>>   	u8 gen;
>>> @@ -2519,14 +2522,23 @@ struct drm_i915_cmd_table {
>>>   #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
>>>   #define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
>>>
>>> -#define REVID_FOREVER		0xff
>>> +#define REVID_FOREVER		0
>>> +
>>>   /*
>>>    * Return true if revision is in range [since,until] inclusive.
>>>    *
>>>    * Use 0 for open-ended since, and REVID_FOREVER for open-ended until.
>>>    */
>>> -#define IS_REVID(p, since, until) \
>>> -	(INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
>>> +#define IS_REVID(p, mask, since, until) ({\
>>> +	unsigned int __s = (since), __e = (until); \
>>> +	BUILD_BUG_ON(!__builtin_constant_p(since)); \
>>> +	BUILD_BUG_ON(!__builtin_constant_p(until)); \
>>> +	BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
>>> +	BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \
>>> +	if ((__e) == REVID_FOREVER) \
>>> +		__e = BITS_PER_LONG - 1; \
>>> +	!!(INTEL_INFO(p)->revision_mask & (mask) & GENMASK((__e), (__s))); \
>>> +})
>>>
>>>   #define IS_I830(dev)		(INTEL_DEVID(dev) == 0x3577)
>>>   #define IS_845G(dev)		(INTEL_DEVID(dev) == 0x2562)
>>> @@ -2605,14 +2617,16 @@ struct drm_i915_cmd_table {
>>>   #define SKL_REVID_E0		0x4
>>>   #define SKL_REVID_F0		0x5
>>>
>>> -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until))
>>> +#define IS_SKL_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->skl_rev_mask, \
>>> +				               since, until)
>>>
>>>   #define BXT_REVID_A0		0x0
>>>   #define BXT_REVID_A1		0x1
>>>   #define BXT_REVID_B0		0x3
>>>   #define BXT_REVID_C0		0x9
>>>
>>> -#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until))
>>> +#define IS_BXT_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->bxt_rev_mask, \
>>> +				               since, until)
>>>
>>>   /*
>>>    * The genX designation typically refers to the render engine, so render
>>

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