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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 18/06/2019 02:13, John Harrison wrote:
> On 6/13/2019 23:48, Tvrtko Ursulin wrote:
>>
>> On 14/06/2019 01:28, Robert M. Fosha 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;
>>>   +    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 edf9f93934a1..10fea5ab3fc3 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+ */
>>
>> Shouldn't this comment be against the RD and WR flags, while the above 
>> is the legacy one?
> 
> Technically, this field does not exist at all on older hardware. However 
> these bits are MBZ hence are equivalent to having the value of RW being 
> set. To me, it makes more sense to be accurate and label the entire 
> field as newer hardware only.

Did not see your reply initially due not being copied.

Okay then I guess. Just going forward make sure the patches for old 
workarounds don't use RING_FORCE_TO_NONPRIV_RW like the previous round 
did. (Can't have it both ways. ;)

Regards,

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