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

[Lucas De Marchi]

On Wed, Sep 06, 2023 at 04:46:02PM +0000, Jose Souza wrote:
>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?


not very difficult, but needs some hours of work to get it right.

https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/668

Lucas De Marchi


>
>>
>> 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);
>>  		}
>>  	}
>>
>