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

[Damien Lespiau]

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