[Intel-xe] [PATCH 2/3] drm/xe/bo: consider dma-resv fences for clear job

[Matthew Auld]

On 25/10/2023 22:06, Matthew Brost wrote:
> On Wed, Oct 25, 2023 at 06:39:40PM +0100, Matthew Auld wrote:
>> There could be active fences already in the dma-resv for the object
>> prior to clearing. Make sure to input them as dependencies for the clear
>> job.
>>
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_migrate.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
>> index 009d728af762..896a6d8398ea 100644
>> --- a/drivers/gpu/drm/xe/xe_migrate.c
>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
>> @@ -980,8 +980,6 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m,
>>   
>>   		size -= clear_L0;
>>   
>> -		/* TODO: Add dependencies here */
>> -
>>   		/* Preemption is enabled again by the ring ops. */
>>   		if (!clear_vram) {
>>   			emit_pte(m, bb, clear_L0_pt, clear_vram, &src_it, clear_L0,
>> @@ -1010,6 +1008,12 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m,
>>   		}
>>   
>>   		xe_sched_job_add_migrate_flush(job, flush_flags);
>> +		if (!fence) {
>> +			err = job_add_deps(job, bo->ttm.base.resv,
>> +					   DMA_RESV_USAGE_BOOKKEEP);
> 
> Hmm, so if the BO is private to the VM new memory allocations for VRAM
> (triggers clears) are now staged behind the entire VM being idle (e.g.
> no execs, no binds inflight) and since this is a kernel operation all
> new VM operations are stage behind this clear. This seems very, very
> expensive. If there not another way around this?

Can make this USAGE_KERNEL I think. It's just the move fences we need 
worry about and not the userspace stuff since it hasn't gotten past the 
initial clear, I think.

I don't think we evict between tiles, but rather just VRAM -> TT. If we 
can guarantee that all copy and clear jobs happen with the same queue, 
and the order we add jobs is the order they will appear in the ring and 
the queue is always tied to one tile/VRAM manager, then I think we can 
drop this, and add big block comment here instead? But I do think adding 
USAGE_KERNEL here is a good idea in general.

> 
> Also on compute VMs this would trigger preempt fences which would not be
> ideal.
> 
> Matt
> 
>> +			if (err)
>> +				goto err_job;
>> +		}
>>   
>>   		xe_sched_job_arm(job);
>>   		dma_fence_put(fence);
>> @@ -1024,6 +1028,8 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m,
>>   		xe_bb_free(bb, fence);
>>   		continue;
>>   
>> +err_job:
>> +		xe_sched_job_put(job);
>>   err:
>>   		mutex_unlock(&m->job_mutex);
>>   		xe_bb_free(bb, NULL);
>> -- 
>> 2.41.0
>>