public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
@ 2014-12-06 20:14 Damien Lespiau
  2014-12-07  4:48 ` shuang.he
  2014-12-08 12:33 ` Jani Nikula
  0 siblings, 2 replies; 20+ messages in thread
From: Damien Lespiau @ 2014-12-06 20:14 UTC (permalink / raw)
  To: intel-gfx

I was playing with clang and oh surprise! a warning trigerred by
-Wshift-overflow (gcc doesn't have this one):

    WA_SET_BIT_MASKED(GEN7_GT_MODE,
                      GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);

    drivers/gpu/drm/i915/intel_ringbuffer.c:786:2: warning: signed shift result
      (0x28002000000) requires 43 bits to represent, but 'int' only has 32 bits
      [-Wshift-overflow]
        WA_SET_BIT_MASKED(GEN7_GT_MODE,
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/gpu/drm/i915/intel_ringbuffer.c:737:15: note: expanded from macro
      'WA_SET_BIT_MASKED'
        WA_REG(addr, _MASKED_BIT_ENABLE(mask), (mask) & 0xffff)

Turned out GEN6_WIZ_HASHING_MASK was already shifted by 16, and we were
trying to shift it a bit more.

The other thing is that it's not the usual case of setting WA bits here, we
need to have separate mask and value.

To fix this, I've introduced a new _MASKED_FIELD() macro that takes both the
(unshifted) mask and the desired value and the rest of the patch ripples
through from it.

This bug was introduced when reworking the WA emission in:

  Commit 7225342ab501befdb64bcec76ded41f5897c0855
  Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
  Date:   Tue Oct 7 17:21:26 2014 +0300

      drm/i915: Build workaround list in ring initialization

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 3 ++-
 drivers/gpu/drm/i915/intel_pm.c         | 6 +++---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++--
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index dc03fac..6c64d61 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -36,6 +36,7 @@
 
 #define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
 #define _MASKED_BIT_DISABLE(a) ((a) << 16)
+#define _MASKED_FIELD(value, mask) (((mask) << 16) | (value))
 
 /* PCI config space */
 
@@ -1284,7 +1285,7 @@ enum punit_power_well {
 #define   GEN6_WIZ_HASHING_8x8				GEN6_WIZ_HASHING(0, 0)
 #define   GEN6_WIZ_HASHING_8x4				GEN6_WIZ_HASHING(0, 1)
 #define   GEN6_WIZ_HASHING_16x4				GEN6_WIZ_HASHING(1, 0)
-#define   GEN6_WIZ_HASHING_MASK				(GEN6_WIZ_HASHING(1, 1) << 16)
+#define   GEN6_WIZ_HASHING_MASK				GEN6_WIZ_HASHING(1, 1)
 #define   GEN6_TD_FOUR_ROW_DISPATCH_DISABLE		(1 << 5)
 
 #define GFX_MODE	0x02520
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 78911e2..209751b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6389,7 +6389,7 @@ static void gen6_init_clock_gating(struct drm_device *dev)
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
 	I915_WRITE(GEN6_GT_MODE,
-		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+		   _MASKED_FIELD(GEN6_WIZ_HASHING_16x4, GEN6_WIZ_HASHING_MASK));
 
 	ilk_init_lp_watermarks(dev);
 
@@ -6587,7 +6587,7 @@ static void haswell_init_clock_gating(struct drm_device *dev)
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
 	I915_WRITE(GEN7_GT_MODE,
-		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+		   _MASKED_FIELD(GEN6_WIZ_HASHING_16x4, GEN6_WIZ_HASHING_MASK));
 
 	/* WaSwitchSolVfFArbitrationPriority:hsw */
 	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
@@ -6684,7 +6684,7 @@ static void ivybridge_init_clock_gating(struct drm_device *dev)
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
 	I915_WRITE(GEN7_GT_MODE,
-		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+		   _MASKED_FIELD(GEN6_WIZ_HASHING_16x4, GEN6_WIZ_HASHING_MASK));
 
 	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
 	snpcr &= ~GEN6_MBC_SNPCR_MASK;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 79b4ca5..40cefef 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -739,6 +739,9 @@ static int wa_add(struct drm_i915_private *dev_priv,
 #define WA_CLR_BIT_MASKED(addr, mask) \
 	WA_REG(addr, _MASKED_BIT_DISABLE(mask), (mask) & 0xffff)
 
+#define WA_SET_FIELD_MASKED(addr, value, mask) \
+	WA_REG(addr, _MASKED_FIELD(value, mask), mask)
+
 #define WA_SET_BIT(addr, mask) WA_REG(addr, I915_READ(addr) | (mask), mask)
 #define WA_CLR_BIT(addr, mask) WA_REG(addr, I915_READ(addr) & ~(mask), mask)
 
@@ -783,8 +786,9 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	 * disable bit, which we don't touch here, but it's good
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
-	WA_SET_BIT_MASKED(GEN7_GT_MODE,
-			  GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+	WA_SET_FIELD_MASKED(GEN7_GT_MODE,
+			    GEN6_WIZ_HASHING_16x4,
+			    GEN6_WIZ_HASHING_MASK);
 
 	return 0;
 }
-- 
1.8.3.1

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-06 20:14 [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode Damien Lespiau
@ 2014-12-07  4:48 ` shuang.he
  2014-12-08 12:33 ` Jani Nikula
  1 sibling, 0 replies; 20+ messages in thread
From: shuang.he @ 2014-12-07  4:48 UTC (permalink / raw)
  To: shuang.he, intel-gfx, damien.lespiau

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1-1              364/366              364/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_flip_plain-flip-fb-recreate-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(1, M26)PASS(1, M26)      PASS(1, M26)
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] 20+ messages in thread
* Re: [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-06 20:14 [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode Damien Lespiau
  2014-12-07  4:48 ` shuang.he
@ 2014-12-08 12:33 ` Jani Nikula
  2014-12-08 13:59   ` Damien Lespiau
  2014-12-08 16:22   ` [PATCH v2] " Damien Lespiau
  1 sibling, 2 replies; 20+ messages in thread
From: Jani Nikula @ 2014-12-08 12:33 UTC (permalink / raw)
  To: Damien Lespiau, intel-gfx

On Sat, 06 Dec 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> I was playing with clang and oh surprise! a warning trigerred by
> -Wshift-overflow (gcc doesn't have this one):
>
>     WA_SET_BIT_MASKED(GEN7_GT_MODE,
>                       GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
>
>     drivers/gpu/drm/i915/intel_ringbuffer.c:786:2: warning: signed shift result
>       (0x28002000000) requires 43 bits to represent, but 'int' only has 32 bits
>       [-Wshift-overflow]
>         WA_SET_BIT_MASKED(GEN7_GT_MODE,
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/gpu/drm/i915/intel_ringbuffer.c:737:15: note: expanded from macro
>       'WA_SET_BIT_MASKED'
>         WA_REG(addr, _MASKED_BIT_ENABLE(mask), (mask) & 0xffff)
>
> Turned out GEN6_WIZ_HASHING_MASK was already shifted by 16, and we were
> trying to shift it a bit more.
>
> The other thing is that it's not the usual case of setting WA bits here, we
> need to have separate mask and value.
>
> To fix this, I've introduced a new _MASKED_FIELD() macro that takes both the
> (unshifted) mask and the desired value and the rest of the patch ripples
> through from it.
>
> This bug was introduced when reworking the WA emission in:
>
>   Commit 7225342ab501befdb64bcec76ded41f5897c0855
>   Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>   Date:   Tue Oct 7 17:21:26 2014 +0300
>
>       drm/i915: Build workaround list in ring initialization
>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Pushed to drm-intel-next-fixes, thanks for the patch.

> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 3 ++-
>  drivers/gpu/drm/i915/intel_pm.c         | 6 +++---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++--
>  3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dc03fac..6c64d61 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -36,6 +36,7 @@
>  
>  #define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
>  #define _MASKED_BIT_DISABLE(a) ((a) << 16)
> +#define _MASKED_FIELD(value, mask) (((mask) << 16) | (value))

Obligatory bikeshed, wouldn't you say _MASKED_BIT_{ENABLE,DISABLE} are
special cases of _MASKED_FIELD...? ;)

BR,
Jani.

>  
>  /* PCI config space */
>  
> @@ -1284,7 +1285,7 @@ enum punit_power_well {
>  #define   GEN6_WIZ_HASHING_8x8				GEN6_WIZ_HASHING(0, 0)
>  #define   GEN6_WIZ_HASHING_8x4				GEN6_WIZ_HASHING(0, 1)
>  #define   GEN6_WIZ_HASHING_16x4				GEN6_WIZ_HASHING(1, 0)
> -#define   GEN6_WIZ_HASHING_MASK				(GEN6_WIZ_HASHING(1, 1) << 16)
> +#define   GEN6_WIZ_HASHING_MASK				GEN6_WIZ_HASHING(1, 1)
>  #define   GEN6_TD_FOUR_ROW_DISPATCH_DISABLE		(1 << 5)
>  
>  #define GFX_MODE	0x02520
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 78911e2..209751b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6389,7 +6389,7 @@ static void gen6_init_clock_gating(struct drm_device *dev)
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
>  	I915_WRITE(GEN6_GT_MODE,
> -		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +		   _MASKED_FIELD(GEN6_WIZ_HASHING_16x4, GEN6_WIZ_HASHING_MASK));
>  
>  	ilk_init_lp_watermarks(dev);
>  
> @@ -6587,7 +6587,7 @@ static void haswell_init_clock_gating(struct drm_device *dev)
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
>  	I915_WRITE(GEN7_GT_MODE,
> -		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +		   _MASKED_FIELD(GEN6_WIZ_HASHING_16x4, GEN6_WIZ_HASHING_MASK));
>  
>  	/* WaSwitchSolVfFArbitrationPriority:hsw */
>  	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
> @@ -6684,7 +6684,7 @@ static void ivybridge_init_clock_gating(struct drm_device *dev)
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
>  	I915_WRITE(GEN7_GT_MODE,
> -		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +		   _MASKED_FIELD(GEN6_WIZ_HASHING_16x4, GEN6_WIZ_HASHING_MASK));
>  
>  	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
>  	snpcr &= ~GEN6_MBC_SNPCR_MASK;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 79b4ca5..40cefef 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -739,6 +739,9 @@ static int wa_add(struct drm_i915_private *dev_priv,
>  #define WA_CLR_BIT_MASKED(addr, mask) \
>  	WA_REG(addr, _MASKED_BIT_DISABLE(mask), (mask) & 0xffff)
>  
> +#define WA_SET_FIELD_MASKED(addr, value, mask) \
> +	WA_REG(addr, _MASKED_FIELD(value, mask), mask)
> +
>  #define WA_SET_BIT(addr, mask) WA_REG(addr, I915_READ(addr) | (mask), mask)
>  #define WA_CLR_BIT(addr, mask) WA_REG(addr, I915_READ(addr) & ~(mask), mask)
>  
> @@ -783,8 +786,9 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	 * disable bit, which we don't touch here, but it's good
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
> -	WA_SET_BIT_MASKED(GEN7_GT_MODE,
> -			  GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +	WA_SET_FIELD_MASKED(GEN7_GT_MODE,
> +			    GEN6_WIZ_HASHING_16x4,
> +			    GEN6_WIZ_HASHING_MASK);
>  
>  	return 0;
>  }
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 12:33 ` Jani Nikula
@ 2014-12-08 13:59   ` Damien Lespiau
  2014-12-08 14:17     ` Dave Gordon
  2014-12-08 14:21     ` Daniel Vetter
  2014-12-08 16:22   ` [PATCH v2] " Damien Lespiau
  1 sibling, 2 replies; 20+ messages in thread
From: Damien Lespiau @ 2014-12-08 13:59 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote:
> >  #define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
> >  #define _MASKED_BIT_DISABLE(a) ((a) << 16)
> > +#define _MASKED_FIELD(value, mask) (((mask) << 16) | (value))
> 
> Obligatory bikeshed, wouldn't you say _MASKED_BIT_{ENABLE,DISABLE} are
> special cases of _MASKED_FIELD...? ;)

That's because we're not just enabling or disabling bits here but
setting a multi-bits value.

  _MASKED_FIELD(2 << 4, 0x3 << 4);

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 13:59   ` Damien Lespiau
@ 2014-12-08 14:17     ` Dave Gordon
  2014-12-08 14:36       ` Damien Lespiau
  2014-12-08 14:21     ` Daniel Vetter
  1 sibling, 1 reply; 20+ messages in thread
From: Dave Gordon @ 2014-12-08 14:17 UTC (permalink / raw)
  To: Damien Lespiau, Jani Nikula; +Cc: intel-gfx

On 08/12/14 13:59, Damien Lespiau wrote:
> On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote:
>>>  #define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
>>>  #define _MASKED_BIT_DISABLE(a) ((a) << 16)
>>> +#define _MASKED_FIELD(value, mask) (((mask) << 16) | (value))
>>
>> Obligatory bikeshed, wouldn't you say _MASKED_BIT_{ENABLE,DISABLE} are
>> special cases of _MASKED_FIELD...? ;)
> 
> That's because we're not just enabling or disabling bits here but
> setting a multi-bits value.
> 
>   _MASKED_FIELD(2 << 4, 0x3 << 4);
> 

So you could

#define	__MASKED_BIT_DISABLE(a)		(__MASKED_FIELD(0, (a)))
#define	__MASKED_BIT_ENABLE(a)		(__MASKED_FIELD((a), (a)))

which I think is what Jani was referring to ...

Bikeshed++: do we care about the double evaluation of (a) in these macros?

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 13:59   ` Damien Lespiau
  2014-12-08 14:17     ` Dave Gordon
@ 2014-12-08 14:21     ` Daniel Vetter
  2014-12-08 14:23       ` Daniel Vetter
  1 sibling, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2014-12-08 14:21 UTC (permalink / raw)
  To: Damien Lespiau; +Cc: intel-gfx

On Mon, Dec 08, 2014 at 01:59:50PM +0000, Damien Lespiau wrote:
> On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote:
> > >  #define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
> > >  #define _MASKED_BIT_DISABLE(a) ((a) << 16)
> > > +#define _MASKED_FIELD(value, mask) (((mask) << 16) | (value))
> > 
> > Obligatory bikeshed, wouldn't you say _MASKED_BIT_{ENABLE,DISABLE} are
> > special cases of _MASKED_FIELD...? ;)
> 
> That's because we're not just enabling or disabling bits here but
> setting a multi-bits value.
> 
>   _MASKED_FIELD(2 << 4, 0x3 << 4);

#define _MASKED_BIT_ENABLE(a) _MASKED_FIELD(a, a)
#define _MASKED_BIT_DISABLE(a) _MASKED_FIELD(a, 0)

is what I guess Jani thought of.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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] 20+ messages in thread
* Re: [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 14:21     ` Daniel Vetter
@ 2014-12-08 14:23       ` Daniel Vetter
  2014-12-08 14:46         ` Damien Lespiau
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2014-12-08 14:23 UTC (permalink / raw)
  To: Damien Lespiau; +Cc: intel-gfx

On Mon, Dec 08, 2014 at 03:21:02PM +0100, Daniel Vetter wrote:
> On Mon, Dec 08, 2014 at 01:59:50PM +0000, Damien Lespiau wrote:
> > On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote:
> > > >  #define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
> > > >  #define _MASKED_BIT_DISABLE(a) ((a) << 16)
> > > > +#define _MASKED_FIELD(value, mask) (((mask) << 16) | (value))
> > > 
> > > Obligatory bikeshed, wouldn't you say _MASKED_BIT_{ENABLE,DISABLE} are
> > > special cases of _MASKED_FIELD...? ;)
> > 
> > That's because we're not just enabling or disabling bits here but
> > setting a multi-bits value.
> > 
> >   _MASKED_FIELD(2 << 4, 0x3 << 4);
> 
> #define _MASKED_BIT_ENABLE(a) _MASKED_FIELD(a, a)
> #define _MASKED_BIT_DISABLE(a) _MASKED_FIELD(a, 0)

Ok and I right away screwed up the argument ordering in the DISABLE one
because the bits we set are before the mask. All the bitmasking functions
we have in e.g. i915_irq.c ilk_update_gt_irq so for consistency I think we
should flip it in this one here, too. Otherwise that bit of inconsistency
will trip up tons of people in the future.

Jani, can you please apply that fixup if Damien acks it?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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] 20+ messages in thread
* Re: [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 14:17     ` Dave Gordon
@ 2014-12-08 14:36       ` Damien Lespiau
  0 siblings, 0 replies; 20+ messages in thread
From: Damien Lespiau @ 2014-12-08 14:36 UTC (permalink / raw)
  To: Dave Gordon; +Cc: intel-gfx

On Mon, Dec 08, 2014 at 02:17:45PM +0000, Dave Gordon wrote:
> On 08/12/14 13:59, Damien Lespiau wrote:
> > On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote:
> >>>  #define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
> >>>  #define _MASKED_BIT_DISABLE(a) ((a) << 16)
> >>> +#define _MASKED_FIELD(value, mask) (((mask) << 16) | (value))
> >>
> >> Obligatory bikeshed, wouldn't you say _MASKED_BIT_{ENABLE,DISABLE} are
> >> special cases of _MASKED_FIELD...? ;)
> > 
> > That's because we're not just enabling or disabling bits here but
> > setting a multi-bits value.
> > 
> >   _MASKED_FIELD(2 << 4, 0x3 << 4);
> > 
> 
> So you could
> 
> #define	__MASKED_BIT_DISABLE(a)		(__MASKED_FIELD(0, (a)))
> #define	__MASKED_BIT_ENABLE(a)		(__MASKED_FIELD((a), (a)))
> 
> which I think is what Jani was referring to ...
> 
> Bikeshed++: do we care about the double evaluation of (a) in these macros?

Oh of course, misread that. We could also avoid the double evaluation
indeed and chris suggests (on IRC) BUILD_BUG_ON(bit & ~mask); (super
good!)

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 14:23       ` Daniel Vetter
@ 2014-12-08 14:46         ` Damien Lespiau
  0 siblings, 0 replies; 20+ messages in thread
From: Damien Lespiau @ 2014-12-08 14:46 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Mon, Dec 08, 2014 at 03:23:49PM +0100, Daniel Vetter wrote:
> > #define _MASKED_BIT_ENABLE(a) _MASKED_FIELD(a, a)
> > #define _MASKED_BIT_DISABLE(a) _MASKED_FIELD(a, 0)
> 
> Ok and I right away screwed up the argument ordering in the DISABLE one
> because the bits we set are before the mask. All the bitmasking functions
> we have in e.g. i915_irq.c ilk_update_gt_irq so for consistency I think we
> should flip it in this one here, too. Otherwise that bit of inconsistency
> will trip up tons of people in the future.
> 
> Jani, can you please apply that fixup if Damien acks it?

(for the record ack'ed)

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH v2] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 12:33 ` Jani Nikula
  2014-12-08 13:59   ` Damien Lespiau
@ 2014-12-08 16:22   ` Damien Lespiau
  2014-12-08 16:27     ` Daniel Vetter
                       ` (2 more replies)
  1 sibling, 3 replies; 20+ messages in thread
From: Damien Lespiau @ 2014-12-08 16:22 UTC (permalink / raw)
  To: intel-gfx

I was playing with clang and oh surprise! a warning trigerred by
-Wshift-overflow (gcc doesn't have this one):

    WA_SET_BIT_MASKED(GEN7_GT_MODE,
                      GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);

    drivers/gpu/drm/i915/intel_ringbuffer.c:786:2: warning: signed shift result
      (0x28002000000) requires 43 bits to represent, but 'int' only has 32 bits
      [-Wshift-overflow]
        WA_SET_BIT_MASKED(GEN7_GT_MODE,
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/gpu/drm/i915/intel_ringbuffer.c:737:15: note: expanded from macro
      'WA_SET_BIT_MASKED'
        WA_REG(addr, _MASKED_BIT_ENABLE(mask), (mask) & 0xffff)

Turned out GEN6_WIZ_HASHING_MASK was already shifted by 16, and we were
trying to shift it a bit more.

The other thing is that it's not the usual case of setting WA bits here, we
need to have separate mask and value.

To fix this, I've introduced a new _MASKED_FIELD() macro that takes both the
(unshifted) mask and the desired value and the rest of the patch ripples
through from it.

This bug was introduced when reworking the WA emission in:

  Commit 7225342ab501befdb64bcec76ded41f5897c0855
  Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
  Date:   Tue Oct 7 17:21:26 2014 +0300

      drm/i915: Build workaround list in ring initialization

v2: Invert the order of the mask and value arguments (Daniel Vetter)
    Rewrite _MASKED_BIT_ENABLE() and _MASKED_BIT_DISABLE() with
    _MASKED_FIELD() (Jani Nikula)
    Make sure we only evaluate 'a' once in _MASKED_BIT_ENABLE() (Dave Gordon)
    Add check to ensure the value is within the mask boundaries (Chris Wilson)

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 13 ++++++++++---
 drivers/gpu/drm/i915/intel_pm.c         |  6 +++---
 drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ++++++--
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index dc03fac..e0cd461 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -34,8 +34,15 @@
 #define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
 			       (port) == PORT_B ? (b) : (c))
 
-#define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
-#define _MASKED_BIT_DISABLE(a) ((a) << 16)
+#define _MASKED_FIELD(mask, value) ({					\
+	if (__builtin_constant_p(mask) && __builtin_constant_p(value))	\
+		BUILD_BUG_ON_MSG((value) & ~(mask),			\
+				 "Incorrect value for mask");		\
+	(mask) << 16 | (value); })
+#define _MASKED_BIT_ENABLE(a)	({ typeof(a) _a = (a); _MASKED_FIELD(_a, _a); })
+#define _MASKED_BIT_DISABLE(a)	(_MASKED_FIELD((a), 0))
+
+
 
 /* PCI config space */
 
@@ -1284,7 +1291,7 @@ enum punit_power_well {
 #define   GEN6_WIZ_HASHING_8x8				GEN6_WIZ_HASHING(0, 0)
 #define   GEN6_WIZ_HASHING_8x4				GEN6_WIZ_HASHING(0, 1)
 #define   GEN6_WIZ_HASHING_16x4				GEN6_WIZ_HASHING(1, 0)
-#define   GEN6_WIZ_HASHING_MASK				(GEN6_WIZ_HASHING(1, 1) << 16)
+#define   GEN6_WIZ_HASHING_MASK				GEN6_WIZ_HASHING(1, 1)
 #define   GEN6_TD_FOUR_ROW_DISPATCH_DISABLE		(1 << 5)
 
 #define GFX_MODE	0x02520
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 78911e2..0f2febd 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6389,7 +6389,7 @@ static void gen6_init_clock_gating(struct drm_device *dev)
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
 	I915_WRITE(GEN6_GT_MODE,
-		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
 
 	ilk_init_lp_watermarks(dev);
 
@@ -6587,7 +6587,7 @@ static void haswell_init_clock_gating(struct drm_device *dev)
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
 	I915_WRITE(GEN7_GT_MODE,
-		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
 
 	/* WaSwitchSolVfFArbitrationPriority:hsw */
 	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
@@ -6684,7 +6684,7 @@ static void ivybridge_init_clock_gating(struct drm_device *dev)
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
 	I915_WRITE(GEN7_GT_MODE,
-		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
 
 	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
 	snpcr &= ~GEN6_MBC_SNPCR_MASK;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 79b4ca5..9deb152 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -739,6 +739,9 @@ static int wa_add(struct drm_i915_private *dev_priv,
 #define WA_CLR_BIT_MASKED(addr, mask) \
 	WA_REG(addr, _MASKED_BIT_DISABLE(mask), (mask) & 0xffff)
 
+#define WA_SET_FIELD_MASKED(addr, mask, value) \
+	WA_REG(addr, _MASKED_FIELD(mask, value), mask)
+
 #define WA_SET_BIT(addr, mask) WA_REG(addr, I915_READ(addr) | (mask), mask)
 #define WA_CLR_BIT(addr, mask) WA_REG(addr, I915_READ(addr) & ~(mask), mask)
 
@@ -783,8 +786,9 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	 * disable bit, which we don't touch here, but it's good
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
-	WA_SET_BIT_MASKED(GEN7_GT_MODE,
-			  GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+	WA_SET_FIELD_MASKED(GEN7_GT_MODE,
+			    GEN6_WIZ_HASHING_MASK,
+			    GEN6_WIZ_HASHING_16x4);
 
 	return 0;
 }
-- 
1.8.3.1

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 16:22   ` [PATCH v2] " Damien Lespiau
@ 2014-12-08 16:27     ` Daniel Vetter
  2014-12-08 16:50       ` Dave Gordon
  2014-12-08 16:56     ` Dave Gordon
  2014-12-09 19:14     ` [PATCH v2] " shuang.he
  2 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2014-12-08 16:27 UTC (permalink / raw)
  To: Damien Lespiau; +Cc: intel-gfx

On Mon, Dec 08, 2014 at 04:22:27PM +0000, Damien Lespiau wrote:
> I was playing with clang and oh surprise! a warning trigerred by
> -Wshift-overflow (gcc doesn't have this one):
> 
>     WA_SET_BIT_MASKED(GEN7_GT_MODE,
>                       GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> 
>     drivers/gpu/drm/i915/intel_ringbuffer.c:786:2: warning: signed shift result
>       (0x28002000000) requires 43 bits to represent, but 'int' only has 32 bits
>       [-Wshift-overflow]
>         WA_SET_BIT_MASKED(GEN7_GT_MODE,
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/gpu/drm/i915/intel_ringbuffer.c:737:15: note: expanded from macro
>       'WA_SET_BIT_MASKED'
>         WA_REG(addr, _MASKED_BIT_ENABLE(mask), (mask) & 0xffff)
> 
> Turned out GEN6_WIZ_HASHING_MASK was already shifted by 16, and we were
> trying to shift it a bit more.
> 
> The other thing is that it's not the usual case of setting WA bits here, we
> need to have separate mask and value.
> 
> To fix this, I've introduced a new _MASKED_FIELD() macro that takes both the
> (unshifted) mask and the desired value and the rest of the patch ripples
> through from it.
> 
> This bug was introduced when reworking the WA emission in:
> 
>   Commit 7225342ab501befdb64bcec76ded41f5897c0855
>   Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>   Date:   Tue Oct 7 17:21:26 2014 +0300
> 
>       drm/i915: Build workaround list in ring initialization
> 
> v2: Invert the order of the mask and value arguments (Daniel Vetter)
>     Rewrite _MASKED_BIT_ENABLE() and _MASKED_BIT_DISABLE() with
>     _MASKED_FIELD() (Jani Nikula)
>     Make sure we only evaluate 'a' once in _MASKED_BIT_ENABLE() (Dave Gordon)
>     Add check to ensure the value is within the mask boundaries (Chris Wilson)
> 
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 13 ++++++++++---
>  drivers/gpu/drm/i915/intel_pm.c         |  6 +++---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ++++++--
>  3 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dc03fac..e0cd461 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -34,8 +34,15 @@
>  #define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
>  			       (port) == PORT_B ? (b) : (c))
>  
> -#define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
> -#define _MASKED_BIT_DISABLE(a) ((a) << 16)
> +#define _MASKED_FIELD(mask, value) ({					\
> +	if (__builtin_constant_p(mask) && __builtin_constant_p(value))	\
> +		BUILD_BUG_ON_MSG((value) & ~(mask),			\
> +				 "Incorrect value for mask");		\

Imo just use WARN_ON, with my patch it'll do the right thing. And maybe
we'll use this in some context once where it's not a compile-time
constant and then the runtime check would be better.

With that addressed it's Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I just realized that WA_REG has the order the other way round too, maybe
throw a follow-up on top?
-Daniel

> +	(mask) << 16 | (value); })
> +#define _MASKED_BIT_ENABLE(a)	({ typeof(a) _a = (a); _MASKED_FIELD(_a, _a); })
> +#define _MASKED_BIT_DISABLE(a)	(_MASKED_FIELD((a), 0))
> +
> +
>  
>  /* PCI config space */
>  
> @@ -1284,7 +1291,7 @@ enum punit_power_well {
>  #define   GEN6_WIZ_HASHING_8x8				GEN6_WIZ_HASHING(0, 0)
>  #define   GEN6_WIZ_HASHING_8x4				GEN6_WIZ_HASHING(0, 1)
>  #define   GEN6_WIZ_HASHING_16x4				GEN6_WIZ_HASHING(1, 0)
> -#define   GEN6_WIZ_HASHING_MASK				(GEN6_WIZ_HASHING(1, 1) << 16)
> +#define   GEN6_WIZ_HASHING_MASK				GEN6_WIZ_HASHING(1, 1)
>  #define   GEN6_TD_FOUR_ROW_DISPATCH_DISABLE		(1 << 5)
>  
>  #define GFX_MODE	0x02520
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 78911e2..0f2febd 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6389,7 +6389,7 @@ static void gen6_init_clock_gating(struct drm_device *dev)
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
>  	I915_WRITE(GEN6_GT_MODE,
> -		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
>  
>  	ilk_init_lp_watermarks(dev);
>  
> @@ -6587,7 +6587,7 @@ static void haswell_init_clock_gating(struct drm_device *dev)
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
>  	I915_WRITE(GEN7_GT_MODE,
> -		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
>  
>  	/* WaSwitchSolVfFArbitrationPriority:hsw */
>  	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
> @@ -6684,7 +6684,7 @@ static void ivybridge_init_clock_gating(struct drm_device *dev)
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
>  	I915_WRITE(GEN7_GT_MODE,
> -		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
>  
>  	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
>  	snpcr &= ~GEN6_MBC_SNPCR_MASK;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 79b4ca5..9deb152 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -739,6 +739,9 @@ static int wa_add(struct drm_i915_private *dev_priv,
>  #define WA_CLR_BIT_MASKED(addr, mask) \
>  	WA_REG(addr, _MASKED_BIT_DISABLE(mask), (mask) & 0xffff)
>  
> +#define WA_SET_FIELD_MASKED(addr, mask, value) \
> +	WA_REG(addr, _MASKED_FIELD(mask, value), mask)
> +
>  #define WA_SET_BIT(addr, mask) WA_REG(addr, I915_READ(addr) | (mask), mask)
>  #define WA_CLR_BIT(addr, mask) WA_REG(addr, I915_READ(addr) & ~(mask), mask)
>  
> @@ -783,8 +786,9 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	 * disable bit, which we don't touch here, but it's good
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
> -	WA_SET_BIT_MASKED(GEN7_GT_MODE,
> -			  GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +	WA_SET_FIELD_MASKED(GEN7_GT_MODE,
> +			    GEN6_WIZ_HASHING_MASK,
> +			    GEN6_WIZ_HASHING_16x4);
>  
>  	return 0;
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 16:27     ` Daniel Vetter
@ 2014-12-08 16:50       ` Dave Gordon
  2014-12-08 16:54         ` Damien Lespiau
  0 siblings, 1 reply; 20+ messages in thread
From: Dave Gordon @ 2014-12-08 16:50 UTC (permalink / raw)
  To: Daniel Vetter, Damien Lespiau; +Cc: intel-gfx

On 08/12/14 16:27, Daniel Vetter wrote:
> On Mon, Dec 08, 2014 at 04:22:27PM +0000, Damien Lespiau wrote:
>> I was playing with clang and oh surprise! a warning trigerred by
>> -Wshift-overflow (gcc doesn't have this one):

[snip]

>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 79b4ca5..9deb152 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -739,6 +739,9 @@ static int wa_add(struct drm_i915_private *dev_priv,
>>  #define WA_CLR_BIT_MASKED(addr, mask) \
>>  	WA_REG(addr, _MASKED_BIT_DISABLE(mask), (mask) & 0xffff)
>>  
>> +#define WA_SET_FIELD_MASKED(addr, mask, value) \
>> +	WA_REG(addr, _MASKED_FIELD(mask, value), mask)
>> +
>>  #define WA_SET_BIT(addr, mask) WA_REG(addr, I915_READ(addr) | (mask), mask)
>>  #define WA_CLR_BIT(addr, mask) WA_REG(addr, I915_READ(addr) & ~(mask), mask)

Not your changes, but:

* WA_{SET,CLR}_BIT() above look dubious and don't seem to be used anyway

* dev_priv->workarounds.reg[idx].mask = mask;

The mask field is set but not used in intel_ring_workarounds_emit() or
intel_logical_ring_workarounds_emit(), only in debugfs printout.
And it's redundant since the 'value' incorporates the bit(field) mask
and the new target value into one parameter, hence 3rd parameter of
WA_REG() is surplus and calculating it in WA_{SET,CLR_BIT_MASKED() is
also redundant.

Unless I've missed something?

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 16:50       ` Dave Gordon
@ 2014-12-08 16:54         ` Damien Lespiau
  0 siblings, 0 replies; 20+ messages in thread
From: Damien Lespiau @ 2014-12-08 16:54 UTC (permalink / raw)
  To: Dave Gordon; +Cc: intel-gfx

On Mon, Dec 08, 2014 at 04:50:13PM +0000, Dave Gordon wrote:
> On 08/12/14 16:27, Daniel Vetter wrote:
> > On Mon, Dec 08, 2014 at 04:22:27PM +0000, Damien Lespiau wrote:
> >> I was playing with clang and oh surprise! a warning trigerred by
> >> -Wshift-overflow (gcc doesn't have this one):
> 
> [snip]
> 
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index 79b4ca5..9deb152 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -739,6 +739,9 @@ static int wa_add(struct drm_i915_private *dev_priv,
> >>  #define WA_CLR_BIT_MASKED(addr, mask) \
> >>  	WA_REG(addr, _MASKED_BIT_DISABLE(mask), (mask) & 0xffff)
> >>  
> >> +#define WA_SET_FIELD_MASKED(addr, mask, value) \
> >> +	WA_REG(addr, _MASKED_FIELD(mask, value), mask)
> >> +
> >>  #define WA_SET_BIT(addr, mask) WA_REG(addr, I915_READ(addr) | (mask), mask)
> >>  #define WA_CLR_BIT(addr, mask) WA_REG(addr, I915_READ(addr) & ~(mask), mask)
> 
> Not your changes, but:
> 
> * WA_{SET,CLR}_BIT() above look dubious and don't seem to be used anyway
> 
> * dev_priv->workarounds.reg[idx].mask = mask;
> 
> The mask field is set but not used in intel_ring_workarounds_emit() or
> intel_logical_ring_workarounds_emit(), only in debugfs printout.
> And it's redundant since the 'value' incorporates the bit(field) mask
> and the new target value into one parameter, hence 3rd parameter of
> WA_REG() is surplus and calculating it in WA_{SET,CLR_BIT_MASKED() is
> also redundant.
> 
> Unless I've missed something?

The mask is used to test that we correctly set/clear W/A values in i-g-t
tests. Imagine the W/A being "clear bit 2", we have a generic (value,
mask) to check that we do indeed do that.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 16:22   ` [PATCH v2] " Damien Lespiau
  2014-12-08 16:27     ` Daniel Vetter
@ 2014-12-08 16:56     ` Dave Gordon
  2014-12-08 17:33       ` [PATCH v3] " Damien Lespiau
  2014-12-09 19:14     ` [PATCH v2] " shuang.he
  2 siblings, 1 reply; 20+ messages in thread
From: Dave Gordon @ 2014-12-08 16:56 UTC (permalink / raw)
  To: Damien Lespiau, intel-gfx

On 08/12/14 16:22, Damien Lespiau wrote:
> I was playing with clang and oh surprise! a warning trigerred by
> -Wshift-overflow (gcc doesn't have this one):
> 
>     WA_SET_BIT_MASKED(GEN7_GT_MODE,
>                       GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> 
>     drivers/gpu/drm/i915/intel_ringbuffer.c:786:2: warning: signed shift result
>       (0x28002000000) requires 43 bits to represent, but 'int' only has 32 bits
>       [-Wshift-overflow]
>         WA_SET_BIT_MASKED(GEN7_GT_MODE,
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/gpu/drm/i915/intel_ringbuffer.c:737:15: note: expanded from macro
>       'WA_SET_BIT_MASKED'
>         WA_REG(addr, _MASKED_BIT_ENABLE(mask), (mask) & 0xffff)
> 
> Turned out GEN6_WIZ_HASHING_MASK was already shifted by 16, and we were
> trying to shift it a bit more.
> 
> The other thing is that it's not the usual case of setting WA bits here, we
> need to have separate mask and value.
> 
> To fix this, I've introduced a new _MASKED_FIELD() macro that takes both the
> (unshifted) mask and the desired value and the rest of the patch ripples
> through from it.
> 
> This bug was introduced when reworking the WA emission in:
> 
>   Commit 7225342ab501befdb64bcec76ded41f5897c0855
>   Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>   Date:   Tue Oct 7 17:21:26 2014 +0300
> 
>       drm/i915: Build workaround list in ring initialization
> 
> v2: Invert the order of the mask and value arguments (Daniel Vetter)
>     Rewrite _MASKED_BIT_ENABLE() and _MASKED_BIT_DISABLE() with
>     _MASKED_FIELD() (Jani Nikula)
>     Make sure we only evaluate 'a' once in _MASKED_BIT_ENABLE() (Dave Gordon)
>     Add check to ensure the value is within the mask boundaries (Chris Wilson)
> 
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 13 ++++++++++---
>  drivers/gpu/drm/i915/intel_pm.c         |  6 +++---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ++++++--
>  3 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dc03fac..e0cd461 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -34,8 +34,15 @@
>  #define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
>  			       (port) == PORT_B ? (b) : (c))
>  
> -#define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
> -#define _MASKED_BIT_DISABLE(a) ((a) << 16)
> +#define _MASKED_FIELD(mask, value) ({					\
> +	if (__builtin_constant_p(mask) && __builtin_constant_p(value))	\
> +		BUILD_BUG_ON_MSG((value) & ~(mask),			\
> +				 "Incorrect value for mask");		\
> +	(mask) << 16 | (value); })

For even more compile- and run-time robustness we could check that
'mask' and 'value' each fit in 16 bits, as we have an explicit '16' in
there already.

.Dave.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH v3] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 16:56     ` Dave Gordon
@ 2014-12-08 17:33       ` Damien Lespiau
  2014-12-09 22:14         ` shuang.he
  2014-12-10  9:42         ` Jani Nikula
  0 siblings, 2 replies; 20+ messages in thread
From: Damien Lespiau @ 2014-12-08 17:33 UTC (permalink / raw)
  To: intel-gfx

I was playing with clang and oh surprise! a warning trigerred by
-Wshift-overflow (gcc doesn't have this one):

    WA_SET_BIT_MASKED(GEN7_GT_MODE,
                      GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);

    drivers/gpu/drm/i915/intel_ringbuffer.c:786:2: warning: signed shift result
      (0x28002000000) requires 43 bits to represent, but 'int' only has 32 bits
      [-Wshift-overflow]
        WA_SET_BIT_MASKED(GEN7_GT_MODE,
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/gpu/drm/i915/intel_ringbuffer.c:737:15: note: expanded from macro
      'WA_SET_BIT_MASKED'
        WA_REG(addr, _MASKED_BIT_ENABLE(mask), (mask) & 0xffff)

Turned out GEN6_WIZ_HASHING_MASK was already shifted by 16, and we were
trying to shift it a bit more.

The other thing is that it's not the usual case of setting WA bits here, we
need to have separate mask and value.

To fix this, I've introduced a new _MASKED_FIELD() macro that takes both the
(unshifted) mask and the desired value and the rest of the patch ripples
through from it.

This bug was introduced when reworking the WA emission in:

  Commit 7225342ab501befdb64bcec76ded41f5897c0855
  Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
  Date:   Tue Oct 7 17:21:26 2014 +0300

      drm/i915: Build workaround list in ring initialization

v2: Invert the order of the mask and value arguments (Daniel Vetter)
    Rewrite _MASKED_BIT_ENABLE() and _MASKED_BIT_DISABLE() with
    _MASKED_FIELD() (Jani Nikula)
    Make sure we only evaluate 'a' once in _MASKED_BIT_ENABLE() (Dave Gordon)
    Add check to ensure the value is within the mask boundaries (Chris Wilson)

v3: Ensure the the value and mask are 16 bits (Dave Gordon)

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 17 ++++++++++++++---
 drivers/gpu/drm/i915/intel_pm.c         |  6 +++---
 drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ++++++--
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index dc03fac..454a3a3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -34,8 +34,19 @@
 #define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
 			       (port) == PORT_B ? (b) : (c))
 
-#define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
-#define _MASKED_BIT_DISABLE(a) ((a) << 16)
+#define _MASKED_FIELD(mask, value) ({					   \
+	if (__builtin_constant_p(mask))					   \
+		BUILD_BUG_ON_MSG(((mask) & 0xffff0000), "Incorrect mask"); \
+	if (__builtin_constant_p(value))				   \
+		BUILD_BUG_ON_MSG((value) & 0xffff0000, "Incorrect value"); \
+	if (__builtin_constant_p(mask) && __builtin_constant_p(value))	   \
+		BUILD_BUG_ON_MSG((value) & ~(mask),			   \
+				 "Incorrect value for mask");		   \
+	(mask) << 16 | (value); })
+#define _MASKED_BIT_ENABLE(a)	({ typeof(a) _a = (a); _MASKED_FIELD(_a, _a); })
+#define _MASKED_BIT_DISABLE(a)	(_MASKED_FIELD((a), 0))
+
+
 
 /* PCI config space */
 
@@ -1284,7 +1295,7 @@ enum punit_power_well {
 #define   GEN6_WIZ_HASHING_8x8				GEN6_WIZ_HASHING(0, 0)
 #define   GEN6_WIZ_HASHING_8x4				GEN6_WIZ_HASHING(0, 1)
 #define   GEN6_WIZ_HASHING_16x4				GEN6_WIZ_HASHING(1, 0)
-#define   GEN6_WIZ_HASHING_MASK				(GEN6_WIZ_HASHING(1, 1) << 16)
+#define   GEN6_WIZ_HASHING_MASK				GEN6_WIZ_HASHING(1, 1)
 #define   GEN6_TD_FOUR_ROW_DISPATCH_DISABLE		(1 << 5)
 
 #define GFX_MODE	0x02520
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 78911e2..0f2febd 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6389,7 +6389,7 @@ static void gen6_init_clock_gating(struct drm_device *dev)
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
 	I915_WRITE(GEN6_GT_MODE,
-		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
 
 	ilk_init_lp_watermarks(dev);
 
@@ -6587,7 +6587,7 @@ static void haswell_init_clock_gating(struct drm_device *dev)
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
 	I915_WRITE(GEN7_GT_MODE,
-		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
 
 	/* WaSwitchSolVfFArbitrationPriority:hsw */
 	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
@@ -6684,7 +6684,7 @@ static void ivybridge_init_clock_gating(struct drm_device *dev)
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
 	I915_WRITE(GEN7_GT_MODE,
-		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
 
 	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
 	snpcr &= ~GEN6_MBC_SNPCR_MASK;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 79b4ca5..9deb152 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -739,6 +739,9 @@ static int wa_add(struct drm_i915_private *dev_priv,
 #define WA_CLR_BIT_MASKED(addr, mask) \
 	WA_REG(addr, _MASKED_BIT_DISABLE(mask), (mask) & 0xffff)
 
+#define WA_SET_FIELD_MASKED(addr, mask, value) \
+	WA_REG(addr, _MASKED_FIELD(mask, value), mask)
+
 #define WA_SET_BIT(addr, mask) WA_REG(addr, I915_READ(addr) | (mask), mask)
 #define WA_CLR_BIT(addr, mask) WA_REG(addr, I915_READ(addr) & ~(mask), mask)
 
@@ -783,8 +786,9 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
 	 * disable bit, which we don't touch here, but it's good
 	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
 	 */
-	WA_SET_BIT_MASKED(GEN7_GT_MODE,
-			  GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
+	WA_SET_FIELD_MASKED(GEN7_GT_MODE,
+			    GEN6_WIZ_HASHING_MASK,
+			    GEN6_WIZ_HASHING_16x4);
 
 	return 0;
 }
-- 
1.8.3.1

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 16:22   ` [PATCH v2] " Damien Lespiau
  2014-12-08 16:27     ` Daniel Vetter
  2014-12-08 16:56     ` Dave Gordon
@ 2014-12-09 19:14     ` shuang.he
  2 siblings, 0 replies; 20+ messages in thread
From: shuang.he @ 2014-12-09 19:14 UTC (permalink / raw)
  To: shuang.he, intel-gfx, damien.lespiau

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1                 364/366              365/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(1, M26)PASS(7, M26M37)      PASS(1, M37)
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] 20+ messages in thread
* Re: [PATCH v3] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 17:33       ` [PATCH v3] " Damien Lespiau
@ 2014-12-09 22:14         ` shuang.he
  2014-12-10  9:42         ` Jani Nikula
  1 sibling, 0 replies; 20+ messages in thread
From: shuang.he @ 2014-12-09 22:14 UTC (permalink / raw)
  To: shuang.he, intel-gfx, damien.lespiau

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1                 364/366              365/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(1, M26)PASS(9, M26M37)      PASS(1, M37)
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] 20+ messages in thread
* Re: [PATCH v3] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-08 17:33       ` [PATCH v3] " Damien Lespiau
  2014-12-09 22:14         ` shuang.he
@ 2014-12-10  9:42         ` Jani Nikula
  2014-12-10 12:03           ` Damien Lespiau
  1 sibling, 1 reply; 20+ messages in thread
From: Jani Nikula @ 2014-12-10  9:42 UTC (permalink / raw)
  To: Damien Lespiau, intel-gfx

On Mon, 08 Dec 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> I was playing with clang and oh surprise! a warning trigerred by
> -Wshift-overflow (gcc doesn't have this one):
>
>     WA_SET_BIT_MASKED(GEN7_GT_MODE,
>                       GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
>
>     drivers/gpu/drm/i915/intel_ringbuffer.c:786:2: warning: signed shift result
>       (0x28002000000) requires 43 bits to represent, but 'int' only has 32 bits
>       [-Wshift-overflow]
>         WA_SET_BIT_MASKED(GEN7_GT_MODE,
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/gpu/drm/i915/intel_ringbuffer.c:737:15: note: expanded from macro
>       'WA_SET_BIT_MASKED'
>         WA_REG(addr, _MASKED_BIT_ENABLE(mask), (mask) & 0xffff)
>
> Turned out GEN6_WIZ_HASHING_MASK was already shifted by 16, and we were
> trying to shift it a bit more.
>
> The other thing is that it's not the usual case of setting WA bits here, we
> need to have separate mask and value.
>
> To fix this, I've introduced a new _MASKED_FIELD() macro that takes both the
> (unshifted) mask and the desired value and the rest of the patch ripples
> through from it.
>
> This bug was introduced when reworking the WA emission in:
>
>   Commit 7225342ab501befdb64bcec76ded41f5897c0855
>   Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>   Date:   Tue Oct 7 17:21:26 2014 +0300
>
>       drm/i915: Build workaround list in ring initialization
>
> v2: Invert the order of the mask and value arguments (Daniel Vetter)
>     Rewrite _MASKED_BIT_ENABLE() and _MASKED_BIT_DISABLE() with
>     _MASKED_FIELD() (Jani Nikula)
>     Make sure we only evaluate 'a' once in _MASKED_BIT_ENABLE() (Dave Gordon)
>     Add check to ensure the value is within the mask boundaries (Chris Wilson)
>
> v3: Ensure the the value and mask are 16 bits (Dave Gordon)
>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Pushed to drm-intel-next-fixes, replacing the old version. Thanks for
the patch.

Now the question is, do we want [1] for 3.19 or 3.20?

BR,
Jani.


http://mid.gmane.org/1418060138-5004-1-git-send-email-damien.lespiau@intel.com


> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 17 ++++++++++++++---
>  drivers/gpu/drm/i915/intel_pm.c         |  6 +++---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ++++++--
>  3 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dc03fac..454a3a3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -34,8 +34,19 @@
>  #define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
>  			       (port) == PORT_B ? (b) : (c))
>  
> -#define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
> -#define _MASKED_BIT_DISABLE(a) ((a) << 16)
> +#define _MASKED_FIELD(mask, value) ({					   \
> +	if (__builtin_constant_p(mask))					   \
> +		BUILD_BUG_ON_MSG(((mask) & 0xffff0000), "Incorrect mask"); \
> +	if (__builtin_constant_p(value))				   \
> +		BUILD_BUG_ON_MSG((value) & 0xffff0000, "Incorrect value"); \
> +	if (__builtin_constant_p(mask) && __builtin_constant_p(value))	   \
> +		BUILD_BUG_ON_MSG((value) & ~(mask),			   \
> +				 "Incorrect value for mask");		   \
> +	(mask) << 16 | (value); })
> +#define _MASKED_BIT_ENABLE(a)	({ typeof(a) _a = (a); _MASKED_FIELD(_a, _a); })
> +#define _MASKED_BIT_DISABLE(a)	(_MASKED_FIELD((a), 0))
> +
> +
>  
>  /* PCI config space */
>  
> @@ -1284,7 +1295,7 @@ enum punit_power_well {
>  #define   GEN6_WIZ_HASHING_8x8				GEN6_WIZ_HASHING(0, 0)
>  #define   GEN6_WIZ_HASHING_8x4				GEN6_WIZ_HASHING(0, 1)
>  #define   GEN6_WIZ_HASHING_16x4				GEN6_WIZ_HASHING(1, 0)
> -#define   GEN6_WIZ_HASHING_MASK				(GEN6_WIZ_HASHING(1, 1) << 16)
> +#define   GEN6_WIZ_HASHING_MASK				GEN6_WIZ_HASHING(1, 1)
>  #define   GEN6_TD_FOUR_ROW_DISPATCH_DISABLE		(1 << 5)
>  
>  #define GFX_MODE	0x02520
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 78911e2..0f2febd 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6389,7 +6389,7 @@ static void gen6_init_clock_gating(struct drm_device *dev)
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
>  	I915_WRITE(GEN6_GT_MODE,
> -		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
>  
>  	ilk_init_lp_watermarks(dev);
>  
> @@ -6587,7 +6587,7 @@ static void haswell_init_clock_gating(struct drm_device *dev)
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
>  	I915_WRITE(GEN7_GT_MODE,
> -		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
>  
>  	/* WaSwitchSolVfFArbitrationPriority:hsw */
>  	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
> @@ -6684,7 +6684,7 @@ static void ivybridge_init_clock_gating(struct drm_device *dev)
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
>  	I915_WRITE(GEN7_GT_MODE,
> -		   GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
>  
>  	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
>  	snpcr &= ~GEN6_MBC_SNPCR_MASK;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 79b4ca5..9deb152 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -739,6 +739,9 @@ static int wa_add(struct drm_i915_private *dev_priv,
>  #define WA_CLR_BIT_MASKED(addr, mask) \
>  	WA_REG(addr, _MASKED_BIT_DISABLE(mask), (mask) & 0xffff)
>  
> +#define WA_SET_FIELD_MASKED(addr, mask, value) \
> +	WA_REG(addr, _MASKED_FIELD(mask, value), mask)
> +
>  #define WA_SET_BIT(addr, mask) WA_REG(addr, I915_READ(addr) | (mask), mask)
>  #define WA_CLR_BIT(addr, mask) WA_REG(addr, I915_READ(addr) & ~(mask), mask)
>  
> @@ -783,8 +786,9 @@ static int bdw_init_workarounds(struct intel_engine_cs *ring)
>  	 * disable bit, which we don't touch here, but it's good
>  	 * to keep in mind (see 3DSTATE_PS and 3DSTATE_WM).
>  	 */
> -	WA_SET_BIT_MASKED(GEN7_GT_MODE,
> -			  GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4);
> +	WA_SET_FIELD_MASKED(GEN7_GT_MODE,
> +			    GEN6_WIZ_HASHING_MASK,
> +			    GEN6_WIZ_HASHING_16x4);
>  
>  	return 0;
>  }
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v3] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-10  9:42         ` Jani Nikula
@ 2014-12-10 12:03           ` Damien Lespiau
  2014-12-10 13:55             ` Daniel Vetter
  0 siblings, 1 reply; 20+ messages in thread
From: Damien Lespiau @ 2014-12-10 12:03 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Wed, Dec 10, 2014 at 11:42:13AM +0200, Jani Nikula wrote:
> Pushed to drm-intel-next-fixes, replacing the old version. Thanks for
> the patch.
> 
> Now the question is, do we want [1] for 3.19 or 3.20?
> 
> [1] http://mid.gmane.org/1418060138-5004-1-git-send-email-damien.lespiau@intel.com

I think that one is for 3.20, not really fixing a bug.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH v3] drm/i915/bdw: Fix the write setting up the WIZ hashing mode
  2014-12-10 12:03           ` Damien Lespiau
@ 2014-12-10 13:55             ` Daniel Vetter
  0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2014-12-10 13:55 UTC (permalink / raw)
  To: Damien Lespiau; +Cc: intel-gfx

On Wed, Dec 10, 2014 at 12:03:20PM +0000, Damien Lespiau wrote:
> On Wed, Dec 10, 2014 at 11:42:13AM +0200, Jani Nikula wrote:
> > Pushed to drm-intel-next-fixes, replacing the old version. Thanks for
> > the patch.
> > 
> > Now the question is, do we want [1] for 3.19 or 3.20?
> > 
> > [1] http://mid.gmane.org/1418060138-5004-1-git-send-email-damien.lespiau@intel.com
> 
> I think that one is for 3.20, not really fixing a bug.

I suggested 3.19/dinf because it'll make merging easier. And it's
expressingly ok for Linus to merge cleanup stuff even in the merge
window/-rc1 if it makes things easier (as long as it's just obvious
refactoring without any functional change).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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] 20+ messages in thread
end of thread, other threads:[~2014-12-10 13:54 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-06 20:14 [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode Damien Lespiau
2014-12-07  4:48 ` shuang.he
2014-12-08 12:33 ` Jani Nikula
2014-12-08 13:59   ` Damien Lespiau
2014-12-08 14:17     ` Dave Gordon
2014-12-08 14:36       ` Damien Lespiau
2014-12-08 14:21     ` Daniel Vetter
2014-12-08 14:23       ` Daniel Vetter
2014-12-08 14:46         ` Damien Lespiau
2014-12-08 16:22   ` [PATCH v2] " Damien Lespiau
2014-12-08 16:27     ` Daniel Vetter
2014-12-08 16:50       ` Dave Gordon
2014-12-08 16:54         ` Damien Lespiau
2014-12-08 16:56     ` Dave Gordon
2014-12-08 17:33       ` [PATCH v3] " Damien Lespiau
2014-12-09 22:14         ` shuang.he
2014-12-10  9:42         ` Jani Nikula
2014-12-10 12:03           ` Damien Lespiau
2014-12-10 13:55             ` Daniel Vetter
2014-12-09 19:14     ` [PATCH v2] " shuang.he

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