public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v1 0/2] Enable legacy behaviour for Push constants
@ 2015-08-11 14:44 Arun Siluvery
  2015-08-11 14:44 ` [PATCH v1 1/2] drm/i915: Contain the WA_REG macro Arun Siluvery
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Arun Siluvery @ 2015-08-11 14:44 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala, Ben Widawsky

Patch1 fixes a simple compile error in Patch2
Patch2 fixes gpu hang observed with a subtest of gem_concurrent_blit.

Arun Siluvery (1):
  drm/i915/gen9: Disable gather at set shader bit

Mika Kuoppala (1):
  drm/i915: Contain the WA_REG macro

 drivers/gpu/drm/i915/i915_reg.h         |  5 +++++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v1 1/2] drm/i915: Contain the WA_REG macro
  2015-08-11 14:44 [PATCH v1 0/2] Enable legacy behaviour for Push constants Arun Siluvery
@ 2015-08-11 14:44 ` Arun Siluvery
  2015-08-12 15:40   ` Dave Gordon
  2015-08-11 14:44 ` [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit Arun Siluvery
  2015-08-11 20:58 ` [PATCH v1 0/2] Enable legacy behaviour for Push constants Timo Aaltonen
  2 siblings, 1 reply; 11+ messages in thread
From: Arun Siluvery @ 2015-08-11 14:44 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala, Ben Widawsky

From: Mika Kuoppala <mika.kuoppala@intel.com>

Prevent leaking the if scoping by containing the WA_REG
macro inside its own scope.

Reported-by: Arun Siluvery <arun.siluvery@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 1c14233..cf61262 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -780,11 +780,11 @@ static int wa_add(struct drm_i915_private *dev_priv,
 	return 0;
 }
 
-#define WA_REG(addr, mask, val) { \
+#define WA_REG(addr, mask, val) do { \
 		const int r = wa_add(dev_priv, (addr), (mask), (val)); \
 		if (r) \
 			return r; \
-	}
+	} while(0)
 
 #define WA_SET_BIT_MASKED(addr, mask) \
 	WA_REG(addr, (mask), _MASKED_BIT_ENABLE(mask))
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit
  2015-08-11 14:44 [PATCH v1 0/2] Enable legacy behaviour for Push constants Arun Siluvery
  2015-08-11 14:44 ` [PATCH v1 1/2] drm/i915: Contain the WA_REG macro Arun Siluvery
@ 2015-08-11 14:44 ` Arun Siluvery
  2015-08-12 15:41   ` Dave Gordon
  2015-08-14 21:17   ` shuang.he
  2015-08-11 20:58 ` [PATCH v1 0/2] Enable legacy behaviour for Push constants Timo Aaltonen
  2 siblings, 2 replies; 11+ messages in thread
From: Arun Siluvery @ 2015-08-11 14:44 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala, Ben Widawsky

From Gen9, Push constant instruction parsing behaviour varies according to
whether set shader is enabled or not. If we want legacy behaviour then it
can be achieved by disabling set shader.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959

Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         |  5 +++++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7456bd2..4d32b67 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1720,6 +1720,10 @@ enum skl_disp_power_wells {
 #define FW_BLC		0x020d8
 #define FW_BLC2		0x020dc
 #define FW_BLC_SELF	0x020e0 /* 915+ only */
+#define CS_RCS_BE       0x20D8
+#define   CS_RCS_DISABLE_GATHER_AT_SHADER    (1<<7)
+#define RS_CHICKEN      0x20DC
+#define   RS_CHICKEN_DISABLE_GATHER_AT_SHADER  (1<<2)
 #define   FW_BLC_SELF_EN_MASK      (1<<31)
 #define   FW_BLC_SELF_FIFO_MASK    (1<<16) /* 945 only */
 #define   FW_BLC_SELF_EN           (1<<15) /* 945 only */
@@ -5834,6 +5838,7 @@ enum skl_disp_power_wells {
 # define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC	((1<<10) | (1<<26))
 # define GEN9_RHWO_OPTIMIZATION_DISABLE		(1<<14)
 #define COMMON_SLICE_CHICKEN2			0x7014
+#define  GEN9_DISABLE_GATHER_SET_SHADER_SLICE   (1<<12)
 # define GEN8_CSC2_SBE_VUE_CACHE_CONSERVATIVE	(1<<0)
 
 #define HIZ_CHICKEN					0x7018
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index cf61262..7d284ed 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -983,6 +983,16 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
 		tmp |= HDC_FORCE_CSR_NON_COHERENT_OVR_DISABLE;
 	WA_SET_BIT_MASKED(HDC_CHICKEN0, tmp);
 
+	/* Chicken bits to disable set shader is in multiple places,
+	 * set bits in all required registers to disable it correctly
+	 */
+	WA_SET_BIT_MASKED(COMMON_SLICE_CHICKEN2, GEN9_DISABLE_GATHER_SET_SHADER_SLICE);
+	if ((IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_D0) ||
+	    (IS_BROXTON(dev) && INTEL_REVID(dev) == BXT_REVID_A0))
+		WA_SET_BIT_MASKED(RS_CHICKEN, RS_CHICKEN_DISABLE_GATHER_AT_SHADER);
+	else
+		WA_SET_BIT_MASKED(CS_RCS_BE, CS_RCS_DISABLE_GATHER_AT_SHADER);
+
 	return 0;
 }
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 0/2] Enable legacy behaviour for Push constants
  2015-08-11 14:44 [PATCH v1 0/2] Enable legacy behaviour for Push constants Arun Siluvery
  2015-08-11 14:44 ` [PATCH v1 1/2] drm/i915: Contain the WA_REG macro Arun Siluvery
  2015-08-11 14:44 ` [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit Arun Siluvery
@ 2015-08-11 20:58 ` Timo Aaltonen
  2015-08-12 12:22   ` Siluvery, Arun
  2 siblings, 1 reply; 11+ messages in thread
From: Timo Aaltonen @ 2015-08-11 20:58 UTC (permalink / raw)
  To: Arun Siluvery, intel-gfx; +Cc: Ben Widawsky, Mika Kuoppala

On 11.08.2015 17:44, Arun Siluvery wrote:
> Patch1 fixes a simple compile error in Patch2
> Patch2 fixes gpu hang observed with a subtest of gem_concurrent_blit.
> 
> Arun Siluvery (1):
>   drm/i915/gen9: Disable gather at set shader bit
> 
> Mika Kuoppala (1):
>   drm/i915: Contain the WA_REG macro
> 
>  drivers/gpu/drm/i915/i915_reg.h         |  5 +++++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 

prw-blt-overwrite-source-read-rcs-forked runs fine with these, tested on
SKL-Y & -H

Tested-by: Timo Aaltonen <timo.aaltonen@canonical.com>


-- 
t
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 0/2] Enable legacy behaviour for Push constants
  2015-08-11 20:58 ` [PATCH v1 0/2] Enable legacy behaviour for Push constants Timo Aaltonen
@ 2015-08-12 12:22   ` Siluvery, Arun
  0 siblings, 0 replies; 11+ messages in thread
From: Siluvery, Arun @ 2015-08-12 12:22 UTC (permalink / raw)
  To: Timo Aaltonen, intel-gfx; +Cc: Ben Widawsky, Mika Kuoppala

On 11/08/2015 21:58, Timo Aaltonen wrote:
> On 11.08.2015 17:44, Arun Siluvery wrote:
>> Patch1 fixes a simple compile error in Patch2
>> Patch2 fixes gpu hang observed with a subtest of gem_concurrent_blit.
>>
>> Arun Siluvery (1):
>>    drm/i915/gen9: Disable gather at set shader bit
>>
>> Mika Kuoppala (1):
>>    drm/i915: Contain the WA_REG macro
>>
>>   drivers/gpu/drm/i915/i915_reg.h         |  5 +++++
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++++++++--
>>   2 files changed, 17 insertions(+), 2 deletions(-)
>>
>
> prw-blt-overwrite-source-read-rcs-forked runs fine with these, tested on
> SKL-Y & -H
>
> Tested-by: Timo Aaltonen <timo.aaltonen@canonical.com>
>
>
This patch can be ignored if the following patch is applied,

[Intel-gfx] [PATCH] lib/rendercopy_gen9: Setup Push constant	pointer 
before sending BTP commands
http://lists.freedesktop.org/archives/intel-gfx/2015-August/073483.html

regards
Arun

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/2] drm/i915: Contain the WA_REG macro
  2015-08-11 14:44 ` [PATCH v1 1/2] drm/i915: Contain the WA_REG macro Arun Siluvery
@ 2015-08-12 15:40   ` Dave Gordon
  2015-08-14  8:51     ` Daniel Vetter
  0 siblings, 1 reply; 11+ messages in thread
From: Dave Gordon @ 2015-08-12 15:40 UTC (permalink / raw)
  To: Arun Siluvery, intel-gfx; +Cc: Ben Widawsky, Mika Kuoppala

On 11/08/15 15:44, Arun Siluvery wrote:
> From: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Prevent leaking the if scoping by containing the WA_REG
> macro inside its own scope.
>
> Reported-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1c14233..cf61262 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -780,11 +780,11 @@ static int wa_add(struct drm_i915_private *dev_priv,
>   	return 0;
>   }
>
> -#define WA_REG(addr, mask, val) { \
> +#define WA_REG(addr, mask, val) do { \
>   		const int r = wa_add(dev_priv, (addr), (mask), (val)); \
>   		if (r) \
>   			return r; \
> -	}
> +	} while(0)
>
>   #define WA_SET_BIT_MASKED(addr, mask) \
>   	WA_REG(addr, (mask), _MASKED_BIT_ENABLE(mask))

On the one hand, yes, this definitely needs the do-while wrapper.

OTOH, hiding a conditional 'return' inside a macro is an abomination  :( 
At least it's only local to this file ...

So, on the grounds that this makes it more correct if no less ugly:

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

.Dave.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit
  2015-08-11 14:44 ` [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit Arun Siluvery
@ 2015-08-12 15:41   ` Dave Gordon
  2015-08-12 15:55     ` Siluvery, Arun
  2015-08-14  8:54     ` Daniel Vetter
  2015-08-14 21:17   ` shuang.he
  1 sibling, 2 replies; 11+ messages in thread
From: Dave Gordon @ 2015-08-12 15:41 UTC (permalink / raw)
  To: Arun Siluvery, intel-gfx; +Cc: Ben Widawsky, Mika Kuoppala

On 11/08/15 15:44, Arun Siluvery wrote:
>  From Gen9, Push constant instruction parsing behaviour varies according to
> whether set shader is enabled or not. If we want legacy behaviour then it
> can be achieved by disabling set shader.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h         |  5 +++++
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++++
>   2 files changed, 15 insertions(+)

[snip]

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index cf61262..7d284ed 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -983,6 +983,16 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
>   		tmp |= HDC_FORCE_CSR_NON_COHERENT_OVR_DISABLE;
>   	WA_SET_BIT_MASKED(HDC_CHICKEN0, tmp);
>
> +	/* Chicken bits to disable set shader is in multiple places,
> +	 * set bits in all required registers to disable it correctly
> +	 */
> +	WA_SET_BIT_MASKED(COMMON_SLICE_CHICKEN2, GEN9_DISABLE_GATHER_SET_SHADER_SLICE);
> +	if ((IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_D0) ||
> +	    (IS_BROXTON(dev) && INTEL_REVID(dev) == BXT_REVID_A0))
> +		WA_SET_BIT_MASKED(RS_CHICKEN, RS_CHICKEN_DISABLE_GATHER_AT_SHADER);
> +	else
> +		WA_SET_BIT_MASKED(CS_RCS_BE, CS_RCS_DISABLE_GATHER_AT_SHADER);
> +
>   	return 0;
>   }

