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

[Matthew Brost]

On Thu, Oct 26, 2023 at 11:04:11AM +0100, Matthew Auld wrote:
> 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.
> 

This makes sense to me as all user operations should be scheduled behind
this clear. KERNEL is likely the right usage.

> 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.
> 

I think if a BO is a VRAM the evict of that BO will be on the same queue
as a clear. We probably can delete this and add a comment? That being
said waiting on the kernel slots really is not a huge deal either as
pretty much everything else that will use this BO will wait on those
slots anyways too. Waiting is probably safer too.

Matt 

> > 
> > 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
> > >