[Intel-gfx] [PATCH v2 4/4] drm/i915/gen9: Add WaSetDisablePixMaskCammingAndRhwoInCommonSliceChicken

[Mika Kuoppala]

Hi Dave,

Dave Gordon <david.s.gordon at intel.com> writes:

> On 13/07/15 14:16, Mika Kuoppala wrote:
>> Arun Siluvery <arun.siluvery at linux.intel.com> writes:
>>
>>> In Indirect context w/a batch buffer,
>>> +WaSetDisablePixMaskCammingAndRhwoInCommonSliceChicken
>>>
>>> v2: SKL revision id was used for BXT, copy paste error found during
>>> internal review (Bob Beckett).
>>>
>>> Cc: Robert Beckett <robert.beckett at intel.com>
>>> Cc: Imre Deak <imre.deak at intel.com>
>>> Signed-off-by: Arun Siluvery <arun.siluvery at linux.intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_lrc.c        | 9 +++++++++
>>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++--
>>>   2 files changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 58247f0..61ed92b 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -1302,6 +1302,15 @@ static int gen9_init_perctx_bb(struct intel_engine_cs *ring,
>>>   	struct drm_device *dev = ring->dev;
>>>   	uint32_t index = wa_ctx_start(wa_ctx, *offset, CACHELINE_DWORDS);
>>>
>>> +	/* WaSetDisablePixMaskCammingAndRhwoInCommonSliceChicken:skl,bxt */
>>> +	if ((IS_SKYLAKE(dev) && (INTEL_REVID(dev) <= SKL_REVID_B0)) ||
>>> +	    (IS_BROXTON(dev) && (INTEL_REVID(dev) == BXT_REVID_A0))) {
>>> +		wa_ctx_emit(batch, MI_LOAD_REGISTER_IMM(1));
>>> +		wa_ctx_emit(batch, GEN9_SLICE_COMMON_ECO_CHICKEN0);
>>> +		wa_ctx_emit(batch, _MASKED_BIT_ENABLE(DISABLE_PIXEL_MASK_CAMMING));
>>> +		wa_ctx_emit(batch, MI_NOOP);
>>> +	}
>>> +
>>>   	/* WaDisableCtxRestoreArbitration:skl,bxt */
>>>   	if ((IS_SKYLAKE(dev) && (INTEL_REVID(dev) <= SKL_REVID_D0)) ||
>>>   	    (IS_BROXTON(dev) && (INTEL_REVID(dev) == BXT_REVID_A0)))
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> index af7c12e..66dde7f 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> @@ -946,8 +946,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
>>>   		/* WaSetDisablePixMaskCammingAndRhwoInCommonSliceChicken:skl,bxt */
>>>   		WA_SET_BIT_MASKED(GEN7_COMMON_SLICE_CHICKEN1,
>>>   				  GEN9_RHWO_OPTIMIZATION_DISABLE);
>>> -		WA_SET_BIT_MASKED(GEN9_SLICE_COMMON_ECO_CHICKEN0,
>>> -				  DISABLE_PIXEL_MASK_CAMMING);
>>> +		/*
>>> +		 * WA also requires GEN9_SLICE_COMMON_ECO_CHICKEN0[14] to be set,
>>> +		 * but that register is write only hence it is set
>>> +		 * in per ctx batch buffer
>>
>> Why the need to move to per ctx bb? Why the write would not stick
>> with this? Is the rationale that we get fails with the gem_workarounds?
>>
>> If so, perhaps we need a write_only marker for workaround list?
>>
>> -Mika
>
> Surely it's the fact that it's a context-saved register that matters, 
> rather than it being write-only (generally we treat masked registers as 
> write-only anyway, since the masking means that you can set or clear a 
> bit without having to read the other bits first).
>

But with workaround list, we push these writes into the ringbuffer
of the context being created. So I still fail to see how they
would end up being out of context.

The per ctx bb would make sense if this register was not part of
contexts and thus would not be saved. Is this the case here?

-Mika

> Whereas AFAICS GEN7_COMMON_SLICE_CHICKEN1 is a global non-context 
> register, so it can be set directly rather than having to be done in a 
> batchbuffer.
>
> .Dave.