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 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?

> 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?

> +        * 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?

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