All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Add means to apply WA conditionally
@ 2014-10-23 15:29 Arun Siluvery
  2014-10-23 15:51 ` Daniel Vetter
  0 siblings, 1 reply; 4+ messages in thread
From: Arun Siluvery @ 2014-10-23 15:29 UTC (permalink / raw)
  To: intel-gfx

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>
---
 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

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Add means to apply WA conditionally
  2014-10-23 15:29 [PATCH] drm/i915: Add means to apply WA conditionally Arun Siluvery
@ 2014-10-23 15:51 ` Daniel Vetter
  2014-10-23 16:07   ` Siluvery, Arun
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2014-10-23 15:51 UTC (permalink / raw)
  To: Arun Siluvery; +Cc: intel-gfx

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.
-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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Add means to apply WA conditionally
  2014-10-23 15:51 ` Daniel Vetter
@ 2014-10-23 16:07   ` Siluvery, Arun
  2014-10-23 19:22     ` Daniel Vetter
  0 siblings, 1 reply; 4+ messages in thread
From: Siluvery, Arun @ 2014-10-23 16:07 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

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
>

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Add means to apply WA conditionally
  2014-10-23 16:07   ` Siluvery, Arun
@ 2014-10-23 19:22     ` Daniel Vetter
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2014-10-23 19:22 UTC (permalink / raw)
  To: Siluvery, Arun; +Cc: intel-gfx

On Thu, Oct 23, 2014 at 6:07 PM, Siluvery, Arun
<arun.siluvery@linux.intel.com> wrote:
> 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?

Yes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-23 19:22 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-23 15:29 [PATCH] drm/i915: Add means to apply WA conditionally Arun Siluvery
2014-10-23 15:51 ` Daniel Vetter
2014-10-23 16:07   ` Siluvery, Arun
2014-10-23 19:22     ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.