| Message ID | 20190528200655.11605-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix off-by-one in looking up icl sseu slice | expand |
On Tue, 2019-05-28 at 21:06 +0100, Chris Wilson wrote: > We want the index corresponding to the set bit but fls() returns the > 1-index value. > > Otherwise, we trigger the sanitycheck > intel_sseu_get_subslices:46 GEM_BUG_ON(slice >= sseu- > >max_slices) > when we look up the invalid slice. > > The only remaining question then is just how reliable the rest of > intel_calculate_mcr_s_ss_select() is -- how many more of those fls() > are > also off-by-one. > > Fixes: 1ac159e23c2c ("drm/i915: Expand subslice mask") > Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement > WaProgramMgsrForCorrectSliceSpecificMmioReads") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Stuart Summers <stuart.summers@intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index fbc853085809..485cd1c8ecc4 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -781,7 +781,7 @@ wa_init_mcr(struct drm_i915_private *i915, struct > i915_wa_list *wal) > * read FUSE3 for enabled L3 Bank IDs, if L3 Bank > matches > * enabled subslice, no need to redirect MCR packet > */ > - u32 slice = fls(sseu->slice_mask); > + u32 slice = __fls(sseu->slice_mask); The condition around this (is_power_of_2) makes sure we meet the case where the slice_mask is uninitialized. This is going to work here, but might not work in all other places. If we propagate this change to the other places we call fls(slice_mask), which I'd recommend, we'll want to make sure we check for that. Once we show results in CI: Reviewed-by: Stuart Summers <stuart.summers@intel.com> > u32 fuse3 = > intel_uncore_read(&i915->uncore, > GEN10_MIRROR_FUSE3); > u32 ss_mask = intel_sseu_get_subslices(sseu, slice);
Quoting Summers, Stuart (2019-05-28 21:45:05) > On Tue, 2019-05-28 at 21:06 +0100, Chris Wilson wrote: > > We want the index corresponding to the set bit but fls() returns the > > 1-index value. > > > > Otherwise, we trigger the sanitycheck > > intel_sseu_get_subslices:46 GEM_BUG_ON(slice >= sseu- > > >max_slices) > > when we look up the invalid slice. > > > > The only remaining question then is just how reliable the rest of > > intel_calculate_mcr_s_ss_select() is -- how many more of those fls() > > are > > also off-by-one. > > > > Fixes: 1ac159e23c2c ("drm/i915: Expand subslice mask") > > Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement > > WaProgramMgsrForCorrectSliceSpecificMmioReads") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > Cc: Stuart Summers <stuart.summers@intel.com> > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > index fbc853085809..485cd1c8ecc4 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > @@ -781,7 +781,7 @@ wa_init_mcr(struct drm_i915_private *i915, struct > > i915_wa_list *wal) > > * read FUSE3 for enabled L3 Bank IDs, if L3 Bank > > matches > > * enabled subslice, no need to redirect MCR packet > > */ > > - u32 slice = fls(sseu->slice_mask); > > + u32 slice = __fls(sseu->slice_mask); > > The condition around this (is_power_of_2) makes sure we meet the case > where the slice_mask is uninitialized. This is going to work here, but > might not work in all other places. If we propagate this change to the > other places we call fls(slice_mask), which I'd recommend, we'll want > to make sure we check for that. > > Once we show results in CI: > Reviewed-by: Stuart Summers <stuart.summers@intel.com> This brought icl back from the dead. The other fls can be fixed up at leisure! Ta, -Chris
Quoting Chris Wilson (2019-05-28 23:03:16) > Quoting Summers, Stuart (2019-05-28 21:45:05) > > On Tue, 2019-05-28 at 21:06 +0100, Chris Wilson wrote: > > > We want the index corresponding to the set bit but fls() returns the > > > 1-index value. > > > > > > Otherwise, we trigger the sanitycheck > > > intel_sseu_get_subslices:46 GEM_BUG_ON(slice >= sseu- > > > >max_slices) > > > when we look up the invalid slice. > > > > > > The only remaining question then is just how reliable the rest of > > > intel_calculate_mcr_s_ss_select() is -- how many more of those fls() > > > are > > > also off-by-one. > > > > > > Fixes: 1ac159e23c2c ("drm/i915: Expand subslice mask") > > > Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement > > > WaProgramMgsrForCorrectSliceSpecificMmioReads") > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > > Cc: Stuart Summers <stuart.summers@intel.com> > > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > index fbc853085809..485cd1c8ecc4 100644 > > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > @@ -781,7 +781,7 @@ wa_init_mcr(struct drm_i915_private *i915, struct > > > i915_wa_list *wal) > > > * read FUSE3 for enabled L3 Bank IDs, if L3 Bank > > > matches > > > * enabled subslice, no need to redirect MCR packet > > > */ > > > - u32 slice = fls(sseu->slice_mask); > > > + u32 slice = __fls(sseu->slice_mask); > > > > The condition around this (is_power_of_2) makes sure we meet the case > > where the slice_mask is uninitialized. This is going to work here, but > > might not work in all other places. If we propagate this change to the > > other places we call fls(slice_mask), which I'd recommend, we'll want > > to make sure we check for that. > > > > Once we show results in CI: > > Reviewed-by: Stuart Summers <stuart.summers@intel.com> > > This brought icl back from the dead. The other fls can be fixed up at > leisure! Ta, Only for it to die at <4>[ 12.083315] WARN_ON((enabled_mask & disabled_mask) != enabled_mask) <4>[ 12.083370] WARNING: CPU: 7 PID: 387 at drivers/gpu/drm/i915/gt/intel_workarounds.c:797 wa_init_mcr+0xfa/0x110 [i915] Onwards, -Chris
On Tue, 2019-05-28 at 23:05 +0100, Chris Wilson wrote: > Quoting Chris Wilson (2019-05-28 23:03:16) > > Quoting Summers, Stuart (2019-05-28 21:45:05) > > > On Tue, 2019-05-28 at 21:06 +0100, Chris Wilson wrote: > > > > We want the index corresponding to the set bit but fls() > > > > returns the > > > > 1-index value. > > > > > > > > Otherwise, we trigger the sanitycheck > > > > intel_sseu_get_subslices:46 GEM_BUG_ON(slice >= sseu- > > > > > max_slices) > > > > > > > > when we look up the invalid slice. > > > > > > > > The only remaining question then is just how reliable the rest > > > > of > > > > intel_calculate_mcr_s_ss_select() is -- how many more of those > > > > fls() > > > > are > > > > also off-by-one. > > > > > > > > Fixes: 1ac159e23c2c ("drm/i915: Expand subslice mask") > > > > Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement > > > > WaProgramMgsrForCorrectSliceSpecificMmioReads") > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > > > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > > > Cc: Stuart Summers <stuart.summers@intel.com> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > > index fbc853085809..485cd1c8ecc4 100644 > > > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c(enabled_mask > > > > & disabled_mask) != enabled_mask > > > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > > > @@ -781,7 +781,7 @@ wa_init_mcr(struct drm_i915_private *i915, > > > > struct > > > > i915_wa_list *wal) > > > > * read FUSE3 for enabled L3 Bank IDs, if L3 Bank > > > > matches > > > > * enabled subslice, no need to redirect MCR > > > > packet > > > > */ > > > > - u32 slice = fls(sseu->slice_mask); > > > > + u32 slice = __fls(sseu->slice_mask); > > > > > > The condition around this (is_power_of_2) makes sure we meet the > > > case > > > where the slice_mask is uninitialized. This is going to work > > > here, but > > > might not work in all other places. If we propagate this change > > > to the > > > other places we call fls(slice_mask), which I'd recommend, we'll > > > want > > > to make sure we check for that. > > > > > > Once we show results in CI: > > > Reviewed-by: Stuart Summers <stuart.summers@intel.com> > > > > This brought icl back from the dead. The other fls can be fixed up > > at > > leisure! Ta, > > Only for it to die at > <4>[ 12.083315] WARN_ON((enabled_mask & disabled_mask) != > enabled_mask) > <4>[ 12.083370] WARNING: CPU: 7 PID: 387 at > drivers/gpu/drm/i915/gt/intel_workarounds.c:797 > wa_init_mcr+0xfa/0x110 [i915] I'll also take a closer look here. I see the warning in the CI logs too. I'm not sure why this didn't come up in my local testing. -Stuart > > Onwards, > -Chris
On Tue, 28 May 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote: > We want the index corresponding to the set bit but fls() returns the > 1-index value. > > Otherwise, we trigger the sanitycheck > intel_sseu_get_subslices:46 GEM_BUG_ON(slice >= sseu->max_slices) > when we look up the invalid slice. > > The only remaining question then is just how reliable the rest of > intel_calculate_mcr_s_ss_select() is -- how many more of those fls() are > also off-by-one. > > Fixes: 1ac159e23c2c ("drm/i915: Expand subslice mask") I sent a revert of this commit [1] for reasons explained in the commit message. BR, Jani. [1] http://patchwork.freedesktop.org/patch/msgid/20190529082150.31526-1-jani.nikula@intel.com > Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Stuart Summers <stuart.summers@intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index fbc853085809..485cd1c8ecc4 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -781,7 +781,7 @@ wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal) > * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches > * enabled subslice, no need to redirect MCR packet > */ > - u32 slice = fls(sseu->slice_mask); > + u32 slice = __fls(sseu->slice_mask); > u32 fuse3 = > intel_uncore_read(&i915->uncore, GEN10_MIRROR_FUSE3); > u32 ss_mask = intel_sseu_get_subslices(sseu, slice);
On Wed, 29 May 2019, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 28 May 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> We want the index corresponding to the set bit but fls() returns the >> 1-index value. >> >> Otherwise, we trigger the sanitycheck >> intel_sseu_get_subslices:46 GEM_BUG_ON(slice >= sseu->max_slices) >> when we look up the invalid slice. >> >> The only remaining question then is just how reliable the rest of >> intel_calculate_mcr_s_ss_select() is -- how many more of those fls() are >> also off-by-one. >> >> Fixes: 1ac159e23c2c ("drm/i915: Expand subslice mask") > > I sent a revert of this commit [1] for reasons explained in the commit > message. I've gone ahead and pushed the revert. We can now calmly get back to the drawing board, and get this sorted out. BR, Jani. > > BR, > Jani. > > > [1] http://patchwork.freedesktop.org/patch/msgid/20190529082150.31526-1-jani.nikula@intel.com > > >> Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads") >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> Cc: Stuart Summers <stuart.summers@intel.com> >> Cc: Manasi Navare <manasi.d.navare@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c >> index fbc853085809..485cd1c8ecc4 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c >> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c >> @@ -781,7 +781,7 @@ wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal) >> * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches >> * enabled subslice, no need to redirect MCR packet >> */ >> - u32 slice = fls(sseu->slice_mask); >> + u32 slice = __fls(sseu->slice_mask); >> u32 fuse3 = >> intel_uncore_read(&i915->uncore, GEN10_MIRROR_FUSE3); >> u32 ss_mask = intel_sseu_get_subslices(sseu, slice);
HI, > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani > Nikula > Sent: keskiviikko 29. toukokuuta 2019 17.23 > To: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix off-by-one in looking up icl sseu slice > > On Wed, 29 May 2019, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Tue, 28 May 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> We want the index corresponding to the set bit but fls() returns the > >> 1-index value. > >> > >> Otherwise, we trigger the sanitycheck > >> intel_sseu_get_subslices:46 GEM_BUG_ON(slice >= sseu->max_slices) > >> when we look up the invalid slice. > >> > >> The only remaining question then is just how reliable the rest of > >> intel_calculate_mcr_s_ss_select() is -- how many more of those fls() > >> are also off-by-one. > >> > >> Fixes: 1ac159e23c2c ("drm/i915: Expand subslice mask") > > > > I sent a revert of this commit [1] for reasons explained in the commit > > message. > > I've gone ahead and pushed the revert. > > We can now calmly get back to the drawing board, and get this sorted out. Ack ;) > > BR, > Jani. > > > > > > BR, > > Jani. > > > > > > [1] > > http://patchwork.freedesktop.org/patch/msgid/20190529082150.31526-1-ja > > ni.nikula@intel.com > > > > > >> Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement > >> WaProgramMgsrForCorrectSliceSpecificMmioReads") > >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > >> Cc: Stuart Summers <stuart.summers@intel.com> > >> Cc: Manasi Navare <manasi.d.navare@intel.com> > >> --- > >> drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > >> b/drivers/gpu/drm/i915/gt/intel_workarounds.c > >> index fbc853085809..485cd1c8ecc4 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > >> @@ -781,7 +781,7 @@ wa_init_mcr(struct drm_i915_private *i915, struct > i915_wa_list *wal) > >> * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches > >> * enabled subslice, no need to redirect MCR packet > >> */ > >> - u32 slice = fls(sseu->slice_mask); > >> + u32 slice = __fls(sseu->slice_mask); > >> u32 fuse3 = > >> intel_uncore_read(&i915->uncore, > GEN10_MIRROR_FUSE3); > >> u32 ss_mask = intel_sseu_get_subslices(sseu, slice); > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c index fbc853085809..485cd1c8ecc4 100644 --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c @@ -781,7 +781,7 @@ wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal) * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches * enabled subslice, no need to redirect MCR packet */ - u32 slice = fls(sseu->slice_mask); + u32 slice = __fls(sseu->slice_mask); u32 fuse3 = intel_uncore_read(&i915->uncore, GEN10_MIRROR_FUSE3); u32 ss_mask = intel_sseu_get_subslices(sseu, slice);
We want the index corresponding to the set bit but fls() returns the 1-index value. Otherwise, we trigger the sanitycheck intel_sseu_get_subslices:46 GEM_BUG_ON(slice >= sseu->max_slices) when we look up the invalid slice. The only remaining question then is just how reliable the rest of intel_calculate_mcr_s_ss_select() is -- how many more of those fls() are also off-by-one. Fixes: 1ac159e23c2c ("drm/i915: Expand subslice mask") Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Cc: Stuart Summers <stuart.summers@intel.com> Cc: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)