Re: [PATCH v5 03/24] drm/pagemap, drm/xe: Ensure that the devmem allocation is idle before use

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Dec 18, 2025 at 08:18:24PM +0100, Thomas Hellström wrote:
> On Thu, 2025-12-18 at 10:33 -0800, Matthew Brost wrote:
> > On Thu, Dec 18, 2025 at 05:20:40PM +0100, Thomas Hellström wrote:
> > > In situations where no system memory is migrated to devmem, and in
> > > upcoming patches where another GPU is performing the migration to
> > > the newly allocated devmem buffer, there is nothing to ensure any
> > > ongoing clear to the devmem allocation or async eviction from the
> > > devmem allocation is complete.
> 
> 
> > > 
> > > Address that by passing a struct dma_fence down to the copy
> > > functions, and ensure it is waited for before migration is marked
> > > complete.
> > > 
> > > v3:
> > > - New patch.
> > > v4:
> > > - Update the logic used for determining when to wait for the
> > >   pre_migrate_fence.
> > > - Update the logic used for determining when to warn for the
> > >   pre_migrate_fence since the scheduler fences apparently
> > >   can signal out-of-order.
> > > v5:
> > > - Fix a UAF (CI)
> > > - Remove references to source P2P migration (Himal)
> > > - Put the pre_migrate_fence after migration.
> > > 
> > > Fixes: c5b3eb5a906c ("drm/xe: Add GPUSVM device memory copy vfunc
> > > functions")
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: <stable@vger.kernel.org> # v6.15+
> > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_pagemap.c | 17 ++++++---
> > >  drivers/gpu/drm/xe/xe_svm.c   | 65 ++++++++++++++++++++++++++++++-
> > > ----
> > >  include/drm/drm_pagemap.h     | 17 +++++++--
> > >  3 files changed, 83 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_pagemap.c
> > > b/drivers/gpu/drm/drm_pagemap.c
> > > index 4cf8f54e5a27..ac3832f85190 100644
> > > --- a/drivers/gpu/drm/drm_pagemap.c
> > > +++ b/drivers/gpu/drm/drm_pagemap.c
> > > @@ -3,6 +3,7 @@
> > >   * Copyright © 2024-2025 Intel Corporation
> > >   */
> > >  
> > > +#include <linux/dma-fence.h>
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/migrate.h>
> > >  #include <linux/pagemap.h>
> > > @@ -408,10 +409,14 @@ int drm_pagemap_migrate_to_devmem(struct
> > > drm_pagemap_devmem *devmem_allocation,
> > >  		drm_pagemap_get_devmem_page(page, zdd);
> > >  	}
> > >  
> > > -	err = ops->copy_to_devmem(pages, pagemap_addr, npages);
> > > +	err = ops->copy_to_devmem(pages, pagemap_addr, npages,
> > > +				  devmem_allocation-
> > > >pre_migrate_fence);
> > >  	if (err)
> > >  		goto err_finalize;
> > >  
> > > +	dma_fence_put(devmem_allocation->pre_migrate_fence);
> > > +	devmem_allocation->pre_migrate_fence = NULL;
> > > +
> > >  	/* Upon success bind devmem allocation to range and zdd */
> > >  	devmem_allocation->timeslice_expiration = get_jiffies_64()
> > > +
> > >  		msecs_to_jiffies(timeslice_ms);
> > > @@ -596,7 +601,7 @@ int drm_pagemap_evict_to_ram(struct
> > > drm_pagemap_devmem *devmem_allocation)
> > >  	for (i = 0; i < npages; ++i)
> > >  		pages[i] = migrate_pfn_to_page(src[i]);
> > >  
> > > -	err = ops->copy_to_ram(pages, pagemap_addr, npages);
> > > +	err = ops->copy_to_ram(pages, pagemap_addr, npages, NULL);
> > >  	if (err)
> > >  		goto err_finalize;
> > >  
> > > @@ -719,7 +724,7 @@ static int __drm_pagemap_migrate_to_ram(struct
> > > vm_area_struct *vas,
> > >  	for (i = 0; i < npages; ++i)
> > >  		pages[i] = migrate_pfn_to_page(migrate.src[i]);
> > >  
> > > -	err = ops->copy_to_ram(pages, pagemap_addr, npages);
> > > +	err = ops->copy_to_ram(pages, pagemap_addr, npages, NULL);
> > >  	if (err)
> > >  		goto err_finalize;
> > >  
> > > @@ -800,11 +805,14 @@
> > > EXPORT_SYMBOL_GPL(drm_pagemap_pagemap_ops_get);
> > >   * @ops: Pointer to the operations structure for GPU SVM device
> > > memory
> > >   * @dpagemap: The struct drm_pagemap we're allocating from.
> > >   * @size: Size of device memory allocation
> > > + * @pre_migrate_fence: Fence to wait for or pipeline behind before
> > > migration starts.
> > > + * (May be NULL).
> > >   */
> > >  void drm_pagemap_devmem_init(struct drm_pagemap_devmem
> > > *devmem_allocation,
> > >  			     struct device *dev, struct mm_struct
> > > *mm,
> > >  			     const struct drm_pagemap_devmem_ops
> > > *ops,
> > > -			     struct drm_pagemap *dpagemap, size_t
> > > size)
> > > +			     struct drm_pagemap *dpagemap, size_t
> > > size,
> > > +			     struct dma_fence *pre_migrate_fence)
> > >  {
> > >  	init_completion(&devmem_allocation->detached);
> > >  	devmem_allocation->dev = dev;
> > > @@ -812,6 +820,7 @@ void drm_pagemap_devmem_init(struct
> > > drm_pagemap_devmem *devmem_allocation,
> > >  	devmem_allocation->ops = ops;
> > >  	devmem_allocation->dpagemap = dpagemap;
> > >  	devmem_allocation->size = size;
> > > +	devmem_allocation->pre_migrate_fence = pre_migrate_fence;
> > >  }
> > >  EXPORT_SYMBOL_GPL(drm_pagemap_devmem_init);
> > >  
> > > diff --git a/drivers/gpu/drm/xe/xe_svm.c
> > > b/drivers/gpu/drm/xe/xe_svm.c
> > > index bab8e6cbe53d..b806a1fce188 100644
> > > --- a/drivers/gpu/drm/xe/xe_svm.c
> > > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > > @@ -472,11 +472,12 @@ static void xe_svm_copy_us_stats_incr(struct
> > > xe_gt *gt,
> > >  
> > >  static int xe_svm_copy(struct page **pages,
> > >  		       struct drm_pagemap_addr *pagemap_addr,
> > > -		       unsigned long npages, const enum
> > > xe_svm_copy_dir dir)
> > > +		       unsigned long npages, const enum
> > > xe_svm_copy_dir dir,
> > > +		       struct dma_fence *pre_migrate_fence)
> > >  {
> > >  	struct xe_vram_region *vr = NULL;
> > >  	struct xe_gt *gt = NULL;
> > > -	struct xe_device *xe;
> > > +	struct xe_device *xe = NULL;
> > >  	struct dma_fence *fence = NULL;
> > >  	unsigned long i;
> > >  #define XE_VRAM_ADDR_INVALID	~0x0ull
> > > @@ -485,6 +486,16 @@ static int xe_svm_copy(struct page **pages,
> > >  	bool sram = dir == XE_SVM_COPY_TO_SRAM;
> > >  	ktime_t start = xe_gt_stats_ktime_get();
> > >  
> > > +	if (pre_migrate_fence &&
> > > dma_fence_is_container(pre_migrate_fence)) {
> > > +		/*
> > > +		 * This would typically be a composite fence
> > > operation on the destination memory.
> > > +		 * Ensure that the other GPU operation on the
> > > destination is complete.
> > > +		 */
> > > +		err = dma_fence_wait(pre_migrate_fence, true);
> > > +		if (err)
> > > +			return err;
> > > +	}
> > > +
> > 
> > I'm not fully convienced this code works. Consider the case where we
> > allocate memory a device A and we copying from device B. In this case
> > device A issues the clear but device B issues the copy. The
> > pre_migrate_fence is not going be a container and I believe our
> > ordering
> > breaks.
> 
> So the logic to handle that case was moved to the patch that enables
> source migration, as per Himal's review comment. So consider this patch
> only for destination migration where the devmem allocation is allocated
> on the same gpu that migrates.
>

Ah, yes I see that logic in patch 23.
 
> > 
> > Would it be simplier to pass in the pre_migrate_fence fence a
> > dependency
> > to the first copy job issued, then set it to NULL. The drm scheduler
> > is
> > smart enough to squash the input fence into a NOP if a copy / clear
> > are
> > from the same devices queues.
> 
> I intended to do that as a follow-up if needed but since this is a fix
> that fixes an existing problem I wanted it to be lightweight. But let
> me take a quick look at that and probably resend only that patch.
> 

I'm fine with doing this in a follow up if that is the preference, I do
think that would be cleaner.

So if you prefer to leave this as is:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> /Thomas
> 
>