Re: [PATCH] drm/i915: Add means to apply WA conditionally

["Siluvery, Arun" <arun.siluvery@linux.intel.com>]

On 23/10/2014 16:51, Daniel Vetter wrote:
> On Thu, Oct 23, 2014 at 04:29:30PM +0100, Arun Siluvery wrote:
>> We would want to apply some of the workarounds based on a condition to a
>> particular platform or Gen but we may not know all possible controlling
>> parameters in advance hence allow to define open conditions; a WA makes
>> it to the list only if the condition is true.
>>
>> With the appropriate conditions we can combine all of the workarounds
>> and apply them from a single place irrespective of platform instead of
>> having them in separate functions.
>>
>> For: VIZ-4090
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>
> Imo we should just pull the condition out into proper control flow. Hiding
> it like that in the macro doesn't seem to buy us anything at all, but
> obfuscates the code.

No we are not hiding the condition, I thought it would be easier to read 
it this way, e.g.,

WA_SET_BIT_MASKED_IF(IS_BDW_GT3(dev), WA_REG, WA_MASK);

do you prefer adding if(cond) to each WA?

regards
Arun

> -Daniel
>
>> ---
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 35 +++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 497b836..0525a5d 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -736,6 +736,41 @@ static int wa_add(struct drm_i915_private *dev_priv,
>>
>>   #define WA_WRITE(addr, val) WA_REG(addr, val, 0xffffffff)
>>
>> +#define WA_SET_BIT_MASKED_IF(cond, addr, mask)		\
>> +	do {						\
>> +		if (cond) {				\
>> +			WA_SET_BIT_MASKED(addr, mask);	\
>> +		}					\
>> +	} while(0)
>> +
>> +#define WA_CLR_BIT_MASKED_IF(cond, addr, mask)		\
>> +	do {						\
>> +		if (cond) {				\
>> +			WA_CLR_BIT_MASKED(addr, mask);	\
>> +		}					\
>> +	} while(0)
>> +
>> +#define WA_SET_BIT_IF(cond, addr, mask)			\
>> +	do {						\
>> +		if (cond) {				\
>> +			WA_SET_BIT(addr, mask);		\
>> +		}					\
>> +	} while(0)
>> +
>> +#define WA_CLR_BIT_IF(cond, addr, mask)			\
>> +	do {						\
>> +		if (cond) {				\
>> +			WA_CLR_BIT(addr, mask);		\
>> +		}					\
>> +	} while(0)
>> +
>> +#define WA_WRITE_IF(cond, addr, val)			\
>> +	do {						\
>> +		if (cond) {				\
>> +			WA_WRITE(addr, val);		\
>> +		}					\
>> +	} while(0)
>> +
>>   static int bdw_init_workarounds(struct intel_engine_cs *ring)
>>   {
>>   	struct drm_device *dev = ring->dev;
>> --
>> 2.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>