[PATCH 02/12] drm/xe: Fix ring flush invalidation

[Tvrtko Ursulin]

On 27/02/2025 23:46, Matt Roper wrote:
> On Fri, Feb 21, 2025 at 10:17:21AM +0000, Tvrtko Ursulin wrote:
>> Emit_flush_invalidate() is incorrectly marking the write to LRC_PPHWSP as
>> a GGTT write and also writing an atypical ~0 dword as the payload. Fix it.
> 
> Technically the write is a GGTT write (the PPHWSP is allocated as part
> of the LRC, and the LRC is mapped to the GGTT).  But the important thing
> here is that bit 21 (MI_FLUSH_DW_STORE_INDEX) redefines the meaning of
> the PPGTT vs GGTT bit to instead mean "index into pphwsp" vs "index into
> ghwsp."  And based on the name of the offset we use
> (LRC_PPHWSP_FLUSH_INVAL_SCRATCH_ADDR) it sounds like the pphwsp was
> indeed intended here, so dropping the 'use GTT' flag is in line with
> that.
> 
> I don't think the actual destination (ghwsp vs pphwsp) or the value (~0
> or 0) actually has any functional impact since there doesn't seem to be
> anything watching and waiting for those writes to show up in memory.  We
> just have to use the OP_STOREDW flag because the TLB invalidate flag
> mandates that it also be present.
> 
> But your patch makes things more consistent, so
> 
> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

Thanks! And yes, my commit message was quite bad. If xe was using csb 
from the ghwsp maybe that would be trashing them (I briefly looked at 
the offsets but got confused by what Tigerlake PRM versus the code was 
claiming), but it isn't even in execlists mode.

Btw, do you know if Alderlake PRMs will be made public? They are not on 
https://www.intel.com/content/www/us/en/docs/graphics-for-linux/developer-reference/1-0/overview.html 
at least. Is there a different home for them nowadays?

Regards,

Tvrtko

>>
>> While at it drop the unused flags argument.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>> ---
>>   drivers/gpu/drm/xe/xe_ring_ops.c | 15 ++++++---------
>>   1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
>> index 0c230ee53bba..a2d1fb8f0adf 100644
>> --- a/drivers/gpu/drm/xe/xe_ring_ops.c
>> +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
>> @@ -111,16 +111,13 @@ static int emit_bb_start(u64 batch_addr, u32 ppgtt_flag, u32 *dw, int i)
>>   	return i;
>>   }
>>   
>> -static int emit_flush_invalidate(u32 flag, u32 *dw, int i)
>> +static int emit_flush_invalidate(u32 *dw, int i)
>>   {
>> -	dw[i] = MI_FLUSH_DW;
>> -	dw[i] |= flag;
>> -	dw[i++] |= MI_INVALIDATE_TLB | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_IMM_DW |
>> -		MI_FLUSH_DW_STORE_INDEX;
>> -
>> -	dw[i++] = LRC_PPHWSP_FLUSH_INVAL_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT;
>> +	dw[i++] = MI_FLUSH_DW | MI_INVALIDATE_TLB | MI_FLUSH_DW_OP_STOREDW |
>> +		  MI_FLUSH_IMM_DW | MI_FLUSH_DW_STORE_INDEX;
>> +	dw[i++] = LRC_PPHWSP_FLUSH_INVAL_SCRATCH_ADDR;
>> +	dw[i++] = 0;
>>   	dw[i++] = 0;
>> -	dw[i++] = ~0U;
>>   
>>   	return i;
>>   }
>> @@ -409,7 +406,7 @@ static void emit_migration_job_gen12(struct xe_sched_job *job,
>>   	if (!IS_SRIOV_VF(gt_to_xe(job->q->gt))) {
>>   		/* XXX: Do we need this? Leaving for now. */
>>   		dw[i++] = preparser_disable(true);
>> -		i = emit_flush_invalidate(0, dw, i);
>> +		i = emit_flush_invalidate(dw, i);
>>   		dw[i++] = preparser_disable(false);
>>   	}
>>   
>> -- 
>> 2.48.0
>>
>