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 <[email protected]>
> > > Cc: <[email protected]> # v6.15+
> > > Signed-off-by: Thomas Hellström <[email protected]>
> > > ---
> > > 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 <[email protected]>
> /Thomas
>
>
