On Wed, Nov 04, 2026 at 10:15:05PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
> - [Medium] drm/xe: premature xe_device_assert_dma_pages_zero() before
> ttm_device_fini()
> - [Low] drm/xe: duplicate dma_map_account callbacks in xe_svm.c and
> xe_userptr.c
> --
>
> commit d97316f4a4863b594d6196037dc27dd4e278bdd0
> Author: Thomas Hellström <[email protected]>
>
> drm/xe: Add debugfs stats for DMA-mapped pages per order
>
> This commit adds per-order DMA-mapped system page counters for TTM buffer
> objects and GPU SVM / userptr ranges, exposes them via debugfs at
> dma_mapped_pages, and asserts that all counters return to zero during device
> teardown.
>
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 273e2c45fea9b..c40bb8f7c697a 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -377,12 +377,42 @@ bool xe_device_is_admin_only(const struct xe_device
> > *xe)
> [ ... ]
> > static void xe_device_destroy(struct drm_device *dev, void *dummy)
> > {
> > struct xe_device *xe = to_xe_device(dev);
> >
> > xe_bo_dev_fini(&xe->bo_device);
> >
> > + /*
> > + * Assert the DMA-mapped-pages accounting only after xe_bo_dev_fini()
> > + * has flushed the async BO-free worker: BOs pending async free still
> > + * hold their DMA mappings (and counts) until the worker runs, so
> > + * asserting before the flush could trip a false positive.
> > + */
> > + xe_device_assert_dma_pages_zero(xe);
> > +
> > if (xe->preempt_fence_wq)
> > destroy_workqueue(xe->preempt_fence_wq);
> >
> > if (xe->ordered_wq)
> > destroy_workqueue(xe->ordered_wq);
> >
> > if (xe->unordered_wq)
> > destroy_workqueue(xe->unordered_wq);
> >
> > if (xe->destroy_wq)
> > destroy_workqueue(xe->destroy_wq);
> >
> > ttm_device_fini(&xe->ttm);
>
> [Severity: Medium]
> Can xe_device_assert_dma_pages_zero() trigger a false-positive assertion here
> because it runs before ttm_device_fini(&xe->ttm)?
>
> When BOs with a populated ttm_tt and DMA-mapped xe_tt->sg are released via
> ttm_bo_put() -> ttm_bo_release(), TTM defers cleanup to bdev->wq
> (xe->ttm.wq) whenever want_init_on_free() is active or reservation fences or
> locks are busy:
>
> drivers/gpu/drm/ttm/ttm_bo.c:ttm_bo_release()
> if (!dma_resv_test_signaled(&bo->base._resv,
> DMA_RESV_USAGE_BOOKKEEP) ||
> (want_init_on_free() && (bo->ttm != NULL)) ||
> bo->type == ttm_bo_type_sg ||
> !dma_resv_trylock(bo->base.resv)) {
> ...
> INIT_WORK(&bo->delayed_delete, ttm_bo_delayed_delete);
> ...
> queue_work_node(bdev->pool.nid, bdev->wq, &bo->delayed_delete);
> return;
> }
>
> In that case, the DMA unmap and counter decrement via:
>
> ttm_bo_delayed_delete()
> -> ttm_bo_cleanup_memtype_use()
> -> ttm_bo_tt_destroy()
> -> ttm_tt_unpopulate()
> -> xe_ttm_tt_unpopulate()
> -> xe_tt_unmap_sg()
> -> xe_tt_account_dma_pages(xe, xe_tt, -1)
>
> will not complete until ttm_device_fini() drains bdev->wq. Should
> xe_device_assert_dma_pages_zero(xe) be called after ttm_device_fini(&xe->ttm)
> instead?
>
Yes, I can fix this when merging.
Matt
> > diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> > index 909a602d091ea..7a49882897495 100644
> > --- a/drivers/gpu/drm/xe/xe_svm.c
> > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > @@ -845,10 +845,50 @@ static int xe_svm_get_pagemaps(struct xe_vm *vm)
> > }
> > #endif
> >
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +/*
> > + * xe_svm_dma_map_account - drm_gpusvm DMA-mapping accounting callback
> > + * @gpusvm: The GPU SVM the mapping belongs to
> > + * @addr: The address descriptor of the chunk being (un)mapped
> > + * @sign: +1 when @addr was DMA-mapped, -1 when it is being unmapped
> > + *
> > + * Maintain per-order counts of the system-memory pages that the full SVM
> > + * instance (SVM ranges, and userptr VMAs in fault mode) has DMA-mapped
> > for GPU
> > + * access. Only DRM_INTERCONNECT_SYSTEM entries are real DMA maps and
> > counted;
> > + * device interconnect (VRAM, P2P) entries are skipped. The counts are
> > solely
> > + * exposed through debugfs, so the callback is only registered when
> > + * CONFIG_DEBUG_FS is enabled.
> > + */
> > +static void xe_svm_dma_map_account(struct drm_gpusvm *gpusvm,
> > + const struct drm_pagemap_addr *addr,
> > + int sign)
> > +{
> > + struct xe_device *xe = gpusvm_to_vm(gpusvm)->xe;
> > + unsigned int order = addr->order;
> > +
> > + if (addr->proto != DRM_INTERCONNECT_SYSTEM)
> > + return;
> > +
> > + /*
> > + * The CPU-PTE map order reported by hmm can exceed MAX_PAGE_ORDER
> > + * (e.g. an order-18 1GB mapping), so clamp the array index to avoid an
> > + * out-of-bounds access. Clamping is symmetric across the +1/-1 calls,
> > + * so the per-order counters stay balanced.
> > + */
> > + order = min(order, (unsigned int)NR_PAGE_ORDERS - 1);
> > +
> > + atomic_long_add((long)(1UL << addr->order) * sign,
> > + &xe->mem.dma_mapped_pages_svm[order]);
> > +}
> > +#endif
> > +
> > static const struct drm_gpusvm_ops gpusvm_ops = {
> > .range_alloc = xe_svm_range_alloc,
> > .range_free = xe_svm_range_free,
> > .invalidate = xe_svm_invalidate,
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > + .dma_map_account = xe_svm_dma_map_account,
> > +#endif
> > };
> [ ... ]
> > diff --git a/drivers/gpu/drm/xe/xe_userptr.c
> > b/drivers/gpu/drm/xe/xe_userptr.c
> > index 9c1dac0fce6f1..40f97bd4751dd 100644
> > --- a/drivers/gpu/drm/xe/xe_userptr.c
> > +++ b/drivers/gpu/drm/xe/xe_userptr.c
> > @@ -8,9 +8,72 @@
> >
> > #include <linux/mm.h>
> >
> > +#include <drm/drm_pagemap.h>
> > +
> > #include "xe_tlb_inval.h"
> > #include "xe_trace_bo.h"
> >
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +/*
> > + * xe_userptr_dma_map_account - drm_gpusvm DMA-mapping accounting callback
> > + * @gpusvm: The GPU SVM the mapping belongs to
> > + * @addr: The address descriptor of the chunk being (un)mapped
> > + * @sign: +1 when @addr was DMA-mapped, -1 when it is being unmapped
> > + *
> > + * Account, per page order, the system-memory pages DMA-mapped for GPU
> > access
> > + * through the drm_gpusvm_pages instance shared by userptr (and, in fault
> > mode,
> > + * SVM). Only DRM_INTERCONNECT_SYSTEM entries are real DMA maps and
> > counted;
> > + * device interconnect (VRAM, P2P) entries are skipped. The counts are
> > solely
> > + * exposed through debugfs, so the callback is only registered when
> > + * CONFIG_DEBUG_FS is enabled.
> > + */
> > +static void xe_userptr_dma_map_account(struct drm_gpusvm *gpusvm,
> > + const struct drm_pagemap_addr *addr,
> > + int sign)
> > +{
> > + struct xe_vm *vm = container_of(gpusvm, struct xe_vm, svm.gpusvm);
> > + unsigned int order = addr->order;
> > +
> > + if (addr->proto != DRM_INTERCONNECT_SYSTEM)
> > + return;
> > +
> > + /*
> > + * The CPU-PTE map order reported by hmm can exceed MAX_PAGE_ORDER
> > + * (e.g. an order-18 1GB mapping), so clamp the array index to avoid an
> > + * out-of-bounds access. Clamping is symmetric across the +1/-1 calls,
> > + * so the per-order counters stay balanced.
> > + */
> > + order = min(order, (unsigned int)NR_PAGE_ORDERS - 1);
> > +
> > + atomic_long_add((long)(1UL << addr->order) * sign,
> > + &vm->xe->mem.dma_mapped_pages_svm[order]);
> > +}
>
> [Severity: Low]
> This isn't a bug, but the commit message and comment state that the
> dma_map_account callback is shared between the fault-mode SVM instance and the
> simple userptr instance, whereas xe_svm_dma_map_account() in
> drivers/gpu/drm/xe/xe_svm.c and xe_userptr_dma_map_account() in
> drivers/gpu/drm/xe/xe_userptr.c duplicate the same implementation.
>
> Since CONFIG_DRM_XE_GPUSVM selects CONFIG_DRM_GPUSVM, could
> xe_userptr_dma_map_account() be exposed and reused in gpusvm_ops instead of
> defining two identical callbacks?
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=2
