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

[Oscar Mateo]

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 at intel.com>
>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>> Cc: Mika Kuoppala <mika.kuoppala at 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?

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