Re: [Intel-gfx] [PATCH v2 3/6] drm/i915/gt: Fix context workarounds with non-masked regs

[Kenneth Graunke <kenneth@whitecape.org>]

[-- Attachment #1: Type: text/plain, Size: 4740 bytes --]

On Saturday, June 24, 2023 10:17:54 AM PDT Lucas De Marchi wrote:
> Most of the context workarounds tweak masked registers, but not all. For
> masked registers, when writing the value it's sufficient to just write
> the wa->set_bits since that will take care of both the clr and set bits
> as well as not overwriting other bits.
> 
> However there are some workarounds, the registers are non-masked. Up
> until now the driver was simply emitting a MI_LOAD_REGISTER_IMM with the
> set_bits to program the register via the GPU in the WA bb. This has the
> side effect of overwriting the content of the register outside of bits
> that should be set and also doesn't handle the bits that should be
> cleared.
> 
> Kenneth reported that on DG2, mesa was seeing a weird behavior due to
> the kernel programming of L3SQCREG5 in dg2_ctx_gt_tuning_init(). With
> the GPU idle, that register could be read via intel_reg as 0x00e001ff,
> but during a 3D workload it would change to 0x0000007f. So the
> programming of that tuning was affecting more than the bits in
> L3_PWM_TIMER_INIT_VAL_MASK. Matt Roper noticed the lack of rmw for the
> context workarounds due to the use of MI_LOAD_REGISTER_IMM.
> 
> So, for registers that are not masked, read its value via mmio, modify
> and then set it in the buffer to be written by the GPU. This should take
> care in a simple way of programming just the bits required by the
> tuning/workaround. If in future there are registers that involved that
> can't be read by the CPU, a more complex approach may be required like
> a) issuing additional instructions to read and modify; or b) scan the
> golden context and patch it in place before saving it; or something
> else. But for now this should suffice.
> 
> Scanning the context workarounds for all platforms, these are the
> impacted ones with the respective registers
> 
> 	mtl: DRAW_WATERMARK
> 	mtl/dg2: XEHP_L3SQCREG5, XEHP_FF_MODE2
> 
> ICL has some non-masked registers in the context workarounds:
> GEN8_L3CNTLREG, IVB_FBC_RT_BASE and VB_FBC_RT_BASE_UPPER, but there
> shouldn't be an impact. The first is already being manually read and the
> other 2 are intentionally overwriting the entire register. Same
> reasoning applies to GEN12_FF_MODE2: the WA is intentionally
> overwriting all the bits to avoid a read-modify-write.
> 
> v2:  Reword commit message wrt GEN12_FF_MODE2 and the changed behavior
> on preparatory patches.
> 
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Link: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23783#note_1968971
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 27 ++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 7d48bd57b6ef..9291c2b4ca0e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -986,6 +986,9 @@ void intel_engine_init_ctx_wa(struct intel_engine_cs *engine)
>  int intel_engine_emit_ctx_wa(struct i915_request *rq)
>  {
>  	struct i915_wa_list *wal = &rq->engine->ctx_wa_list;
> +	struct intel_uncore *uncore = rq->engine->uncore;
> +	enum forcewake_domains fw;
> +	unsigned long flags;
>  	struct i915_wa *wa;
>  	unsigned int i;
>  	u32 *cs;
> @@ -1002,13 +1005,35 @@ int intel_engine_emit_ctx_wa(struct i915_request *rq)
>  	if (IS_ERR(cs))
>  		return PTR_ERR(cs);
>  
> +	fw = wal_get_fw_for_rmw(uncore, wal);
> +
> +	intel_gt_mcr_lock(wal->gt, &flags);
> +	spin_lock(&uncore->lock);
> +	intel_uncore_forcewake_get__locked(uncore, fw);
> +
>  	*cs++ = MI_LOAD_REGISTER_IMM(wal->count);
>  	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +		u32 val;
> +
> +		if (wa->masked_reg || wa->set == U32_MAX) {

I think you still want:

		if (wa->masked_reg || wa->set == U32_MAX || wa->clr == U32_MAX) {

since there's no point to doing a read just to mask off 100% of the
values.  Harmless, of course, but unnecessary.

Either way, patches 1-5 are:
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

> +			val = wa->set;
> +		} else {
> +			val = wa->is_mcr ?
> +				intel_gt_mcr_read_any_fw(wal->gt, wa->mcr_reg) :
> +				intel_uncore_read_fw(uncore, wa->reg);
> +			val &= ~wa->clr;
> +			val |= wa->set;
> +		}
> +
>  		*cs++ = i915_mmio_reg_offset(wa->reg);
> -		*cs++ = wa->set;
> +		*cs++ = val;
>  	}
>  	*cs++ = MI_NOOP;
>  
> +	intel_uncore_forcewake_put__locked(uncore, fw);
> +	spin_unlock(&uncore->lock);
> +	intel_gt_mcr_unlock(wal->gt, flags);
> +
>  	intel_ring_advance(rq, cs);
>  
>  	ret = rq->engine->emit_flush(rq, EMIT_BARRIER);
> 


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]