[Intel-xe] [PATCH 3/3] drm/xe/bo: sync kernel fences for KMD buffers

[Matthew Brost]

On Thu, Oct 26, 2023 at 10:17:19AM +0100, Matthew Auld wrote:
> On 25/10/2023 22:00, Matthew Brost wrote:
> > On Wed, Oct 25, 2023 at 06:39:41PM +0100, Matthew Auld wrote:
> > > With things like pipelined evictions, VRAM pages can be marked as free
> > > and yet still have some active kernel fences, with the idea that the
> > > next caller to allocate the memory will respect them. However it looks
> > > like we are missing synchronisation for KMD internal buffers, like
> > > page-tables, lrc etc. For userspace objects we should already have the
> > > required serialisation for CPU access via the fault handler, and
> > > likewise for GPU access when vm_binding them.
> > > 
> > > To fix this serialise against any kernel fences for all KMD objects at
> > > creation. This seems to resolve some severe corruption seen during
> > > evictions.
> > > 
> > > Closes: ?
> > > Testcase: igt at xe-evict-ccs
> > > 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_bo.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > > index 61789c0e88fb..26a103aa5d48 100644
> > > --- a/drivers/gpu/drm/xe/xe_bo.c
> > > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > > @@ -1272,6 +1272,16 @@ struct xe_bo *__xe_bo_create_locked(struct xe_device *xe, struct xe_bo *bo,
> > >   	else
> > >   		ttm_bo_move_to_lru_tail_unlocked(&bo->ttm);
> > > +	/*
> > > +	 * TTM can give us VRAM that still has active fences i.e GPU might still
> > > +	 * be accessing it. To keep things simple just sync any kernel fences
> > > +	 * here, if the buffer is KMD internal. For normal userspace objects we
> > > +	 * should already have the required pipelining or sync waiting.
> > > +	 */
> > > +	if (type == ttm_bo_type_kernel)
> > > +		dma_resv_wait_timeout(bo->ttm.base.resv, DMA_RESV_USAGE_KERNEL,
> > > +				      false, MAX_SCHEDULE_TIMEOUT);
> > > +
> > 
> > So this is the case where we create a kernel BO within a VM (e.g. memory
> > for page tables) and the memory we received is also within the same VM
> > (bo->ttm.base.resv == VM resv object)? The memory we received could
> > still be being used thus we need to wait for the memory to be idle? Is
> > that right? Also how did you arrive at the DMA_RESV_USAGE_KERNEL slot to
> > determine idle?
> 
> Yeah, if the BO dma-resv is the same as the vm, then waiting on the vm
> dma-resv would also work, or using the KERNEL fences inside as dependencies
> if we want to pipeline it.
> 
> In TTM when evicting VRAM it can be done async. When TTM finds appropriate
> BO(s) to evict we don't need to sync wait on any fences (like for userspace
> job etc). In our xe_bo_move() callback when handed some object to evict, we
> just keep the pipeline going, and schedule the move from VRAM -> TT, using
> the dma-resv fences as input dependencies for the copy job (if any). At the
> end of xe_bo_move() we call ttm_bo_move_accel_cleanup(), passing in the
> final fence for the copy job. TTM then adds that fence as USAGE_KERNEL into
> the dma-resv of the object being evicted. The other important step is
> attaching that same fence onto the actual VRAM resource manager, as per
> ttm_bo_move_pipeline_evict(). With the fence attached to the VRAM manager
> the buddy allocator blocks are all freed, even though the memory ranges they
> represent are potentially still being accessed by the GPU. The next users
> that come along allocating memory from the VRAM manager, will get the copy
> job fence that was attached added to their dma-resv with USAGE_KERNEL. See
> ttm_bo_mem_space()/ttm_bo_mem_force_space() -> ttm_bo_add_move_fence().
> 
> So with all that waiting for DMA_RESV_USAGE_KERNEL here for KMD internal
> stuff should be enough.
> 

Makes sense. Can you adjust the comment explaining why the kernel slot
is the correct slot?

Matt

> > 
> > Matt
> > 
> > >   	return bo;
> > >   }
> > > -- 
> > > 2.41.0
> > >