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

["Summers, Stuart" <stuart.summers@intel.com>]

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?

> 
> > > +        * 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);
> >