Re: [PATCH 3/3] drm/xe: Do not clear SVM device memory allocations up front

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

On Tue, Sep 29, 2026 at 02:16:18PM -0600, Summers, Stuart wrote:
> On Tue, 2026-09-29 at 11:10 -0700, Matthew Brost wrote:
> > xe_drm_pagemap_populate_mm() allocates a BO to back the range being
> > migrated into device memory, and TTM clears it. That clear is on the
> > GPU
> > page fault and SVM prefetch critical paths, and in the common case it
> > is
> > immediately overwritten in its entirety by the migration itself.
> > 
> > Allocate the BO with XE_BO_FLAG_SKIP_CLEAR and instead deal with the
> > contents in xe_svm_copy(). A migration to VRAM only sources pages
> > which
> > are populated on the CPU side, so if every page has a source DMA
> > address
> > the copy covers the whole allocation and nothing else is needed. Only
> > when the migration is sparse - holes in the CPU VMA from never
> > faulted
> > anonymous memory, for instance - is a clear issued, ahead of the
> > copies,
> > so the uncovered pages still read as zero.
> > 
> > The clear walks the destination device pages, taking the extent of
> > each
> > entry from its folio order since only folio heads are populated, and
> > coalesces physically contiguous entries into chunks of at most 8M. It
> 
> Why 8M?
> 

XE_MIGRATE_CHUNK_SIZE is existing code which has picked 8M for copies,
so using same size for clears. In practice this is limited at 2M as that
is max SVM allocation size but that part is table driven and can be
changed at any time. 

