[1/4] drm/i915: Fix GEN8_MCR_SELECTOR programming
diff mbox series

Message ID 20190709210620.15805-2-tvrtko.ursulin@linux.intel.com
State New
Headers show
Series
  • MCR fixes
Related show

Commit Message

Tvrtko Ursulin July 9, 2019, 9:06 p.m. UTC
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")
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Chris Wilson July 9, 2019, 9:09 p.m. UTC | #1
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
Tvrtko Ursulin July 10, 2019, 6:21 a.m. UTC | #2
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
Chris Wilson July 10, 2019, 6:39 a.m. UTC | #3
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

Patch
diff mbox series

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) |