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