Re: [PATCH 06/11] drm/i915: Save all MMIO WAs and apply them at a later time

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On Fri, 2017-11-03 at 15:56 -0700, Oscar Mateo wrote:
> 
> On 10/16/2017 03:34 AM, Joonas Lahtinen wrote:
> > On Fri, 2017-10-13 at 13:49 -0700, Oscar Mateo wrote:
> > > On 10/12/2017 03:35 AM, Joonas Lahtinen wrote:
> > > > On Wed, 2017-10-11 at 11:15 -0700, Oscar Mateo wrote:
> > > > > By doing this, we can dump these workarounds in debugfs for
> > > > > validation (which,
> > > > > at the moment, we are only able to do for the contexts WAs).
> > > > > 
> > > > > v2:
> > > > >     - Wrong macro used for MMIO set bit masked
> > > > >     - Improved naming
> > > > >     - Rebased
> > > > > 
> > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > > > 
> > > > <SNIP>
> > > > 
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -1960,12 +1960,16 @@ struct i915_wa_reg {
> > > > >    	u32 mask;
> > > > >    };
> > > > >    
> > > > > -#define I915_MAX_WA_REGS 16
> > > > > +#define I915_MAX_CTX_WA_REGS 16
> > > > > +#define I915_MAX_MMIO_WA_REGS 32
> > > > >    
> > > > >    struct i915_workarounds {
> > > > > -	struct i915_wa_reg ctx_wa_reg[I915_MAX_WA_REGS];
> > > > > +	struct i915_wa_reg ctx_wa_reg[I915_MAX_CTX_WA_REGS];
> > > > >    	u32 ctx_wa_count;
> > > > >    
> > > > > +	struct i915_wa_reg mmio_wa_reg[I915_MAX_MMIO_WA_REGS];
> > > > > +	u32 mmio_wa_count;
> > > > > +
> > > > >    	u32 hw_whitelist_count[I915_NUM_ENGINES];
> > > > >    };
> > > > 
> > > > Could we instead consider a constant structure with platform bitmasks?
> > > > If that's not dynamic enough, then a bitmap which is initialized by the
> > > > platform bitmasks as a default. So instead of running code to add to
> > > > list, make it bit more declarative. Pseudo-code;
> > > > 
> > > > 
> > > > struct i915_workaround {
> > > > 	u16 gen_mask;
> > > > 	enum {
> > > > 		I915_WA_CTX,
> > > > 		I915_WA_MMIO,
> > > > 		I915_WA_WHITELIST,
> > > > 	} type;
> > > > 	u32 reg;
> > > > }
> > > > 
> > > > ... elsewhere in .c file
> > > > 
> > > > static const struct i915_workaround i915_workarounds[] = { {
> > > > 	/* WaSomethingSomewhereUMDFoo:skl */
> > > > 	.gen_mask = INTEL_GEN_MASK(X, Y),
> > > > 	.type = I915_WA_CTX,
> > > > 	.reg = ...
> > > > } };
> > > > 
> > > > Regards, Joonas
> > > 
> > > I considered it, but we have some workarounds that are even more dynamic
> > > than that. E.g.:
> > > 
> > > * WaCompressedResourceSamplerPbeMediaNewHashMode depends on
> > > HAS_LLC(dev_priv)
> > > * skl_tune_iz_hashing depends on number of subslices (although maybe
> > > this is not technically a WA?)
> > > * WaGttCachingOffByDefault needs HAS_PAGE_SIZES(dev_priv,
> > > I915_GTT_PAGE_SIZE_2M)
> > > * WaPsrDPRSUnmaskVBlankInSRD is applied for_each_pipe
> > 
> > Could be multiple Wa lines each with HAS_PIPE() condition.
> > 
> > > * Wa #1181 needs HAS_PCH_CNP(dev_priv)
> > > * ...
> > > 
> > > We even have a WA (WaTempDisableDOPClkGating) where the new design is
> > > not dynamic enough :(
> > 
> > I was thinking the array would cover the simple register writes, I
> > think most of the detection problems could be covered by a simple probe
> > function. One could consider caching the probing result to a bitmap.
> > 
> > > I guess we could add a callback pointer to the table for those WAs that
> > > need extra information. Maybe even a "pre" and a "post" callback
> > > pointers (to cover that pesky WaTempDisableDOPClkGating).
> > 
> > You'd still have to keep some state between pre and post. Maybe just
> > have I915_WA_FUNC and then a callback to apply the ones not fitting the
> > "detection" + "simple register write" scheme.
> > 
> 
> It took me some time, but I found the flaw in this design: I cannot use 
> a callback to apply the ones not fitting the"detection" + "simple 
> register write" scheme as you mention, because then debugfs will not 
> know about those (which was the point of this whole exercise).
> 
> I tried something like this:
> 
> typedef bool (* wa_pre_hook_func)(struct drm_i915_private *dev_priv,
>                    const struct i915_wa_reg *wa,
>                    u32 *mask, u32 *value, u32 *data);
> typedef void (* wa_post_hook_func)(struct drm_i915_private *dev_priv,
>                     const struct i915_wa_reg *wa,
>                     u32 data);
> 
> In case mask/value need to be recalculated (e.g. skl_tune_iz_hashing or 
> WaGttCachingOffByDefault) but then I need to call the pre_hook on 
> debugfs, which I might not want to do in all cases (see 
> WaProgramL3SqcReg1Default).
> I can special-case the problematic WAs, or even left them out of the 
> array, but somehow that seems worse than what we already had...
> 
> Thoughts?

I'm not sure I see the issue. If looking at gen8_set_l3sqc_credits

You'd have the specialized apply() callback which sets the
GEN8_L3SQCREG1 register respecting the WaTempDisableDOPClkGating.

When you have a WA for applying a WA, I don't see how we can completely
avoid the specialized functions.

For debugfs, wouldn't you dump the value of what GEN8_L3SQCREG1 is for
a specific dev_priv? If there's more than that, I guess we can't but
store that state in dev_priv. And have an optional hook function to
fill in whatever depends on runtime and we want displayed in debugfs?

I think it might be best to start with moving all the WAs to a file,
and then continue looking at how we can best present them.

Regards, Joonas

> 
> > > If this is where we want to go, I can write a patch, but I believe it
> > > would be better to land this first (code review is critical for these
> > > kind of changes, and it is easier to first review that all WAs make it
> > > to i915_workarounds.c correctly, and then review that they are all
> > > transformed to a static table correctly).
> > 
> > First collecting the WA code to same source file is a good idea if that
> > can be made conveniently, but I would not transform them to arrays or
> > some other intermediate form like this patch proposes. Instead after
> > the initial code motion, convert straight to the agreed format.
> > 
> > This is a good clarification to the WAs, so I like the general idea.
> > 
> > Regards, Joonas
> 
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx