[Intel-gfx] [PATCH] drm/i915/gen11: use ffs for minconfig slice/subslice

[Matt Roper]

On Sat, Jun 12, 2021 at 09:55:02AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Sent: 11 June 2021 23:36
> > To: Surendrakumar Upadhyay, TejaskumarX
> > <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gen11: use ffs for minconfig
> > slice/subslice
> > 
> > On Fri, Jun 11, 2021 at 08:04:09PM +0530, Tejas Upadhyay wrote:
> > > w/a on gen11 platforms not working as expected and causing more harm
> > > on the RC6 flow. Because of subslice steering disturbance w/a read is
> > > failing. By using ffs we can default steering of slice/sublice to
> > > minconfig hence w/a will pass and any warns will go away.
> > >
> > > Fixes: fb899dd8ea9c ("drm/i915: Apply Wa_1406680159:icl,ehl as an
> > > engine workaround")
> > > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > Signed-off-by: Tejas Upadhyay
> > > <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 14 +++++++++++---
> > >  drivers/gpu/drm/i915/intel_pm.c             | 10 +++++++---
> > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index b62d1e31a645..68b14141088a 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -991,13 +991,21 @@ wa_init_mcr(struct drm_i915_private *i915,
> > struct i915_wa_list *wal)
> > >  		l3_en = ~0;
> > >  	}
> > >
> > > -	slice = fls(sseu->slice_mask) - 1;
> > > -	subslice = fls(l3_en & intel_sseu_get_subslices(sseu, slice));
> > > +	if (GRAPHICS_VER(i915) == 11) {
> > > +		slice = ffs(sseu->slice_mask) - 1;
> > > +		subslice = ffs(l3_en & intel_sseu_get_subslices(sseu, slice));
> > > +	} else {
> > > +		slice = fls(sseu->slice_mask) - 1;
> > > +		subslice = fls(l3_en & intel_sseu_get_subslices(sseu, slice));
> > > +	}
> > >  	if (!subslice) {
> > >  		drm_warn(&i915->drm,
> > >  			 "No common index found between subslice mask %x
> > and L3 bank mask %x!\n",
> > >  			 intel_sseu_get_subslices(sseu, slice), l3_en);
> > > -		subslice = fls(l3_en);
> > > +		if (GRAPHICS_VER(i915) == 11)
> > > +			subslice = ffs(l3_en);
> > > +		else
> > > +			subslice = fls(l3_en);
> > >  		drm_WARN_ON(&i915->drm, !subslice);
> > >  	}
> > >  	subslice--;
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c index 45fefa0ed160..d1352ec3546a
> > > 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4049,9 +4049,13 @@ skl_ddb_entry_for_slices(struct
> > drm_i915_private *dev_priv, u8 slice_mask,
> > >  		ddb->end = 0;
> > >  		return;
> > >  	}
> > > -
> > > -	ddb->start = (ffs(slice_mask) - 1) * slice_size;
> > > -	ddb->end = fls(slice_mask) * slice_size;
> > > +	if (GRAPHICS_VER(dev_priv) == 11) {
> > > +		ddb->start = (fls(slice_mask) - 1) * slice_size;
> > > +		ddb->end = ffs(slice_mask) * slice_size;
> > > +	} else {
> > > +		ddb->start = (ffs(slice_mask) - 1) * slice_size;
> > > +		ddb->end = fls(slice_mask) * slice_size;
> > > +	}
> > 
> > This code has nothing to do with GT slices.
> 
> Without this change we are observing "gem_exec_suspend (basic-s0)
> Starting subtest: basic-S0" test hangs and crash eventually. Thus
> change identified and added. Would you please help reviewing?
> 
> Also I am seeing ICL igt at kms_frontbuffer_tracking@fbc-suspend is
> seeing workaround(0x9524) lost warning after this patch while EHL and
> JSL are working fine. does someone has insight why that should be
>  the case? 

See the patch I sent last week:

        https://patchwork.freedesktop.org/series/91367/

and my response to the CI results here that explains the ICL behavior:

        https://lists.freedesktop.org/archives/intel-gfx/2021-June/269097.html

Basically the fact that we're trying to combine subslice steering and
l3bank steering into a single value prevents us from using the
minconfig, even after switching to ffs().  ICL has been broken all along
too and we just didn't notice because we were getting "lucky" and
reading back random garbage that happened to have a '1' in the relevant
bit.

The next platform that we're getting ready to start upstreaming soon
adds a bunch more types of multicast steering, so the right approach is
to extract some of the refactoring from that platform and apply it to
gen11+ l3bank ranges too.  Something along the lines of this series that
I sent to trybot:

        https://patchwork.freedesktop.org/series/91421/

although it looks like I've still got some bugs that need to be hammered
out before its ready.


Matt

> 
> Thanks,
> Tejas
> > 
> > >
> > >  	WARN_ON(ddb->start >= ddb->end);
> > >  	WARN_ON(ddb->end > INTEL_INFO(dev_priv)->dbuf.size);
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795