Re: [PATCH v3 1/2] drm/gpusvm: Add a DMA-mapping accounting callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] drm/gpusvm: incomplete full-SVM ops check and doc in
  drm_gpusvm_init()
- [Low] drm/gpusvm: document locking context for
  drm_gpusvm_ops.dma_map_account
--

commit 8cdf85308b0a5f01cbf31357b5001cfa11f7973f
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>

drm/gpusvm: Add a DMA-mapping accounting callback

This commit adds an optional dma_map_account callback to struct
drm_gpusvm_ops for symmetric DMA-mapping accounting and allows a restricted
ops table with page-level hooks when initializing drm_gpusvm without an mm.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 45169eb6de619..18b51be7d6df3 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -503,8 +503,16 @@ int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
>  			return -EINVAL;
>  		mmgrab(mm);
>  	} else {
> -		/* No full SVM mode, only core drm_gpusvm_pages API. */
> -		if (ops || num_chunks || mm_range || notifier_size)
> +		/*
> +		 * No full SVM mode, only core drm_gpusvm_pages API. A
> +		 * restricted @ops that only implements the page-level hooks
> +		 * (e.g. @dma_map_account) is allowed; the full-SVM hooks must
> +		 * not be set.
> +		 */
> +		if (num_chunks || mm_range || notifier_size)
> +			return -EINVAL;
> +		if (ops && (ops->invalidate || ops->range_alloc ||
> +			    ops->range_free))
>  			return -EINVAL;
>  	}

[Severity: Low]
Should this check also reject ops->notifier_alloc and ops->notifier_free
when mm is NULL, as those are full-SVM hooks on struct drm_gpusvm_ops as
well?

Also, the kernel-doc above drm_gpusvm_init() still says that only @gpusvm
and @name are expected when using the simple drm_gpusvm_pages API.

[ ... ]

> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> index 4d31c6bba7455..637c19f7169f1 100644
> --- a/include/drm/drm_gpusvm.h
> +++ b/include/drm/drm_gpusvm.h
> @@ -75,6 +75,25 @@ struct drm_gpusvm_ops {
>  	void (*invalidate)(struct drm_gpusvm *gpusvm,
>  			   struct drm_gpusvm_notifier *notifier,
>  			   const struct mmu_notifier_range *mmu_range);
> +
> +	/**
> +	 * @dma_map_account: Account a DMA-mapping change (optional)
> +	 * @gpusvm: Pointer to the GPU SVM
> +	 * @addr: The address descriptor of the chunk being (un)mapped
> +	 * @sign: +1 when @addr has just been DMA-mapped, -1 when it is being
> +	 *        unmapped
> +	 *
> +	 * Called once per &drm_pagemap_addr entry, when the entry is
> +	 * DMA-mapped by drm_gpusvm_get_pages() (@sign == +1) and when it is
> +	 * unmapped (@sign == -1), so the driver can keep symmetric accounting
> +	 * of the pages it has mapped for GPU access. The @addr->proto field
> +	 * can be used to distinguish system-memory mappings from device
> +	 * interconnect mappings. May be provided in both full SVM mode and the
> +	 * core drm_gpusvm_pages-only mode.
> +	 */
> +	void (*dma_map_account)(struct drm_gpusvm *gpusvm,
> +				const struct drm_pagemap_addr *addr,
> +				int sign);
>  };

[Severity: Low]
Could the kernel-doc for @dma_map_account document its locking and
execution context, similar to @invalidate and @devmem_fn?

It is called under gpusvm->notifier_lock in read mode during
drm_gpusvm_dma_map_pages() and non-notifier unmaps (allowing concurrent
calls), and in write mode from MMU notifier invalidation context when
ctx->in_notifier is set in drm_gpusvm_unmap_pages().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113190128.2118723-1-matthew.brost@intel.com?part=1