diff mbox series

[1/6] drm/i915: Fix GEN8_MCR_SELECTOR programming

Message ID 20190717180624.20354-2-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series MCR fixes and more | expand

Commit Message

Tvrtko Ursulin July 17, 2019, 6: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")
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(-)

Comments

Summers, Stuart July 17, 2019, 9:25 p.m. UTC | #1 
On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Not opposed to this exactly, but do we really need this patch if we're
just getting rid of this routine later in the series?

Thanks,
Stuart

> 
> 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")
> 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 c0bc9cb7f228..6f93caf7a5a1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -962,9 +962,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) |
Tvrtko Ursulin July 18, 2019, 5:58 a.m. UTC | #2 
On 17/07/2019 22:25, Summers, Stuart wrote:
> On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Not opposed to this exactly, but do we really need this patch if we're
> just getting rid of this routine later in the series?

It just happens this fix alone is enough to fix MCR on all ICL parts I 
have seen (what CI has in BAT and in shards) and being a small one it is 
realistic to backport it to stable kernels. Sounds reasonable or I have 
missed something?

Regards,

Tvrtko

> Thanks,
> Stuart
> 
>>
>> 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")
>> 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 c0bc9cb7f228..6f93caf7a5a1 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> @@ -962,9 +962,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) |
Summers, Stuart July 19, 2019, 9:39 p.m. UTC | #3 
On Thu, 2019-07-18 at 06:58 +0100, Tvrtko Ursulin wrote:
> On 17/07/2019 22:25, Summers, Stuart wrote:
> > On Wed, 2019-07-17 at 19:06 +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Not opposed to this exactly, but do we really need this patch if
> > we're
> > just getting rid of this routine later in the series?
> 
> It just happens this fix alone is enough to fix MCR on all ICL parts
> I 
> have seen (what CI has in BAT and in shards) and being a small one it
> is 
> realistic to backport it to stable kernels. Sounds reasonable or I
> have 
> missed something?

Sorry for the delayed response here. Yes that sounds reasonable to me
too, thanks for the explanation.

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

Thanks,
Stuart

> 
> Regards,
> 
> Tvrtko
> 
> > Thanks,
> > Stuart
> > 
> > > 
> > > 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")
> > > 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 c0bc9cb7f228..6f93caf7a5a1 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > @@ -962,9 +962,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) |
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index c0bc9cb7f228..6f93caf7a5a1 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -962,9 +962,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) |