Re: [PATCH 2/2] drm/xe: Add debugfs stats for DMA-mapped pages per order

[sashiko-bot@kernel.org]

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 <thomas.hellstrom@linux.intel.com>

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/20261104215228.1533692-1-matthew.brost@intel.com?part=2