This workaround isn't tagged with a specific /* WaXyz:chip */ comment.
Also, the style isn't consistent with the other paragraphs earlier in 
this function: those have braces round the body part even when there's 
only one line of code, possibly to make it clear where the WA comment
applies (of course, this is why the buggy WA_REG() macro wasn't spotted 
earlier).

So, maybe prettify this a bit, if possible? The code actually looks 
correct, just ugly.

Oh, and keep patch 1 even if you decide to abandon this one!

.Dave.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit
  2015-08-12 15:41   ` Dave Gordon
@ 2015-08-12 15:55     ` Siluvery, Arun
  2015-08-14  8:54     ` Daniel Vetter
  1 sibling, 0 replies; 11+ messages in thread
From: Siluvery, Arun @ 2015-08-12 15:55 UTC (permalink / raw)
  To: Dave Gordon, intel-gfx; +Cc: Ben Widawsky, Mika Kuoppala

On 12/08/2015 16:41, Dave Gordon wrote:
> On 11/08/15 15:44, Arun Siluvery wrote:
>>  From Gen9, Push constant instruction parsing behaviour varies
>> according to
>> whether set shader is enabled or not. If we want legacy behaviour then it
>> can be achieved by disabling set shader.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
>>
>> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h         |  5 +++++
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++++
>>   2 files changed, 15 insertions(+)
>
> [snip]
>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index cf61262..7d284ed 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -983,6 +983,16 @@ static int gen9_init_workarounds(struct
>> intel_engine_cs *ring)
>>           tmp |= HDC_FORCE_CSR_NON_COHERENT_OVR_DISABLE;
>>       WA_SET_BIT_MASKED(HDC_CHICKEN0, tmp);
>>
>> +    /* Chicken bits to disable set shader is in multiple places,
>> +     * set bits in all required registers to disable it correctly
>> +     */
>> +    WA_SET_BIT_MASKED(COMMON_SLICE_CHICKEN2,
>> GEN9_DISABLE_GATHER_SET_SHADER_SLICE);
>> +    if ((IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_D0) ||
>> +        (IS_BROXTON(dev) && INTEL_REVID(dev) == BXT_REVID_A0))
>> +        WA_SET_BIT_MASKED(RS_CHICKEN,
>> RS_CHICKEN_DISABLE_GATHER_AT_SHADER);
>> +    else
>> +        WA_SET_BIT_MASKED(CS_RCS_BE, CS_RCS_DISABLE_GATHER_AT_SHADER);
>> +
>>       return 0;
>>   }
>
> This workaround isn't tagged with a specific /* WaXyz:chip */ comment.
> Also, the style isn't consistent with the other paragraphs earlier in
> this function: those have braces round the body part even when there's
> only one line of code, possibly to make it clear where the WA comment
> applies (of course, this is why the buggy WA_REG() macro wasn't spotted
> earlier).
>
> So, maybe prettify this a bit, if possible? The code actually looks
> correct, just ugly.
>
> Oh, and keep patch 1 even if you decide to abandon this one!
>

Hi Dave,

This patch can be ignored if we use below patch,
[Intel-gfx] [PATCH] lib/rendercopy_gen9: Setup Push constant    pointer 
before sending BTP commands
http://lists.freedesktop.org/archives/intel-gfx/2015-August/073483.html

I think the correct option would be to ignore this patch.

regards
Arun

> .Dave.
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/2] drm/i915: Contain the WA_REG macro
  2015-08-12 15:40   ` Dave Gordon
