[Intel-xe] [PATCH v2 4/4] drm/xe: Fix LRC workarounds

[Souza, Jose]

On Tue, 2023-09-05 at 18:20 -0700, Lucas De Marchi wrote:
> Fix 2 issues when writing LRC workarounds by copying the same handling
> done when processing other RTP entries:
> 
> For masked registers, it was not correctly setting the upper 16bits.
> Differently than i915, the entry itself doesn't set the upper bits
> for masked registers: this is done when applying them. Testing on ADL-P:
> 
> Before:
> 	[drm:xe_gt_record_default_lrcs [xe]] LRC WA rcs0 save-restore MMIOs
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x2580] = 0x00000002
> 	...
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x7018] = 0x00002000
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x7300] = 0x00000040
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x7304] = 0x00000200
> 
> After:
> 	[drm:xe_gt_record_default_lrcs [xe]] LRC WA rcs0 save-restore MMIOs
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x2580] = 0x00060002
> 	...
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x7018] = 0x20002000
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x7300] = 0x00400040
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x7304] = 0x02000200

Would it be too hard to test if LRC workarounds were applied correctly?

> 
> All of these registers are masked registers, so writing to them without
> the relevant bits in the upper 16b doesn't have any effect.
> 
> Also, this adds support to regular registers; previously it was assumed
> that LRC entries would only contain masked registers. However this is
> not true. 0x6604 is not a masked register, but used in workarounds for
> e.g.  ADL-P. See commit 28cf243a341a ("drm/i915/gt: Fix context
> workarounds with non-masked regs"). In the same test with ADL-P as
> above:
> 
> Before:
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x6604] = 0xe0000000
> After:
> 	[drm:xe_gt_record_default_lrcs [xe]] REG[0x6604] = 0xe0efef6f
> 
> As can be seen, now it will read what was in the register rather than
> completely overwrite the other bits.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt.c | 41 +++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 9e226b8a005a..678a276a25dc 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -114,11 +114,20 @@ static int emit_nop_job(struct xe_gt *gt, struct xe_exec_queue *q)
>  	return 0;
>  }
>  
> +/*
> + * Convert back from encoded value to type-safe, only to be used when reg.mcr
> + * is true
> + */
> +static struct xe_reg_mcr to_xe_reg_mcr(const struct xe_reg reg)
> +{
> +	return (const struct xe_reg_mcr){.__reg.raw = reg.raw };
> +}
> +
>  static int emit_wa_job(struct xe_gt *gt, struct xe_exec_queue *q)
>  {
>  	struct xe_reg_sr *sr = &q->hwe->reg_lrc;
>  	struct xe_reg_sr_entry *entry;
> -	unsigned long reg;
> +	unsigned long idx;
>  	struct xe_sched_job *job;
>  	struct xe_bb *bb;
>  	struct dma_fence *fence;
> @@ -129,18 +138,36 @@ static int emit_wa_job(struct xe_gt *gt, struct xe_exec_queue *q)
>  	if (IS_ERR(bb))
>  		return PTR_ERR(bb);
>  
> -	xa_for_each(&sr->xa, reg, entry)
> +	xa_for_each(&sr->xa, idx, entry)
>  		++count;
>  
>  	if (count) {
>  		xe_gt_dbg(gt, "LRC WA %s save-restore batch\n", sr->name);
>  
>  		bb->cs[bb->len++] = MI_LOAD_REGISTER_IMM(count);
> -		xa_for_each(&sr->xa, reg, entry) {
> -			bb->cs[bb->len++] = reg;
> -			bb->cs[bb->len++] = entry->set_bits;
> -			xe_gt_dbg(gt, "REG[0x%lx] = 0x%08x", reg,
> -				  entry->set_bits);
> +
> +		xa_for_each(&sr->xa, idx, entry) {
> +			struct xe_reg reg = entry->reg;
> +			struct xe_reg_mcr reg_mcr = to_xe_reg_mcr(reg);
> +			u32 val;
> +
> +			/*
> +			 * Skip reading the register if it's not really needed
> +			 */
> +			if (reg.masked)
> +				val = entry->clr_bits << 16;
> +			else if (entry->clr_bits + 1)
> +				val = (reg.mcr ?
> +				       xe_gt_mcr_unicast_read_any(gt, reg_mcr) :
> +				       xe_mmio_read32(gt, reg)) & (~entry->clr_bits);
> +			else
> +				val = 0;
> +
> +			val |= entry->set_bits;
> +
> +			bb->cs[bb->len++] = reg.addr;
> +			bb->cs[bb->len++] = val;
> +			xe_gt_dbg(gt, "REG[0x%x] = 0x%08x", reg.addr, val);
>  		}
>  	}
>