| Message ID | 1417896857-26021-1-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 364/364 364/364
ILK +1-1 364/366 364/366
SNB 448/450 448/450
IVB 497/498 497/498
BYT 289/289 289/289
HSW 563/564 563/564
BDW 417/417 417/417
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt_kms_flip_plain-flip-fb-recreate-interruptible PASS(2, M26) DMESG_WARN(1, M26)
ILK igt_kms_flip_wf_vblank-ts-check DMESG_WARN(1, M26)PASS(1, M26) PASS(1, M26)
Note: You need to pay more attention to line start with '*'
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
On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote: > > #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...? ;) That's because we're not just enabling or disabling bits here but setting a multi-bits value. _MASKED_FIELD(2 << 4, 0x3 << 4);
On 08/12/14 13:59, Damien Lespiau wrote: > On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote: >>> #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...? ;) > > That's because we're not just enabling or disabling bits here but > setting a multi-bits value. > > _MASKED_FIELD(2 << 4, 0x3 << 4); > So you could #define __MASKED_BIT_DISABLE(a) (__MASKED_FIELD(0, (a))) #define __MASKED_BIT_ENABLE(a) (__MASKED_FIELD((a), (a))) which I think is what Jani was referring to ... Bikeshed++: do we care about the double evaluation of (a) in these macros? .Dave.
On Mon, Dec 08, 2014 at 01:59:50PM +0000, Damien Lespiau wrote: > On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote: > > > #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...? ;) > > That's because we're not just enabling or disabling bits here but > setting a multi-bits value. > > _MASKED_FIELD(2 << 4, 0x3 << 4); #define _MASKED_BIT_ENABLE(a) _MASKED_FIELD(a, a) #define _MASKED_BIT_DISABLE(a) _MASKED_FIELD(a, 0) is what I guess Jani thought of. -Daniel
On Mon, Dec 08, 2014 at 03:21:02PM +0100, Daniel Vetter wrote: > On Mon, Dec 08, 2014 at 01:59:50PM +0000, Damien Lespiau wrote: > > On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote: > > > > #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...? ;) > > > > That's because we're not just enabling or disabling bits here but > > setting a multi-bits value. > > > > _MASKED_FIELD(2 << 4, 0x3 << 4); > > #define _MASKED_BIT_ENABLE(a) _MASKED_FIELD(a, a) > #define _MASKED_BIT_DISABLE(a) _MASKED_FIELD(a, 0) Ok and I right away screwed up the argument ordering in the DISABLE one because the bits we set are before the mask. All the bitmasking functions we have in e.g. i915_irq.c ilk_update_gt_irq so for consistency I think we should flip it in this one here, too. Otherwise that bit of inconsistency will trip up tons of people in the future. Jani, can you please apply that fixup if Damien acks it? -Daniel
On Mon, Dec 08, 2014 at 02:17:45PM +0000, Dave Gordon wrote: > On 08/12/14 13:59, Damien Lespiau wrote: > > On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote: > >>> #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...? ;) > > > > That's because we're not just enabling or disabling bits here but > > setting a multi-bits value. > > > > _MASKED_FIELD(2 << 4, 0x3 << 4); > > > > So you could > > #define __MASKED_BIT_DISABLE(a) (__MASKED_FIELD(0, (a))) > #define __MASKED_BIT_ENABLE(a) (__MASKED_FIELD((a), (a))) > > which I think is what Jani was referring to ... > > Bikeshed++: do we care about the double evaluation of (a) in these macros? Oh of course, misread that. We could also avoid the double evaluation indeed and chris suggests (on IRC) BUILD_BUG_ON(bit & ~mask); (super good!)
On Mon, Dec 08, 2014 at 03:23:49PM +0100, Daniel Vetter wrote: > > #define _MASKED_BIT_ENABLE(a) _MASKED_FIELD(a, a) > > #define _MASKED_BIT_DISABLE(a) _MASKED_FIELD(a, 0) > > Ok and I right away screwed up the argument ordering in the DISABLE one > because the bits we set are before the mask. All the bitmasking functions > we have in e.g. i915_irq.c ilk_update_gt_irq so for consistency I think we > should flip it in this one here, too. Otherwise that bit of inconsistency > will trip up tons of people in the future. > > Jani, can you please apply that fixup if Damien acks it? (for the record ack'ed)
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)) /* 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; }
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> --- 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(-)