@ 2015-08-14  8:51     ` Daniel Vetter
  0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2015-08-14  8:51 UTC (permalink / raw)
  To: Dave Gordon; +Cc: Mika Kuoppala, intel-gfx, Ben Widawsky

On Wed, Aug 12, 2015 at 04:40:03PM +0100, Dave Gordon wrote:
> On 11/08/15 15:44, Arun Siluvery wrote:
> >From: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> >Prevent leaking the if scoping by containing the WA_REG
> >macro inside its own scope.
> >
> >Reported-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> >Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >index 1c14233..cf61262 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >@@ -780,11 +780,11 @@ static int wa_add(struct drm_i915_private *dev_priv,
> >  	return 0;
> >  }
> >
> >-#define WA_REG(addr, mask, val) { \
> >+#define WA_REG(addr, mask, val) do { \
> >  		const int r = wa_add(dev_priv, (addr), (mask), (val)); \
> >  		if (r) \
> >  			return r; \
> >-	}
> >+	} while(0)
> >
> >  #define WA_SET_BIT_MASKED(addr, mask) \
> >  	WA_REG(addr, (mask), _MASKED_BIT_ENABLE(mask))
> 
> On the one hand, yes, this definitely needs the do-while wrapper.
> 
> OTOH, hiding a conditional 'return' inside a macro is an abomination  :( At
> least it's only local to this file ...
> 
> So, on the grounds that this makes it more correct if no less ugly:
> 
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

Queued for -next, thanks for the patch. And I fixed the checkpatch ERROR,
tsk ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit
  2015-08-12 15:41   ` Dave Gordon
  2015-08-12 15:55     ` Siluvery, Arun
@ 2015-08-14  8:54     ` Daniel Vetter
  1 sibling, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2015-08-14  8:54 UTC (permalink / raw)
  To: Dave Gordon; +Cc: Mika Kuoppala, intel-gfx, Ben Widawsky

