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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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?

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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx