Re: [PATCH] drm/i915/bdw: Fix the write setting up the WIZ hashing mode

[Jani Nikula <jani.nikula@linux.intel.com>]

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