On Wed, Aug 12, 2015 at 04:41:13PM +0100, Dave Gordon wrote:
> On 11/08/15 15:44, Arun Siluvery wrote:
> > From Gen9, Push constant instruction parsing behaviour varies according to
> >whether set shader is enabled or not. If we want legacy behaviour then it
> >can be achieved by disabling set shader.
> >
> >Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
> >
> >Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_reg.h         |  5 +++++
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++++
> >  2 files changed, 15 insertions(+)
> 
> [snip]
> 
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >index cf61262..7d284ed 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >@@ -983,6 +983,16 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
> >  		tmp |= HDC_FORCE_CSR_NON_COHERENT_OVR_DISABLE;
> >  	WA_SET_BIT_MASKED(HDC_CHICKEN0, tmp);
> >
> >+	/* Chicken bits to disable set shader is in multiple places,
> >+	 * set bits in all required registers to disable it correctly
> >+	 */
> >+	WA_SET_BIT_MASKED(COMMON_SLICE_CHICKEN2, GEN9_DISABLE_GATHER_SET_SHADER_SLICE);
> >+	if ((IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_D0) ||
> >+	    (IS_BROXTON(dev) && INTEL_REVID(dev) == BXT_REVID_A0))
> >+		WA_SET_BIT_MASKED(RS_CHICKEN, RS_CHICKEN_DISABLE_GATHER_AT_SHADER);
> >+	else
> >+		WA_SET_BIT_MASKED(CS_RCS_BE, CS_RCS_DISABLE_GATHER_AT_SHADER);
> >+
> >  	return 0;
> >  }
> 
> This workaround isn't tagged with a specific /* WaXyz:chip */ comment.
> Also, the style isn't consistent with the other paragraphs earlier in this
> function: those have braces round the body part even when there's only one
> line of code, possibly to make it clear where the WA comment
> applies (of course, this is why the buggy WA_REG() macro wasn't spotted
> earlier).

