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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> Cc: Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx>
> Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> ---
> 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@xxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
