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@xxxxxxxxx>
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> >
> > <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.
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