Imo if we want to bikeshed this is should be

	/*
	 * WaBlaFoo:pft
	 * extended comment if need
	 */
	if (IS_FOO(dev))
		WA_SET_();

i.e. follow the style in this patch here for all of them.
-Daniel
> 
> So, maybe prettify this a bit, if possible? The code actually looks correct,
> just ugly.
> 
> Oh, and keep patch 1 even if you decide to abandon this one!
> 
> .Dave.
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit
  2015-08-11 14:44 ` [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit Arun Siluvery
  2015-08-12 15:41   ` Dave Gordon
@ 2015-08-14 21:17   ` shuang.he
  1 sibling, 0 replies; 11+ messages in thread
From: shuang.he @ 2015-08-14 21:17 UTC (permalink / raw)
  To: shuang.he, julianx.dumez, christophe.sureau, lei.a.liu, intel-gfx,
	arun.siluvery

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7150
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                 -2              283/283              281/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-08-14 21:17 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-11 14:44 [PATCH v1 0/2] Enable legacy behaviour for Push constants Arun Siluvery
2015-08-11 14:44 ` [PATCH v1 1/2] drm/i915: Contain the WA_REG macro Arun Siluvery
2015-08-12 15:40   ` Dave Gordon
2015-08-14  8:51     ` Daniel Vetter
2015-08-11 14:44 ` [PATCH v1 2/2] drm/i915/gen9: Disable gather at set shader bit Arun Siluvery
2015-08-12 15:41   ` Dave Gordon
2015-08-12 15:55     ` Siluvery, Arun
2015-08-14  8:54     ` Daniel Vetter
2015-08-14 21:17   ` shuang.he
2015-08-11 20:58 ` [PATCH v1 0/2] Enable legacy behaviour for Push constants Timo Aaltonen
2015-08-12 12:22   ` Siluvery, Arun

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox