Re: [PATCH 1/4] drm/i915: Support flags in whitlist WAs

From: John Harrison <John.C.Harrison@Intel.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	Intel-GFX@Lists.FreeDesktop.Org
Subject: Re: [PATCH 1/4] drm/i915: Support flags in whitlist WAs
Date: Tue, 18 Jun 2019 06:48:07 -0700	[thread overview]
Message-ID: <c9f0fbd4-35df-573b-1de4-464da35e7f29@Intel.com> (raw)
In-Reply-To: <9f85944a-24c7-7409-fdd1-9c06eea04dbf@linux.intel.com>

On 6/17/2019 23:35, Tvrtko Ursulin wrote:
> On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Newer hardware adds flags to the whitelist work-around register. These
>> allow per access direction privileges and ranges.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
>>   drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
>> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index 165b0a45e009..ae82340fff45 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct 
>> drm_i915_private *i915,
>>   }
>>     static void
>> -whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>> +whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>>   {
>>       struct i915_wa wa = {
>>           .reg = reg
>> @@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, 
>> i915_reg_t reg)
>>       if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
>>           return;
>
> Actually how about we add somewhere around here:
>
> GEM_BUG_ON(hweight32(flags & (..RD | .. WR)) > 1);
>
> To ensure correct usage of the flags?
>

It should probably be more like BUG_ON((flags & ACCESS_MASK) > 
ACCESS_MAX). It is intended to be an access enum with three valid values 
rather than a pair of flags. But yes, such a check could be added in the 
next version of the patch series along with the selftest updates.

John.

> Regards,
>
> Tvrtko
>
>> +    wa.reg.reg |= flags;
>>       _wa_add(wal, &wa);
>>   }
>>   +static void
>> +whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>> +{
>> +    whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
>> +}
>> +
>>   static void gen9_whitelist_build(struct i915_wa_list *w)
>>   {
>>       /* 
>> WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 7a26766ba84d..cc295a4f6e92 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2513,6 +2513,13 @@ enum i915_power_well_id {
>>   #define   RING_WAIT_SEMAPHORE    (1 << 10) /* gen6+ */
>>     #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + 
>> (i) * 4)
>> +#define   RING_FORCE_TO_NONPRIV_RW        (0 << 28)    /* CFL+ & 
>> Gen11+ */
>> +#define   RING_FORCE_TO_NONPRIV_RD        (1 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_WR        (2 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_1        (0 << 0)     /* CFL+ 
>> & Gen11+ */
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_4        (1 << 0)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_16    (2 << 0)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_64    (3 << 0)
>>   #define   RING_MAX_NONPRIV_SLOTS  12
>>     #define GEN7_TLB_RD_ADDR    _MMIO(0x4700)
>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx