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

[Dave Gordon <david.s.gordon@intel.com>]

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