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 04:05:44PM -0600, Summers, Stuart wrote:
> On Tue, 2026-09-29 at 14:14 -0700, Matthew Brost wrote:
> > 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. 
> 
> Yeah I guess my question was related to the page size granularity (2M),
> so the 8M chunk seems a little arbitrary to me. I don't have all the
> migrate chunk size history though...
> 
> But yeah makes sense generally.
> 
> > 
> > > > 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.
> 
> Ok so basically the SVM layer is going to do this no matter what and we
> just need to ensure in the BO layer that this isn't doing a double
> clear? Should we have a performance test to make sure we don't regress
> there?
> 

The old code:
 - Always clear upon alloc BO
 - Copy

The new code:
 - Skip clear upon alloc BO
 - Do clear late *after* we get the CPU pages if the CPU pages don't
   fully cover BO, if fully covered skip clear
 - Copy

Having a test to verify this is tricky, as the speed of each machine's
copy engine varies based on several factors: CPU type, whether the IOMMU
is enabled, the discrete GPU SKU, and the GPU power management
configuration. In other words, hardcoding an expected value in an IGT
will not work.
 
I have scripts that measure copy time via GT stats after running an IGT,
and they show that this optimization shaves roughly 10 µs off the copy
time. Prefetch IGTs show an approximately 4 GB/s throughput increase in
benchmark sections on BMG G31, and similarly, UMD benchmarks that
measure fault throughput show roughly a 4 GB/s improvement.
 
I have a good understanding of what "good" numbers look like on the
machines I use regularly, and I also review any code that touches SVM,
so I'm not overly concerned about regressions.

Matt

> > 
> > > > +        * 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
> 
> Oh sorry that was really unclear by "a comment" here I meant my review
> comment, not your in code comment which looks fine :)
> 
> > 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.
> 
> Yeah perfect.
> 
> Thanks,
> Stuart
> 
> > 
> > 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);
> > > 
>