| Message ID | 20190709210620.15805-2-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MCR fixes | expand |
Quoting Tvrtko Ursulin (2019-07-09 22:06:17) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > fls returns bit positions starting from one for the lsb and the MCR > register expects zero based (sub)slice addressing. > > Incorrent MCR programming can have the effect of directing MMIO reads of > registers in the 0xb100-0xb3ff range to invalid subslice returning zeroes > instead of actual content. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads") Makes sense to me, just from my meagre understanding of arrays Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > index bdf279fa3b2e..ee15d1934486 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > @@ -975,9 +975,14 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) > u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private *dev_priv) > { > const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu; > + unsigned int slice = fls(sseu->slice_mask) - 1; I'd vote for __fls() here instead of fls() - 1. > + unsigned int subslice; > u32 mcr_s_ss_select; > - u32 slice = fls(sseu->slice_mask); > - u32 subslice = fls(sseu->subslice_mask[slice]); > + > + GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask)); > + subslice = fls(sseu->subslice_mask[slice]); > + GEM_BUG_ON(!subslice); > + subslice--; And I think we're a bit late on the BUG_ON here (it's shouldn't change after probing) so could be happily reduced to __fls(). -Chris
On 09/07/2019 22:09, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-07-09 22:06:17) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> fls returns bit positions starting from one for the lsb and the MCR >> register expects zero based (sub)slice addressing. >> >> Incorrent MCR programming can have the effect of directing MMIO reads of >> registers in the 0xb100-0xb3ff range to invalid subslice returning zeroes >> instead of actual content. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads") > > Makes sense to me, just from my meagre understanding of arrays > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > >> --- >> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >> index bdf279fa3b2e..ee15d1934486 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c >> @@ -975,9 +975,14 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) >> u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private *dev_priv) >> { >> const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu; >> + unsigned int slice = fls(sseu->slice_mask) - 1; > > I'd vote for __fls() here instead of fls() - 1. With fls() I get zero slice mask check for free, in the array out of bounds check below. > >> + unsigned int subslice; >> u32 mcr_s_ss_select; >> - u32 slice = fls(sseu->slice_mask); >> - u32 subslice = fls(sseu->subslice_mask[slice]); >> + >> + GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask)); >> + subslice = fls(sseu->subslice_mask[slice]); >> + GEM_BUG_ON(!subslice); >> + subslice--; > > And I think we're a bit late on the BUG_ON here (it's shouldn't change > after probing) so could be happily reduced to __fls(). Why late? This one is not checking the array for out of bounds, just if zero subslice mask happens to be in a valid slot. Too paranoid? Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-07-10 07:21:19) > > On 09/07/2019 22:09, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-07-09 22:06:17) > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >> fls returns bit positions starting from one for the lsb and the MCR > >> register expects zero based (sub)slice addressing. > >> > >> Incorrent MCR programming can have the effect of directing MMIO reads of > >> registers in the 0xb100-0xb3ff range to invalid subslice returning zeroes > >> instead of actual content. > >> > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> Fixes: 1e40d4aea57b ("drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads") > > > > Makes sense to me, just from my meagre understanding of arrays > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > >> --- > >> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > >> index bdf279fa3b2e..ee15d1934486 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > >> @@ -975,9 +975,14 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) > >> u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private *dev_priv) > >> { > >> const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu; > >> + unsigned int slice = fls(sseu->slice_mask) - 1; > > > > I'd vote for __fls() here instead of fls() - 1. > > With fls() I get zero slice mask check for free, in the array out of > bounds check below. > > > > >> + unsigned int subslice; > >> u32 mcr_s_ss_select; > >> - u32 slice = fls(sseu->slice_mask); > >> - u32 subslice = fls(sseu->subslice_mask[slice]); > >> + > >> + GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask)); > >> + subslice = fls(sseu->subslice_mask[slice]); > >> + GEM_BUG_ON(!subslice); > >> + subslice--; > > > > And I think we're a bit late on the BUG_ON here (it's shouldn't change > > after probing) so could be happily reduced to __fls(). > > Why late? This one is not checking the array for out of bounds, just if > zero subslice mask happens to be in a valid slot. Too paranoid? Just saying this won't change from setup where we should have validated our HW discovery. -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index bdf279fa3b2e..ee15d1934486 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -975,9 +975,14 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type) u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private *dev_priv) { const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu; + unsigned int slice = fls(sseu->slice_mask) - 1; + unsigned int subslice; u32 mcr_s_ss_select; - u32 slice = fls(sseu->slice_mask); - u32 subslice = fls(sseu->subslice_mask[slice]); + + GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask)); + subslice = fls(sseu->subslice_mask[slice]); + GEM_BUG_ON(!subslice); + subslice--; if (IS_GEN(dev_priv, 10)) mcr_s_ss_select = GEN8_MCR_SLICE(slice) |