> > runs on the same ordered migrate queue as the copies, so it takes
> > over
> > the pre-migrate fence dependency and the copies are implicitly
> > ordered
> > behind it.
> > 
> > Assisted-by: Github-Copilot:Claude-opus-5
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_svm.c | 150
> > ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 143 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_svm.c
> > b/drivers/gpu/drm/xe/xe_svm.c
> > index f39e647512ad..1b4d1222fbb7 100644
> > --- a/drivers/gpu/drm/xe/xe_svm.c
> > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > @@ -586,6 +586,124 @@ static void xe_svm_copy_us_stats_incr(struct
> > xe_gt *gt,
> >         }
> >  }
> >  
> > +#define XE_MIGRATE_CHUNK_SIZE  SZ_8M
> > +#define XE_VRAM_ADDR_INVALID   ~0x0ull
> > +
> > +/**
> > + * xe_svm_copy_covers_all() - Does a migration write every page?
> > + * @pagemap_addr: Array of DMA information for the system side of
> > the migration
> > + * @npages: Number of pages covered by @pagemap_addr
> > + *
> > + * A migration to device memory only sources pages which are
> > actually populated
> > + * on the CPU side. Holes in the CPU VMA (never faulted anonymous
> > memory, for
> > + * instance) have no DMA address and leave the corresponding device
> > pages
> > + * untouched by the copy.
> > + *
> > + * Return: true if every page has a source address, false otherwise.
> > + */
> > +static bool xe_svm_copy_covers_all(struct drm_pagemap_addr
> > *pagemap_addr,
> > +                                  unsigned long npages)
> > +{
> > +       unsigned long i;
> > +
> > +       for (i = 0; i < npages;) {
> > +               if (!pagemap_addr[i].addr)
> > +                       return false;
> > +
> > +               i += NR_PAGES(pagemap_addr[i].order);
> > +       }
> > +
> > +       return true;
> > +}
> > +
> > +static int xe_svm_clear_vram_chunk(struct xe_vram_region *vr, u64
> > vram_addr,
> > +                                  unsigned long npages,
> > +                                  struct dma_fence **fence,
> > +                                  struct dma_fence **deps)
> > +{
> > +       struct dma_fence *__fence;
> > +
> > +       vm_dbg(&vr->xe->drm, "CLEAR VRAM - 0x%016llx, NPAGES=%ld",
> > +              vram_addr, npages);
> > +
> > +       __fence = xe_migrate_clear_vram(vr->migrate, npages,
> > vram_addr, *deps);
> > +       if (IS_ERR(__fence))
> > +               return PTR_ERR(__fence);
> > +
> > +       /* Ordered queue - only the first job needs to take the
> > dependency */
> > +       *deps = NULL;
> > +       dma_fence_put(*fence);
> > +       *fence = __fence;
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * xe_svm_clear_vram() - Clear the device memory backing a migration
> > + * @pages: Array of device pages which back the migration
> > destination
> > + * @npages: Number of pages in @pages
> > + * @fence: In/out pointer to the last fence issued on the migrate
> > queue
> > + * @deps: In/out pointer to a dependency to attach to the first job
> > issued
> > + *
> > + * Zero the device memory described by @pages. Entries in @pages are
> > only
> > + * populated at the head of each folio, so the extent of each entry
> > is taken
> > + * from the folio order, and physically contiguous entries are
> > coalesced into a
> > + * single clear of at most XE_MIGRATE_CHUNK_SIZE.
> > + *
> > + * Return: 0 on success, negative error code on failure.
> > + */
> > +static int xe_svm_clear_vram(struct page **pages, unsigned long
> > npages,
> > +                            struct dma_fence **fence,
> > +                            struct dma_fence **deps)
> > +{
> > +       struct xe_vram_region *vr = NULL;
> > +       unsigned long i, count = 0;
> > +       u64 vram_addr = XE_VRAM_ADDR_INVALID;
> > +       int err;
> > +
> > +       for (i = 0; i < npages;) {
> > +               struct page *page = pages[i];
> > +               unsigned long nr;
> > +               u64 addr;
> > +
> > +               if (!page) {
> > +                       ++i;
> > +                       continue;
> > +               }
> > +
> > +               if (!vr)
> > +                       vr = xe_page_to_vr(page);
> > +               XE_WARN_ON(xe_page_to_vr(page) != vr);
> > +
> > +               nr = NR_PAGES(folio_order(page_folio(page)));
> > +               addr = xe_page_to_dpa(page);
> > +
> > +               /* Not contiguous with the pending clear, or chunk is
> > full */
> > +               if (count && (addr != vram_addr + count * PAGE_SIZE
> > ||
> > +                             count + nr > XE_MIGRATE_CHUNK_SIZE /
> > PAGE_SIZE)) {
> > +                       err = xe_svm_clear_vram_chunk(vr, vram_addr,
> > count,
> > +                                                     fence, deps);
> > +                       if (err)
> > +                               return err;
> > +                       count = 0;
> > +               }
> > +
> > +               if (!count)
> > +                       vram_addr = addr;
> > +               count += nr;
> > +               i += nr;
> > +       }
> > +
> > +       if (count) {
> > +               err = xe_svm_clear_vram_chunk(vr, vram_addr, count,
> > fence,
> > +                                             deps);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int xe_svm_copy(struct page **pages,
> >                        struct drm_pagemap_addr *pagemap_addr,
> >                        unsigned long npages, const enum
> > xe_svm_copy_dir dir,
> > @@ -596,12 +714,23 @@ static int xe_svm_copy(struct page **pages,
> >         struct xe_device *xe;
> >         struct dma_fence *fence = NULL;
> >         unsigned long i;
> > -#define XE_VRAM_ADDR_INVALID   ~0x0ull
> >         u64 vram_addr = XE_VRAM_ADDR_INVALID;
> >         int err = 0, pos = 0;
> >         bool sram = dir == XE_SVM_COPY_TO_SRAM;
> >         ktime_t start = xe_gt_stats_ktime_get();
> >  
> > +       /*
> > +        * Device memory is allocated with XE_BO_FLAG_SKIP_CLEAR, so
> > it still
> 
> Should we check that explicitly somewhere here (or in the wrapper)?
> What if someone changes the code down the road to not skip the clear
> accidentally... I guess we just have a slight performance drop so maybe
> not a functional problem?
> 

We don't have the BO here, only addresses. The why interfaces in GPUSVM
are defined that layer has no idea if driver allocated a BO or not.

> > +        * holds whatever the previous owner left behind. A copy
> > covering every
> > +        * page scrubs it, anything less has to be cleared first.
> > +        */
> > +       if (!sram && !xe_svm_copy_covers_all(pagemap_addr, npages)) {
> > +               err = xe_svm_clear_vram(pages, npages, &fence,
> > +                                       &pre_migrate_fence);
> > +               if (err)
> > +                       goto err_out;
> > +       }
> > +
> >         /*
> >          * This flow is complex: it locates physically contiguous
> > device pages,
> >          * derives the starting physical address, and performs a
> > single GPU copy
> > @@ -617,7 +746,6 @@ static int xe_svm_copy(struct page **pages,
> >                 u64 __vram_addr;
> >                 bool match = false, chunk, last;
> >  
> > -#define XE_MIGRATE_CHUNK_SIZE  SZ_8M
> >                 chunk = (i - pos) == (XE_MIGRATE_CHUNK_SIZE /
> > PAGE_SIZE);
> >                 last = (i + 1) == npages;
> >  
> > @@ -758,8 +886,6 @@ static int xe_svm_copy(struct page **pages,
> >                 xe_svm_copy_us_stats_incr(gt, dir, npages, start);
> >  
> >         return err;
> > -#undef XE_MIGRATE_CHUNK_SIZE
> > -#undef XE_VRAM_ADDR_INVALID
> >  }
> >  
> >  static int xe_svm_copy_to_devmem(struct page **pages,
> > @@ -1121,18 +1247,28 @@ static int xe_drm_pagemap_populate_mm(struct
> > drm_pagemap *dpagemap,
> >         struct xe_validation_ctx vctx;
> >         struct drm_exec exec;
> >         struct xe_bo *bo;
> > +       u32 bo_flags;
> >         int err = 0, idx;
> >  
> >         if (!drm_dev_enter(&xe->drm, &idx))
> >                 return -ENODEV;
> >  
> > +       /*
> > +        * Skip the clear on device memory - xe_svm_copy() either
> > fully
> > +        * overwrites the allocation or clears it explicitly, so
> > clearing here
> > +        * is pure overhead on the page fault and prefetch paths.
> > +        */
> > +       if (IS_DGFX(xe))
> > +               bo_flags = XE_BO_FLAG_VRAM(vr) |
> > XE_BO_FLAG_SKIP_CLEAR;
> 
> This is maybe a comment that should go in the earlier patch that adds
> the flag, but should we have a check that ensures this is a
> kernel/migration BO and not a user BO?

I think the comment here is valid as it explains why setting
XE_BO_FLAG_SKIP_CLEAR is safe as it done later if it is required, but I
should likely add an assert !xe_bo_is_user if XE_BO_FLAG_SKIP_CLEAR is
set. Let me add that.

Matt

> 
> Thanks,
> Stuart
> 
> > +       else
> > +               bo_flags = XE_BO_FLAG_SYSTEM;
> > +       bo_flags |= XE_BO_FLAG_CPU_ADDR_MIRROR;
> > +
> >         xe_pm_runtime_get(xe);
> >  
> >         xe_validation_guard(&vctx, &xe->val, &exec, (struct
> > xe_val_flags) {}, err) {
> >                 bo = xe_bo_create_locked(xe, NULL, NULL, end - start,
> > -                                        ttm_bo_type_device,
> > -                                        (IS_DGFX(xe) ?
> > XE_BO_FLAG_VRAM(vr) : XE_BO_FLAG_SYSTEM) |
> > -                                        XE_BO_FLAG_CPU_ADDR_MIRROR,
> > &exec);
> > +                                        ttm_bo_type_device,
> > bo_flags, &exec);
> >                 drm_exec_retry_on_contention(&exec);
> >                 if (IS_ERR(bo)) {
> >                         err = PTR_ERR(bo);
>