[Intel-gfx] [PATCH] drm/i915/cnl: Avoid WA_SET_BIT_MASK on write-only CNL_HDC_CHICKEN0.

[Oscar Mateo]

On 09/27/2017 02:17 PM, Rodrigo Vivi wrote:
> On Wed, Sep 27, 2017 at 09:08:10PM +0000, Oscar Mateo wrote:
>>
>> On 09/27/2017 02:01 PM, Rodrigo Vivi wrote:
>>> On CNL, HDC_CHICKEN0 "is write-only from LRI command.
>>> However, it is readable for context save."
>>>
>>> So we have no ways to check the coherency by reading it back on
>>> our tests.
>>>
>>> So let's just write that bit directly without saving it to
>>> dev_priv->workarounds.
>> Wait, it's not that simple: CNL_HDC_CHICKEN0 lives in the context image, so
>> you are going to revert the value as soon as you restore the next context
>> (you want to save it in dev_priv->workarounds so that
>> intel_ring_workarounds_emit applies it to every newly created context).
> hm... that makes sense...
>
> I was only with the WA description in my head that states to set only once at boot:
>
> " To avoid a potential hang condition with TLB invalidation
> driver should enable masked bit 5 of MMIO 0xE5F0 at boot."
>
> But apparently just a bad description in spec, right?!
>
> If this is the case probably dev_priv->workarouds will need another component
> "permission" to handle write-only registers and avoid reading that on debugfs?
> Or any thing smarter?
>

Well, to begin with, the current debugfs interface does not make a lot 
of sense for things that live inside the context image. That's why Chris 
sent this patch earlier:

"igt/gem_workarounds: Read the workaround registers from the active context"

But this depends on LRI commands and thus makes it impossible to work 
with this particular register :/

Maybe create a context, force the HW to switch in & out of it and then a 
debugfs interface to read the WAs from inside the context image? Another 
possibility: in CNL+, the good old MI_SET_CONTEXT command is back, so 
maybe it can be used imaginatively to retrieve the context image :)

In any case, this looks like a lot of work only to debug that one WA is 
applied correctly...

>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102943
>>> Fixes: acfb5554c769 ("drm/i915/cnl: WaForceContextSaveRestoreNonCoherent")
>>> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> index a28e2a864cf1..047ba0a0308d 100644
>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> @@ -1251,8 +1251,8 @@ static int cnl_init_workarounds(struct intel_engine_cs *engine)
>>>    			    GAMT_CHKN_DISABLE_I2M_CYCLE_ON_WR_PORT));
>>>    	/* WaForceContextSaveRestoreNonCoherent:cnl */
>>> -	WA_SET_BIT_MASKED(CNL_HDC_CHICKEN0,
>>> -			  HDC_FORCE_CONTEXT_SAVE_RESTORE_NON_COHERENT);
>>> +	I915_WRITE(CNL_HDC_CHICKEN0,
>>> +		   _MASKED_BIT_ENABLE(HDC_FORCE_CONTEXT_SAVE_RESTORE_NON_COHERENT));
>>>    	/* WaThrottleEUPerfToAvoidTDBackPressure:cnl(pre-prod) */
>>>    	if (IS_CNL_REVID(dev_priv, CNL_REVID_B0, CNL_REVID_B0))