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

[Oscar Mateo]

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
* Wa #1181 needs HAS_PCH_CNP(dev_priv)
* ...

We even have a WA (WaTempDisableDOPClkGating) where the new design is 
not dynamic enough :(

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

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

-- Oscar