On Mon, 08 Dec 2014, Damien Lespiau <damien.lespiau@xxxxxxxxx> 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) > > v3: Ensure the the value and mask are 16 bits (Dave Gordon) > > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> > Cc: Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx> > Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx> 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@xxxxxxxxx > --- > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
