Bunch of DMA-buf cleanup patches

Bunch of DMA-buf cleanup patches

[Christian König]

Hello everyone,

just a few DMA-buf cleanup patches. The first one is fixing an incorrect
documentation which has annoyed me for quite a while.

The rest basically just removes stuff we no longer need or which was
just added as abstraction which was never used.

Please review and/or voice objection if some of that is till needed for
something.

Regards,
Christian.

[PATCH 1/4] dma-buf: fix incorrect dma-fence documentation

[Christian König]

There isn't much worse than documentation giving an incorrect advise.
Grabbing a spinlock while interrupts are disabled usually means that you
must also disable interrupts for all other uses of this spinlock.

Otherwise really hard to debug issues can occur. So fix that invalid
documentation.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 include/linux/dma-fence.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index e7ad819962e3..e230af0d123f 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -169,8 +169,8 @@ struct dma_fence_ops {
 	 * implementation know that there is another driver waiting on the
 	 * signal (ie. hw->sw case).
 	 *
-	 * This function can be called from atomic context, but not
-	 * from irq context, so normal spinlocks can be used.
+	 * This is called with irq's disabled, so only spinlocks which also
+	 * disable irq's can be used.
 	 *
 	 * A return value of false indicates the fence already passed,
 	 * or some failure occurred that made it impossible to enable
-- 
2.34.1

Re: [PATCH 1/4] dma-buf: fix incorrect dma-fence documentation

[Simona Vetter]

On Tue, Feb 11, 2025 at 05:31:06PM +0100, Christian König wrote:
> There isn't much worse than documentation giving an incorrect advise.
> Grabbing a spinlock while interrupts are disabled usually means that you
> must also disable interrupts for all other uses of this spinlock.
> 
> Otherwise really hard to debug issues can occur. So fix that invalid
> documentation.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Oops :-/

Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>

> ---
>  include/linux/dma-fence.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index e7ad819962e3..e230af0d123f 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -169,8 +169,8 @@ struct dma_fence_ops {
>  	 * implementation know that there is another driver waiting on the
>  	 * signal (ie. hw->sw case).
>  	 *
> -	 * This function can be called from atomic context, but not
> -	 * from irq context, so normal spinlocks can be used.
> +	 * This is called with irq's disabled, so only spinlocks which also
> +	 * disable irq's can be used.
>  	 *
>  	 * A return value of false indicates the fence already passed,
>  	 * or some failure occurred that made it impossible to enable
> -- 
> 2.34.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] dma-buf: fix incorrect dma-fence documentation

[Dmitry Osipenko]

On 2/11/25 19:31, Christian König wrote:
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index e7ad819962e3..e230af0d123f 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -169,8 +169,8 @@ struct dma_fence_ops {
>  	 * implementation know that there is another driver waiting on the
>  	 * signal (ie. hw->sw case).
>  	 *
> -	 * This function can be called from atomic context, but not
> -	 * from irq context, so normal spinlocks can be used.
> +	 * This is called with irq's disabled, so only spinlocks which also
> +	 * disable irq's can be used.

Nit:

The description sounds a bit cryptic to me. I'd add ".. so only
spinlocks which disable IRQ's can be used in the code outside of this
callback".

Note that I removed the word 'also' because disabling IRQs isn't
necessary for a spinlock taken within interrupt context because IRQs are
already disabled there.

-- 
Best regards,
Dmitry

[PATCH 2/4] dma-buf/dma-fence: remove unnecessary callbacks

[Christian König]

The fence_value_str and timeline_value_str callbacks were just an
unnecessary abstraction in the SW sync implementation.

The only caller of those callbacks already knew that the fence in
questions is a timeline_fence. So print the values directly instead
of using a redirection.

Additional to that remove the implementations from virtgpu and vgem.
As far as I can see those were never used in the first place.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/sw_sync.c              | 16 ----------------
 drivers/dma-buf/sync_debug.c           | 21 ++-------------------
 drivers/gpu/drm/vgem/vgem_fence.c      | 15 ---------------
 drivers/gpu/drm/virtio/virtgpu_fence.c | 16 ----------------
 include/linux/dma-fence.h              | 21 ---------------------
 5 files changed, 2 insertions(+), 87 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index f5905d67dedb..849280ae79a9 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -173,20 +173,6 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
 	return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
 }
 
-static void timeline_fence_value_str(struct dma_fence *fence,
-				    char *str, int size)
-{
-	snprintf(str, size, "%lld", fence->seqno);
-}
-
-static void timeline_fence_timeline_value_str(struct dma_fence *fence,
-					     char *str, int size)
-{
-	struct sync_timeline *parent = dma_fence_parent(fence);
-
-	snprintf(str, size, "%d", parent->value);
-}
-
 static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
 {
 	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
@@ -208,8 +194,6 @@ static const struct dma_fence_ops timeline_fence_ops = {
 	.get_timeline_name = timeline_fence_get_timeline_name,
 	.signaled = timeline_fence_signaled,
 	.release = timeline_fence_release,
-	.fence_value_str = timeline_fence_value_str,
-	.timeline_value_str = timeline_fence_timeline_value_str,
 	.set_deadline = timeline_fence_set_deadline,
 };
 
diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
index 237bce21d1e7..270daae7d89a 100644
--- a/drivers/dma-buf/sync_debug.c
+++ b/drivers/dma-buf/sync_debug.c
@@ -82,25 +82,8 @@ static void sync_print_fence(struct seq_file *s,
 		seq_printf(s, "@%lld.%09ld", (s64)ts64.tv_sec, ts64.tv_nsec);
 	}
 
-	if (fence->ops->timeline_value_str &&
-		fence->ops->fence_value_str) {
-		char value[64];
-		bool success;
-
-		fence->ops->fence_value_str(fence, value, sizeof(value));
-		success = strlen(value);
-
-		if (success) {
-			seq_printf(s, ": %s", value);
-
-			fence->ops->timeline_value_str(fence, value,
-						       sizeof(value));
-
-			if (strlen(value))
-				seq_printf(s, " / %s", value);
-		}
-	}
-
+	seq_printf(s, ": %lld", fence->seqno);
+	seq_printf(s, " / %d", parent->value);
 	seq_putc(s, '\n');
 }
 
diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c
index e15754178395..5298d995faa7 100644
--- a/drivers/gpu/drm/vgem/vgem_fence.c
+++ b/drivers/gpu/drm/vgem/vgem_fence.c
@@ -53,25 +53,10 @@ static void vgem_fence_release(struct dma_fence *base)
 	dma_fence_free(&fence->base);
 }
 
-static void vgem_fence_value_str(struct dma_fence *fence, char *str, int size)
-{
-	snprintf(str, size, "%llu", fence->seqno);
-}
-
-static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str,
-					  int size)
-{
-	snprintf(str, size, "%llu",
-		 dma_fence_is_signaled(fence) ? fence->seqno : 0);
-}
-
 static const struct dma_fence_ops vgem_fence_ops = {
 	.get_driver_name = vgem_fence_get_driver_name,
 	.get_timeline_name = vgem_fence_get_timeline_name,
 	.release = vgem_fence_release,
-
-	.fence_value_str = vgem_fence_value_str,
-	.timeline_value_str = vgem_fence_timeline_value_str,
 };
 
 static void vgem_fence_timeout(struct timer_list *t)
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index f28357dbde35..44c1d8ef3c4d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -49,26 +49,10 @@ static bool virtio_gpu_fence_signaled(struct dma_fence *f)
 	return false;
 }
 
-static void virtio_gpu_fence_value_str(struct dma_fence *f, char *str, int size)
-{
-	snprintf(str, size, "[%llu, %llu]", f->context, f->seqno);
-}
-
-static void virtio_gpu_timeline_value_str(struct dma_fence *f, char *str,
-					  int size)
-{
-	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
-
-	snprintf(str, size, "%llu",
-		 (u64)atomic64_read(&fence->drv->last_fence_id));
-}
-
 static const struct dma_fence_ops virtio_gpu_fence_ops = {
 	.get_driver_name     = virtio_gpu_get_driver_name,
 	.get_timeline_name   = virtio_gpu_get_timeline_name,
 	.signaled            = virtio_gpu_fence_signaled,
-	.fence_value_str     = virtio_gpu_fence_value_str,
-	.timeline_value_str  = virtio_gpu_timeline_value_str,
 };
 
 struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index e230af0d123f..8778e2d758da 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -238,27 +238,6 @@ struct dma_fence_ops {
 	 */
 	void (*release)(struct dma_fence *fence);
 
-	/**
-	 * @fence_value_str:
-	 *
-	 * Callback to fill in free-form debug info specific to this fence, like
-	 * the sequence number.
-	 *
-	 * This callback is optional.
-	 */
-	void (*fence_value_str)(struct dma_fence *fence, char *str, int size);
-
-	/**
-	 * @timeline_value_str:
-	 *
-	 * Fills in the current value of the timeline as a string, like the
-	 * sequence number. Note that the specific fence passed to this function
-	 * should not matter, drivers should only use it to look up the
-	 * corresponding timeline structures.
-	 */
-	void (*timeline_value_str)(struct dma_fence *fence,
-				   char *str, int size);
-
 	/**
 	 * @set_deadline:
 	 *
-- 
2.34.1

Re: [PATCH 2/4] dma-buf/dma-fence: remove unnecessary callbacks

[Simona Vetter]

On Tue, Feb 11, 2025 at 05:31:07PM +0100, Christian König wrote:
> The fence_value_str and timeline_value_str callbacks were just an
> unnecessary abstraction in the SW sync implementation.
> 
> The only caller of those callbacks already knew that the fence in
> questions is a timeline_fence. So print the values directly instead
> of using a redirection.
> 
> Additional to that remove the implementations from virtgpu and vgem.
> As far as I can see those were never used in the first place.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Ok I got really confused here for a moment, but checks all out.

Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>

> ---
>  drivers/dma-buf/sw_sync.c              | 16 ----------------
>  drivers/dma-buf/sync_debug.c           | 21 ++-------------------
>  drivers/gpu/drm/vgem/vgem_fence.c      | 15 ---------------
>  drivers/gpu/drm/virtio/virtgpu_fence.c | 16 ----------------
>  include/linux/dma-fence.h              | 21 ---------------------
>  5 files changed, 2 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index f5905d67dedb..849280ae79a9 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -173,20 +173,6 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
>  	return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
>  }
>  
> -static void timeline_fence_value_str(struct dma_fence *fence,
> -				    char *str, int size)
> -{
> -	snprintf(str, size, "%lld", fence->seqno);
> -}
> -
> -static void timeline_fence_timeline_value_str(struct dma_fence *fence,
> -					     char *str, int size)
> -{
> -	struct sync_timeline *parent = dma_fence_parent(fence);
> -
> -	snprintf(str, size, "%d", parent->value);
> -}
> -
>  static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
>  {
>  	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> @@ -208,8 +194,6 @@ static const struct dma_fence_ops timeline_fence_ops = {
>  	.get_timeline_name = timeline_fence_get_timeline_name,
>  	.signaled = timeline_fence_signaled,
>  	.release = timeline_fence_release,
> -	.fence_value_str = timeline_fence_value_str,
> -	.timeline_value_str = timeline_fence_timeline_value_str,
>  	.set_deadline = timeline_fence_set_deadline,
>  };
>  
> diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
> index 237bce21d1e7..270daae7d89a 100644
> --- a/drivers/dma-buf/sync_debug.c
> +++ b/drivers/dma-buf/sync_debug.c
> @@ -82,25 +82,8 @@ static void sync_print_fence(struct seq_file *s,
>  		seq_printf(s, "@%lld.%09ld", (s64)ts64.tv_sec, ts64.tv_nsec);
>  	}
>  
> -	if (fence->ops->timeline_value_str &&
> -		fence->ops->fence_value_str) {
> -		char value[64];
> -		bool success;
> -
> -		fence->ops->fence_value_str(fence, value, sizeof(value));
> -		success = strlen(value);
> -
> -		if (success) {
> -			seq_printf(s, ": %s", value);
> -
> -			fence->ops->timeline_value_str(fence, value,
> -						       sizeof(value));
> -
> -			if (strlen(value))
> -				seq_printf(s, " / %s", value);
> -		}
> -	}
> -
> +	seq_printf(s, ": %lld", fence->seqno);
> +	seq_printf(s, " / %d", parent->value);
>  	seq_putc(s, '\n');
>  }
>  
> diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c
> index e15754178395..5298d995faa7 100644
> --- a/drivers/gpu/drm/vgem/vgem_fence.c
> +++ b/drivers/gpu/drm/vgem/vgem_fence.c
> @@ -53,25 +53,10 @@ static void vgem_fence_release(struct dma_fence *base)
>  	dma_fence_free(&fence->base);
>  }
>  
> -static void vgem_fence_value_str(struct dma_fence *fence, char *str, int size)
> -{
> -	snprintf(str, size, "%llu", fence->seqno);
> -}
> -
> -static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str,
> -					  int size)
> -{
> -	snprintf(str, size, "%llu",
> -		 dma_fence_is_signaled(fence) ? fence->seqno : 0);
> -}
> -
>  static const struct dma_fence_ops vgem_fence_ops = {
>  	.get_driver_name = vgem_fence_get_driver_name,
>  	.get_timeline_name = vgem_fence_get_timeline_name,
>  	.release = vgem_fence_release,
> -
> -	.fence_value_str = vgem_fence_value_str,
> -	.timeline_value_str = vgem_fence_timeline_value_str,
>  };
>  
>  static void vgem_fence_timeout(struct timer_list *t)
> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
> index f28357dbde35..44c1d8ef3c4d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
> @@ -49,26 +49,10 @@ static bool virtio_gpu_fence_signaled(struct dma_fence *f)
>  	return false;
>  }
>  
> -static void virtio_gpu_fence_value_str(struct dma_fence *f, char *str, int size)
> -{
> -	snprintf(str, size, "[%llu, %llu]", f->context, f->seqno);
> -}
> -
> -static void virtio_gpu_timeline_value_str(struct dma_fence *f, char *str,
> -					  int size)
> -{
> -	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
> -
> -	snprintf(str, size, "%llu",
> -		 (u64)atomic64_read(&fence->drv->last_fence_id));
> -}
> -
>  static const struct dma_fence_ops virtio_gpu_fence_ops = {
>  	.get_driver_name     = virtio_gpu_get_driver_name,
>  	.get_timeline_name   = virtio_gpu_get_timeline_name,
>  	.signaled            = virtio_gpu_fence_signaled,
> -	.fence_value_str     = virtio_gpu_fence_value_str,
> -	.timeline_value_str  = virtio_gpu_timeline_value_str,
>  };
>  
>  struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index e230af0d123f..8778e2d758da 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -238,27 +238,6 @@ struct dma_fence_ops {
>  	 */
>  	void (*release)(struct dma_fence *fence);
>  
> -	/**
> -	 * @fence_value_str:
> -	 *
> -	 * Callback to fill in free-form debug info specific to this fence, like
> -	 * the sequence number.
> -	 *
> -	 * This callback is optional.
> -	 */
> -	void (*fence_value_str)(struct dma_fence *fence, char *str, int size);
> -
> -	/**
> -	 * @timeline_value_str:
> -	 *
> -	 * Fills in the current value of the timeline as a string, like the
> -	 * sequence number. Note that the specific fence passed to this function
> -	 * should not matter, drivers should only use it to look up the
> -	 * corresponding timeline structures.
> -	 */
> -	void (*timeline_value_str)(struct dma_fence *fence,
> -				   char *str, int size);
> -
>  	/**
>  	 * @set_deadline:
>  	 *
> -- 
> 2.34.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/4] dma-buf/dma-fence: remove unnecessary callbacks

[Dmitry Osipenko]

On 2/11/25 19:31, Christian König wrote:
> The fence_value_str and timeline_value_str callbacks were just an
> unnecessary abstraction in the SW sync implementation.
> 
> The only caller of those callbacks already knew that the fence in
> questions is a timeline_fence. So print the values directly instead
> of using a redirection.
> 
> Additional to that remove the implementations from virtgpu and vgem.
> As far as I can see those were never used in the first place.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/sw_sync.c              | 16 ----------------
>  drivers/dma-buf/sync_debug.c           | 21 ++-------------------
>  drivers/gpu/drm/vgem/vgem_fence.c      | 15 ---------------
>  drivers/gpu/drm/virtio/virtgpu_fence.c | 16 ----------------
>  include/linux/dma-fence.h              | 21 ---------------------
>  5 files changed, 2 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index f5905d67dedb..849280ae79a9 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -173,20 +173,6 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
>  	return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
>  }
>  
> -static void timeline_fence_value_str(struct dma_fence *fence,
> -				    char *str, int size)
> -{
> -	snprintf(str, size, "%lld", fence->seqno);
> -}
> -
> -static void timeline_fence_timeline_value_str(struct dma_fence *fence,
> -					     char *str, int size)
> -{
> -	struct sync_timeline *parent = dma_fence_parent(fence);
> -
> -	snprintf(str, size, "%d", parent->value);
> -}
> -
>  static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
>  {
>  	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> @@ -208,8 +194,6 @@ static const struct dma_fence_ops timeline_fence_ops = {
>  	.get_timeline_name = timeline_fence_get_timeline_name,
>  	.signaled = timeline_fence_signaled,
>  	.release = timeline_fence_release,
> -	.fence_value_str = timeline_fence_value_str,
> -	.timeline_value_str = timeline_fence_timeline_value_str,
>  	.set_deadline = timeline_fence_set_deadline,
>  };
>  
> diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
> index 237bce21d1e7..270daae7d89a 100644
> --- a/drivers/dma-buf/sync_debug.c
> +++ b/drivers/dma-buf/sync_debug.c
> @@ -82,25 +82,8 @@ static void sync_print_fence(struct seq_file *s,
>  		seq_printf(s, "@%lld.%09ld", (s64)ts64.tv_sec, ts64.tv_nsec);
>  	}
>  
> -	if (fence->ops->timeline_value_str &&
> -		fence->ops->fence_value_str) {
> -		char value[64];
> -		bool success;
> -
> -		fence->ops->fence_value_str(fence, value, sizeof(value));
> -		success = strlen(value);
> -
> -		if (success) {
> -			seq_printf(s, ": %s", value);
> -
> -			fence->ops->timeline_value_str(fence, value,
> -						       sizeof(value));
> -
> -			if (strlen(value))
> -				seq_printf(s, " / %s", value);
> -		}
> -	}
> -
> +	seq_printf(s, ": %lld", fence->seqno);
> +	seq_printf(s, " / %d", parent->value);
>  	seq_putc(s, '\n');
>  }
>  
> diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c
> index e15754178395..5298d995faa7 100644
> --- a/drivers/gpu/drm/vgem/vgem_fence.c
> +++ b/drivers/gpu/drm/vgem/vgem_fence.c
> @@ -53,25 +53,10 @@ static void vgem_fence_release(struct dma_fence *base)
>  	dma_fence_free(&fence->base);
>  }
>  
> -static void vgem_fence_value_str(struct dma_fence *fence, char *str, int size)
> -{
> -	snprintf(str, size, "%llu", fence->seqno);
> -}
> -
> -static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str,
> -					  int size)
> -{
> -	snprintf(str, size, "%llu",
> -		 dma_fence_is_signaled(fence) ? fence->seqno : 0);
> -}
> -
>  static const struct dma_fence_ops vgem_fence_ops = {
>  	.get_driver_name = vgem_fence_get_driver_name,
>  	.get_timeline_name = vgem_fence_get_timeline_name,
>  	.release = vgem_fence_release,
> -
> -	.fence_value_str = vgem_fence_value_str,
> -	.timeline_value_str = vgem_fence_timeline_value_str,
>  };
>  
>  static void vgem_fence_timeout(struct timer_list *t)
> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
> index f28357dbde35..44c1d8ef3c4d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
> @@ -49,26 +49,10 @@ static bool virtio_gpu_fence_signaled(struct dma_fence *f)
>  	return false;
>  }
>  
> -static void virtio_gpu_fence_value_str(struct dma_fence *f, char *str, int size)
> -{
> -	snprintf(str, size, "[%llu, %llu]", f->context, f->seqno);
> -}
> -
> -static void virtio_gpu_timeline_value_str(struct dma_fence *f, char *str,
> -					  int size)
> -{
> -	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
> -
> -	snprintf(str, size, "%llu",
> -		 (u64)atomic64_read(&fence->drv->last_fence_id));
> -}
> -
>  static const struct dma_fence_ops virtio_gpu_fence_ops = {
>  	.get_driver_name     = virtio_gpu_get_driver_name,
>  	.get_timeline_name   = virtio_gpu_get_timeline_name,
>  	.signaled            = virtio_gpu_fence_signaled,
> -	.fence_value_str     = virtio_gpu_fence_value_str,
> -	.timeline_value_str  = virtio_gpu_timeline_value_str,
>  };
>  
>  struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index e230af0d123f..8778e2d758da 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -238,27 +238,6 @@ struct dma_fence_ops {
>  	 */
>  	void (*release)(struct dma_fence *fence);
>  
> -	/**
> -	 * @fence_value_str:
> -	 *
> -	 * Callback to fill in free-form debug info specific to this fence, like
> -	 * the sequence number.
> -	 *
> -	 * This callback is optional.
> -	 */
> -	void (*fence_value_str)(struct dma_fence *fence, char *str, int size);
> -
> -	/**
> -	 * @timeline_value_str:
> -	 *
> -	 * Fills in the current value of the timeline as a string, like the
> -	 * sequence number. Note that the specific fence passed to this function
> -	 * should not matter, drivers should only use it to look up the
> -	 * corresponding timeline structures.
> -	 */
> -	void (*timeline_value_str)(struct dma_fence *fence,
> -				   char *str, int size);
> -
>  	/**
>  	 * @set_deadline:
>  	 *

Look okay. No sure if Google's perfetto makes use of the syncfile
debugfs and then this change may brake the userspace parser for
virtio-gpu, but debugfs isn't ABI and perfetto will adapt if needed.

+cc Rob Clark for visibility

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>


-- 
Best regards,
Dmitry

Re: [PATCH 2/4] dma-buf/dma-fence: remove unnecessary callbacks

[Dmitry Osipenko]

On 2/17/25 20:11, Dmitry Osipenko wrote:
> On 2/11/25 19:31, Christian König wrote:
>> The fence_value_str and timeline_value_str callbacks were just an
>> unnecessary abstraction in the SW sync implementation.
>>
>> The only caller of those callbacks already knew that the fence in
>> questions is a timeline_fence. So print the values directly instead
>> of using a redirection.
>>
>> Additional to that remove the implementations from virtgpu and vgem.
>> As far as I can see those were never used in the first place.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>  drivers/dma-buf/sw_sync.c              | 16 ----------------
>>  drivers/dma-buf/sync_debug.c           | 21 ++-------------------
>>  drivers/gpu/drm/vgem/vgem_fence.c      | 15 ---------------
>>  drivers/gpu/drm/virtio/virtgpu_fence.c | 16 ----------------
>>  include/linux/dma-fence.h              | 21 ---------------------
>>  5 files changed, 2 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
>> index f5905d67dedb..849280ae79a9 100644
>> --- a/drivers/dma-buf/sw_sync.c
>> +++ b/drivers/dma-buf/sw_sync.c
>> @@ -173,20 +173,6 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
>>  	return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops);
>>  }
>>  
>> -static void timeline_fence_value_str(struct dma_fence *fence,
>> -				    char *str, int size)
>> -{
>> -	snprintf(str, size, "%lld", fence->seqno);
>> -}
>> -
>> -static void timeline_fence_timeline_value_str(struct dma_fence *fence,
>> -					     char *str, int size)
>> -{
>> -	struct sync_timeline *parent = dma_fence_parent(fence);
>> -
>> -	snprintf(str, size, "%d", parent->value);
>> -}
>> -
>>  static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
>>  {
>>  	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
>> @@ -208,8 +194,6 @@ static const struct dma_fence_ops timeline_fence_ops = {
>>  	.get_timeline_name = timeline_fence_get_timeline_name,
>>  	.signaled = timeline_fence_signaled,
>>  	.release = timeline_fence_release,
>> -	.fence_value_str = timeline_fence_value_str,
>> -	.timeline_value_str = timeline_fence_timeline_value_str,
>>  	.set_deadline = timeline_fence_set_deadline,
>>  };
>>  
>> diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
>> index 237bce21d1e7..270daae7d89a 100644
>> --- a/drivers/dma-buf/sync_debug.c
>> +++ b/drivers/dma-buf/sync_debug.c
>> @@ -82,25 +82,8 @@ static void sync_print_fence(struct seq_file *s,
>>  		seq_printf(s, "@%lld.%09ld", (s64)ts64.tv_sec, ts64.tv_nsec);
>>  	}
>>  
>> -	if (fence->ops->timeline_value_str &&
>> -		fence->ops->fence_value_str) {
>> -		char value[64];
>> -		bool success;
>> -
>> -		fence->ops->fence_value_str(fence, value, sizeof(value));
>> -		success = strlen(value);
>> -
>> -		if (success) {
>> -			seq_printf(s, ": %s", value);
>> -
>> -			fence->ops->timeline_value_str(fence, value,
>> -						       sizeof(value));
>> -
>> -			if (strlen(value))
>> -				seq_printf(s, " / %s", value);
>> -		}
>> -	}
>> -
>> +	seq_printf(s, ": %lld", fence->seqno);
>> +	seq_printf(s, " / %d", parent->value);
>>  	seq_putc(s, '\n');
>>  }
>>  
>> diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c
>> index e15754178395..5298d995faa7 100644
>> --- a/drivers/gpu/drm/vgem/vgem_fence.c
>> +++ b/drivers/gpu/drm/vgem/vgem_fence.c
>> @@ -53,25 +53,10 @@ static void vgem_fence_release(struct dma_fence *base)
>>  	dma_fence_free(&fence->base);
>>  }
>>  
>> -static void vgem_fence_value_str(struct dma_fence *fence, char *str, int size)
>> -{
>> -	snprintf(str, size, "%llu", fence->seqno);
>> -}
>> -
>> -static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str,
>> -					  int size)
>> -{
>> -	snprintf(str, size, "%llu",
>> -		 dma_fence_is_signaled(fence) ? fence->seqno : 0);
>> -}
>> -
>>  static const struct dma_fence_ops vgem_fence_ops = {
>>  	.get_driver_name = vgem_fence_get_driver_name,
>>  	.get_timeline_name = vgem_fence_get_timeline_name,
>>  	.release = vgem_fence_release,
>> -
>> -	.fence_value_str = vgem_fence_value_str,
>> -	.timeline_value_str = vgem_fence_timeline_value_str,
>>  };
>>  
>>  static void vgem_fence_timeout(struct timer_list *t)
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
>> index f28357dbde35..44c1d8ef3c4d 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
>> @@ -49,26 +49,10 @@ static bool virtio_gpu_fence_signaled(struct dma_fence *f)
>>  	return false;
>>  }
>>  
>> -static void virtio_gpu_fence_value_str(struct dma_fence *f, char *str, int size)
>> -{
>> -	snprintf(str, size, "[%llu, %llu]", f->context, f->seqno);
>> -}
>> -
>> -static void virtio_gpu_timeline_value_str(struct dma_fence *f, char *str,
>> -					  int size)
>> -{
>> -	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
>> -
>> -	snprintf(str, size, "%llu",
>> -		 (u64)atomic64_read(&fence->drv->last_fence_id));
>> -}
>> -
>>  static const struct dma_fence_ops virtio_gpu_fence_ops = {
>>  	.get_driver_name     = virtio_gpu_get_driver_name,
>>  	.get_timeline_name   = virtio_gpu_get_timeline_name,
>>  	.signaled            = virtio_gpu_fence_signaled,
>> -	.fence_value_str     = virtio_gpu_fence_value_str,
>> -	.timeline_value_str  = virtio_gpu_timeline_value_str,
>>  };
>>  
>>  struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index e230af0d123f..8778e2d758da 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -238,27 +238,6 @@ struct dma_fence_ops {
>>  	 */
>>  	void (*release)(struct dma_fence *fence);
>>  
>> -	/**
>> -	 * @fence_value_str:
>> -	 *
>> -	 * Callback to fill in free-form debug info specific to this fence, like
>> -	 * the sequence number.
>> -	 *
>> -	 * This callback is optional.
>> -	 */
>> -	void (*fence_value_str)(struct dma_fence *fence, char *str, int size);
>> -
>> -	/**
>> -	 * @timeline_value_str:
>> -	 *
>> -	 * Fills in the current value of the timeline as a string, like the
>> -	 * sequence number. Note that the specific fence passed to this function
>> -	 * should not matter, drivers should only use it to look up the
>> -	 * corresponding timeline structures.
>> -	 */
>> -	void (*timeline_value_str)(struct dma_fence *fence,
>> -				   char *str, int size);
>> -
>>  	/**
>>  	 * @set_deadline:
>>  	 *
> 
> Look okay. No sure if Google's perfetto makes use of the syncfile
> debugfs and then this change may brake the userspace parser for
> virtio-gpu, but debugfs isn't ABI and perfetto will adapt if needed.
> 
> +cc Rob Clark for visibility
> 
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

and +Gurchetan Singh for even more visibility

-- 
Best regards,
Dmitry

[PATCH 3/4] dma-buf: dma-buf: stop mapping sg_tables on attach

[Christian König]

As a workaround to smoothly transit from static to dynamic DMA-buf
handling we cached the sg_table on attach if dynamic handling mismatched
between exporter and importer.

Since Dmitry and Thomas cleaned that up and also documented the lock
handling we can drop this workaround now.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c | 149 ++++++++++++++------------------------
 include/linux/dma-buf.h   |  14 ----
 2 files changed, 56 insertions(+), 107 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 5baa83b85515..357b94a3dbaa 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -782,7 +782,7 @@ static void mangle_sg_table(struct sg_table *sg_table)
 
 	/* To catch abuse of the underlying struct page by importers mix
 	 * up the bits, but take care to preserve the low SG_ bits to
-	 * not corrupt the sgt. The mixing is undone in __unmap_dma_buf
+	 * not corrupt the sgt. The mixing is undone on unmap
 	 * before passing the sgt back to the exporter.
 	 */
 	for_each_sgtable_sg(sg_table, sg, i)
@@ -790,29 +790,20 @@ static void mangle_sg_table(struct sg_table *sg_table)
 #endif
 
 }
-static struct sg_table *__map_dma_buf(struct dma_buf_attachment *attach,
-				       enum dma_data_direction direction)
-{
-	struct sg_table *sg_table;
-	signed long ret;
-
-	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
-	if (IS_ERR_OR_NULL(sg_table))
-		return sg_table;
-
-	if (!dma_buf_attachment_is_dynamic(attach)) {
-		ret = dma_resv_wait_timeout(attach->dmabuf->resv,
-					    DMA_RESV_USAGE_KERNEL, true,
-					    MAX_SCHEDULE_TIMEOUT);
-		if (ret < 0) {
-			attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
-							   direction);
-			return ERR_PTR(ret);
-		}
-	}
 
-	mangle_sg_table(sg_table);
-	return sg_table;
+/**
+ * dma_buf_pin_on_map - check if a DMA-buf should be pinned when mapped
+ * @attach: the DMA-buf attachment to check
+ *
+ * Returns: True if a DMA-buf export provided pin/unpin callbacks and we can't
+ * use the importers move notify for some reason.
+ */
+static bool
+dma_buf_pin_on_map(struct dma_buf_attachment *attach)
+{
+	return attach->dmabuf->ops->pin &&
+		(!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY) ||
+		 !attach->importer_ops);
 }
 
 /**
@@ -935,48 +926,11 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
 	list_add(&attach->node, &dmabuf->attachments);
 	dma_resv_unlock(dmabuf->resv);
 
-	/* When either the importer or the exporter can't handle dynamic
-	 * mappings we cache the mapping here to avoid issues with the
-	 * reservation object lock.
-	 */
-	if (dma_buf_attachment_is_dynamic(attach) !=
-	    dma_buf_is_dynamic(dmabuf)) {
-		struct sg_table *sgt;
-
-		dma_resv_lock(attach->dmabuf->resv, NULL);
-		if (dma_buf_is_dynamic(attach->dmabuf)) {
-			ret = dmabuf->ops->pin(attach);
-			if (ret)
-				goto err_unlock;
-		}
-
-		sgt = __map_dma_buf(attach, DMA_BIDIRECTIONAL);
-		if (!sgt)
-			sgt = ERR_PTR(-ENOMEM);
-		if (IS_ERR(sgt)) {
-			ret = PTR_ERR(sgt);
-			goto err_unpin;
-		}
-		dma_resv_unlock(attach->dmabuf->resv);
-		attach->sgt = sgt;
-		attach->dir = DMA_BIDIRECTIONAL;
-	}
-
 	return attach;
 
 err_attach:
 	kfree(attach);
 	return ERR_PTR(ret);
-
-err_unpin:
-	if (dma_buf_is_dynamic(attach->dmabuf))
-		dmabuf->ops->unpin(attach);
-
-err_unlock:
-	dma_resv_unlock(attach->dmabuf->resv);
-
-	dma_buf_detach(dmabuf, attach);
-	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_dynamic_attach, "DMA_BUF");
 
@@ -995,16 +949,6 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_attach, "DMA_BUF");
 
-static void __unmap_dma_buf(struct dma_buf_attachment *attach,
-			    struct sg_table *sg_table,
-			    enum dma_data_direction direction)
-{
-	/* uses XOR, hence this unmangles */
-	mangle_sg_table(sg_table);
-
-	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
-}
-
 /**
  * dma_buf_detach - Remove the given attachment from dmabuf's attachments list
  * @dmabuf:	[in]	buffer to detach from.
@@ -1022,11 +966,12 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
 	dma_resv_lock(dmabuf->resv, NULL);
 
 	if (attach->sgt) {
+		mangle_sg_table(attach->sgt);
+		attach->dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
+						   attach->dir);
 
-		__unmap_dma_buf(attach, attach->sgt, attach->dir);
-
-		if (dma_buf_is_dynamic(attach->dmabuf))
-			dmabuf->ops->unpin(attach);
+		if (dma_buf_pin_on_map(attach))
+			dma_buf_unpin(attach);
 	}
 	list_del(&attach->node);
 
@@ -1058,7 +1003,7 @@ int dma_buf_pin(struct dma_buf_attachment *attach)
 	struct dma_buf *dmabuf = attach->dmabuf;
 	int ret = 0;
 
-	WARN_ON(!dma_buf_attachment_is_dynamic(attach));
+	WARN_ON(!attach->importer_ops);
 
 	dma_resv_assert_held(dmabuf->resv);
 
@@ -1081,7 +1026,7 @@ void dma_buf_unpin(struct dma_buf_attachment *attach)
 {
 	struct dma_buf *dmabuf = attach->dmabuf;
 
-	WARN_ON(!dma_buf_attachment_is_dynamic(attach));
+	WARN_ON(!attach->importer_ops);
 
 	dma_resv_assert_held(dmabuf->resv);
 
@@ -1115,7 +1060,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 					enum dma_data_direction direction)
 {
 	struct sg_table *sg_table;
-	int r;
+	signed long ret;
 
 	might_sleep();
 
@@ -1136,29 +1081,37 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 		return attach->sgt;
 	}
 
-	if (dma_buf_is_dynamic(attach->dmabuf)) {
-		if (!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) {
-			r = attach->dmabuf->ops->pin(attach);
-			if (r)
-				return ERR_PTR(r);
-		}
+	if (dma_buf_pin_on_map(attach)) {
+		ret = attach->dmabuf->ops->pin(attach);
+		if (ret)
+			return ERR_PTR(ret);
 	}
 
-	sg_table = __map_dma_buf(attach, direction);
+	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
 	if (!sg_table)
 		sg_table = ERR_PTR(-ENOMEM);
+	if (IS_ERR(sg_table))
+		goto error_unpin;
 
-	if (IS_ERR(sg_table) && dma_buf_is_dynamic(attach->dmabuf) &&
-	     !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY))
-		attach->dmabuf->ops->unpin(attach);
+	/*
+	 * When not providing ops the importer doesn't wait for fences either.
+	 */
+	if (!attach->importer_ops) {
+		ret = dma_resv_wait_timeout(attach->dmabuf->resv,
+					    DMA_RESV_USAGE_KERNEL, true,
+					    MAX_SCHEDULE_TIMEOUT);
+		if (ret < 0)
+			goto error_unmap;
+	}
+	mangle_sg_table(sg_table);
 
-	if (!IS_ERR(sg_table) && attach->dmabuf->ops->cache_sgt_mapping) {
+	if (attach->dmabuf->ops->cache_sgt_mapping) {
 		attach->sgt = sg_table;
 		attach->dir = direction;
 	}
 
 #ifdef CONFIG_DMA_API_DEBUG
-	if (!IS_ERR(sg_table)) {
+	{
 		struct scatterlist *sg;
 		u64 addr;
 		int len;
@@ -1175,6 +1128,16 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 	}
 #endif /* CONFIG_DMA_API_DEBUG */
 	return sg_table;
+
+error_unmap:
+	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
+	sg_table = ERR_PTR(ret);
+
+error_unpin:
+	if (dma_buf_pin_on_map(attach))
+		attach->dmabuf->ops->unpin(attach);
+
+	return sg_table;
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment, "DMA_BUF");
 
@@ -1230,11 +1193,11 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
 	if (attach->sgt == sg_table)
 		return;
 
-	__unmap_dma_buf(attach, sg_table, direction);
+	mangle_sg_table(sg_table);
+	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
 
-	if (dma_buf_is_dynamic(attach->dmabuf) &&
-	    !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY))
-		dma_buf_unpin(attach);
+	if (dma_buf_pin_on_map(attach))
+		attach->dmabuf->ops->unpin(attach);
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment, "DMA_BUF");
 
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 36216d28d8bd..c54ff2dda8cb 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -583,20 +583,6 @@ static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf)
 	return !!dmabuf->ops->pin;
 }
 
-/**
- * dma_buf_attachment_is_dynamic - check if a DMA-buf attachment uses dynamic
- * mappings
- * @attach: the DMA-buf attachment to check
- *
- * Returns true if a DMA-buf importer wants to call the map/unmap functions with
- * the dma_resv lock held.
- */
-static inline bool
-dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
-{
-	return !!attach->importer_ops;
-}
-
 struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
 					  struct device *dev);
 struct dma_buf_attachment *
-- 
2.34.1

Re: [PATCH 3/4] dma-buf: dma-buf: stop mapping sg_tables on attach

[Simona Vetter]

On Tue, Feb 11, 2025 at 05:31:08PM +0100, Christian König wrote:
> As a workaround to smoothly transit from static to dynamic DMA-buf
> handling we cached the sg_table on attach if dynamic handling mismatched
> between exporter and importer.
> 
> Since Dmitry and Thomas cleaned that up and also documented the lock
> handling we can drop this workaround now.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/dma-buf.c | 149 ++++++++++++++------------------------
>  include/linux/dma-buf.h   |  14 ----
>  2 files changed, 56 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 5baa83b85515..357b94a3dbaa 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -782,7 +782,7 @@ static void mangle_sg_table(struct sg_table *sg_table)
>  
>  	/* To catch abuse of the underlying struct page by importers mix
>  	 * up the bits, but take care to preserve the low SG_ bits to
> -	 * not corrupt the sgt. The mixing is undone in __unmap_dma_buf
> +	 * not corrupt the sgt. The mixing is undone on unmap
>  	 * before passing the sgt back to the exporter.
>  	 */
>  	for_each_sgtable_sg(sg_table, sg, i)
> @@ -790,29 +790,20 @@ static void mangle_sg_table(struct sg_table *sg_table)
>  #endif
>  
>  }
> -static struct sg_table *__map_dma_buf(struct dma_buf_attachment *attach,
> -				       enum dma_data_direction direction)
> -{
> -	struct sg_table *sg_table;
> -	signed long ret;
> -
> -	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> -	if (IS_ERR_OR_NULL(sg_table))
> -		return sg_table;
> -
> -	if (!dma_buf_attachment_is_dynamic(attach)) {
> -		ret = dma_resv_wait_timeout(attach->dmabuf->resv,
> -					    DMA_RESV_USAGE_KERNEL, true,
> -					    MAX_SCHEDULE_TIMEOUT);
> -		if (ret < 0) {
> -			attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> -							   direction);
> -			return ERR_PTR(ret);
> -		}
> -	}
>  
> -	mangle_sg_table(sg_table);
> -	return sg_table;
> +/**
> + * dma_buf_pin_on_map - check if a DMA-buf should be pinned when mapped
> + * @attach: the DMA-buf attachment to check

Generally we don't do kerneldoc for static helper functions, the function
name should be clear enough here. I think you can just delete this.

> + *
> + * Returns: True if a DMA-buf export provided pin/unpin callbacks and we can't
> + * use the importers move notify for some reason.
> + */
> +static bool
> +dma_buf_pin_on_map(struct dma_buf_attachment *attach)
> +{
> +	return attach->dmabuf->ops->pin &&
> +		(!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY) ||
> +		 !attach->importer_ops);

I think moving the dma_buf_attachment_is_dynamic helper into this file
right above as a static inline helper without kerneldoc would be good,
just as a piece of self-documentation of what this check here means. It's
a bit opaque otherwise, and if we add more importer_ops we might screw
this up otherwise.

>  }
>  
>  /**
> @@ -935,48 +926,11 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
>  	list_add(&attach->node, &dmabuf->attachments);
>  	dma_resv_unlock(dmabuf->resv);
>  
> -	/* When either the importer or the exporter can't handle dynamic
> -	 * mappings we cache the mapping here to avoid issues with the
> -	 * reservation object lock.
> -	 */
> -	if (dma_buf_attachment_is_dynamic(attach) !=
> -	    dma_buf_is_dynamic(dmabuf)) {
> -		struct sg_table *sgt;
> -
> -		dma_resv_lock(attach->dmabuf->resv, NULL);
> -		if (dma_buf_is_dynamic(attach->dmabuf)) {
> -			ret = dmabuf->ops->pin(attach);
> -			if (ret)
> -				goto err_unlock;
> -		}
> -
> -		sgt = __map_dma_buf(attach, DMA_BIDIRECTIONAL);
> -		if (!sgt)
> -			sgt = ERR_PTR(-ENOMEM);
> -		if (IS_ERR(sgt)) {
> -			ret = PTR_ERR(sgt);
> -			goto err_unpin;
> -		}
> -		dma_resv_unlock(attach->dmabuf->resv);
> -		attach->sgt = sgt;
> -		attach->dir = DMA_BIDIRECTIONAL;
> -	}
> -
>  	return attach;
>  
>  err_attach:
>  	kfree(attach);
>  	return ERR_PTR(ret);
> -
> -err_unpin:
> -	if (dma_buf_is_dynamic(attach->dmabuf))
> -		dmabuf->ops->unpin(attach);
> -
> -err_unlock:
> -	dma_resv_unlock(attach->dmabuf->resv);
> -
> -	dma_buf_detach(dmabuf, attach);
> -	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_NS_GPL(dma_buf_dynamic_attach, "DMA_BUF");
>  
> @@ -995,16 +949,6 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>  }
>  EXPORT_SYMBOL_NS_GPL(dma_buf_attach, "DMA_BUF");
>  
> -static void __unmap_dma_buf(struct dma_buf_attachment *attach,
> -			    struct sg_table *sg_table,
> -			    enum dma_data_direction direction)
> -{
> -	/* uses XOR, hence this unmangles */
> -	mangle_sg_table(sg_table);
> -
> -	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
> -}
> -
>  /**
>   * dma_buf_detach - Remove the given attachment from dmabuf's attachments list
>   * @dmabuf:	[in]	buffer to detach from.
> @@ -1022,11 +966,12 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>  	dma_resv_lock(dmabuf->resv, NULL);
>  
>  	if (attach->sgt) {
> +		mangle_sg_table(attach->sgt);
> +		attach->dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
> +						   attach->dir);
>  
> -		__unmap_dma_buf(attach, attach->sgt, attach->dir);
> -
> -		if (dma_buf_is_dynamic(attach->dmabuf))
> -			dmabuf->ops->unpin(attach);
> +		if (dma_buf_pin_on_map(attach))
> +			dma_buf_unpin(attach);
>  	}
>  	list_del(&attach->node);
>  
> @@ -1058,7 +1003,7 @@ int dma_buf_pin(struct dma_buf_attachment *attach)
>  	struct dma_buf *dmabuf = attach->dmabuf;
>  	int ret = 0;
>  
> -	WARN_ON(!dma_buf_attachment_is_dynamic(attach));
> +	WARN_ON(!attach->importer_ops);
>  
>  	dma_resv_assert_held(dmabuf->resv);
>  
> @@ -1081,7 +1026,7 @@ void dma_buf_unpin(struct dma_buf_attachment *attach)
>  {
>  	struct dma_buf *dmabuf = attach->dmabuf;
>  
> -	WARN_ON(!dma_buf_attachment_is_dynamic(attach));
> +	WARN_ON(!attach->importer_ops);
>  
>  	dma_resv_assert_held(dmabuf->resv);
>  
> @@ -1115,7 +1060,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  					enum dma_data_direction direction)
>  {
>  	struct sg_table *sg_table;
> -	int r;
> +	signed long ret;
>  
>  	might_sleep();
>  
> @@ -1136,29 +1081,37 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  		return attach->sgt;
>  	}
>  
> -	if (dma_buf_is_dynamic(attach->dmabuf)) {
> -		if (!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) {
> -			r = attach->dmabuf->ops->pin(attach);
> -			if (r)
> -				return ERR_PTR(r);
> -		}
> +	if (dma_buf_pin_on_map(attach)) {
> +		ret = attach->dmabuf->ops->pin(attach);
> +		if (ret)

I do wonder whether we should have a WARN_ON(ret = -EBUSY) or similar, to
catch drivers who've moved the memory to an unsuitable place despite
attachments existing?

> +			return ERR_PTR(ret);
>  	}
>  
> -	sg_table = __map_dma_buf(attach, direction);

> +	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
>  	if (!sg_table)
>  		sg_table = ERR_PTR(-ENOMEM);
> +	if (IS_ERR(sg_table))
> +		goto error_unpin;
>  
> -	if (IS_ERR(sg_table) && dma_buf_is_dynamic(attach->dmabuf) &&
> -	     !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY))
> -		attach->dmabuf->ops->unpin(attach);
> +	/*
> +	 * When not providing ops the importer doesn't wait for fences either.
> +	 */
> +	if (!attach->importer_ops) {

Yeah we definitely want to keep this static helper function to make this
check less opaque. Also I think this is strictly speaking only needed for
the dma_buf_pin_on_map case and not for everyone, plus there shouldn't be
any harm to do this after pinning, but before calling map_dma_buf. But
maybe better to leave as-is.

> +		ret = dma_resv_wait_timeout(attach->dmabuf->resv,
> +					    DMA_RESV_USAGE_KERNEL, true,
> +					    MAX_SCHEDULE_TIMEOUT);
> +		if (ret < 0)
> +			goto error_unmap;
> +	}
> +	mangle_sg_table(sg_table);
>  
> -	if (!IS_ERR(sg_table) && attach->dmabuf->ops->cache_sgt_mapping) {
> +	if (attach->dmabuf->ops->cache_sgt_mapping) {
>  		attach->sgt = sg_table;
>  		attach->dir = direction;
>  	}
>  
>  #ifdef CONFIG_DMA_API_DEBUG
> -	if (!IS_ERR(sg_table)) {
> +	{
>  		struct scatterlist *sg;
>  		u64 addr;
>  		int len;
> @@ -1175,6 +1128,16 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  	}
>  #endif /* CONFIG_DMA_API_DEBUG */
>  	return sg_table;
> +
> +error_unmap:
> +	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
> +	sg_table = ERR_PTR(ret);
> +
> +error_unpin:
> +	if (dma_buf_pin_on_map(attach))
> +		attach->dmabuf->ops->unpin(attach);
> +
> +	return sg_table;
>  }
>  EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment, "DMA_BUF");
>  
> @@ -1230,11 +1193,11 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>  	if (attach->sgt == sg_table)
>  		return;
>  
> -	__unmap_dma_buf(attach, sg_table, direction);
> +	mangle_sg_table(sg_table);
> +	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
>  
> -	if (dma_buf_is_dynamic(attach->dmabuf) &&
> -	    !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY))
> -		dma_buf_unpin(attach);
> +	if (dma_buf_pin_on_map(attach))
> +		attach->dmabuf->ops->unpin(attach);
>  }
>  EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment, "DMA_BUF");
>  
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 36216d28d8bd..c54ff2dda8cb 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -583,20 +583,6 @@ static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf)
>  	return !!dmabuf->ops->pin;
>  }
>  
> -/**
> - * dma_buf_attachment_is_dynamic - check if a DMA-buf attachment uses dynamic
> - * mappings
> - * @attach: the DMA-buf attachment to check
> - *
> - * Returns true if a DMA-buf importer wants to call the map/unmap functions with
> - * the dma_resv lock held.

Yeah this kerneldoc is a bit much outdated :-)

> - */
> -static inline bool
> -dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
> -{
> -	return !!attach->importer_ops;
> -}

With the nits addressed:

Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>

Cheers, Sima


> -
>  struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>  					  struct device *dev);
>  struct dma_buf_attachment *
> -- 
> 2.34.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/4] dma-buf: dma-buf: stop mapping sg_tables on attach

[Dmitry Osipenko]

On 2/11/25 19:31, Christian König wrote:
> As a workaround to smoothly transit from static to dynamic DMA-buf
> handling we cached the sg_table on attach if dynamic handling mismatched
> between exporter and importer.
> 
> Since Dmitry and Thomas cleaned that up and also documented the lock
> handling we can drop this workaround now.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/dma-buf.c | 149 ++++++++++++++------------------------
>  include/linux/dma-buf.h   |  14 ----
>  2 files changed, 56 insertions(+), 107 deletions(-)

Looks good, as long as IGT passes.

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

[PATCH 4/4] dma-buf: drop caching of sg_tables

[Christian König]

That was purely for the transition from static to dynamic dma-buf
handling and can be removed again now.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c              | 34 --------------------------
 drivers/dma-buf/udmabuf.c              |  1 -
 drivers/gpu/drm/drm_prime.c            |  1 -
 drivers/gpu/drm/virtio/virtgpu_prime.c |  1 -
 include/linux/dma-buf.h                | 13 ----------
 5 files changed, 50 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 357b94a3dbaa..35221c4ddbf5 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -636,10 +636,6 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
 		    || !exp_info->ops->release))
 		return ERR_PTR(-EINVAL);
 
-	if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
-		    (exp_info->ops->pin || exp_info->ops->unpin)))
-		return ERR_PTR(-EINVAL);
-
 	if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
 		return ERR_PTR(-EINVAL);
 
@@ -964,17 +960,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
 		return;
 
 	dma_resv_lock(dmabuf->resv, NULL);
-
-	if (attach->sgt) {
-		mangle_sg_table(attach->sgt);
-		attach->dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
-						   attach->dir);
-
-		if (dma_buf_pin_on_map(attach))
-			dma_buf_unpin(attach);
-	}
 	list_del(&attach->node);
-
 	dma_resv_unlock(dmabuf->resv);
 
 	if (dmabuf->ops->detach)
@@ -1069,18 +1055,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 
 	dma_resv_assert_held(attach->dmabuf->resv);
 
-	if (attach->sgt) {
-		/*
-		 * Two mappings with different directions for the same
-		 * attachment are not allowed.
-		 */
-		if (attach->dir != direction &&
-		    attach->dir != DMA_BIDIRECTIONAL)
-			return ERR_PTR(-EBUSY);
-
-		return attach->sgt;
-	}
-
 	if (dma_buf_pin_on_map(attach)) {
 		ret = attach->dmabuf->ops->pin(attach);
 		if (ret)
@@ -1105,11 +1079,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 	}
 	mangle_sg_table(sg_table);
 
-	if (attach->dmabuf->ops->cache_sgt_mapping) {
-		attach->sgt = sg_table;
-		attach->dir = direction;
-	}
-
 #ifdef CONFIG_DMA_API_DEBUG
 	{
 		struct scatterlist *sg;
@@ -1190,9 +1159,6 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
 
 	dma_resv_assert_held(attach->dmabuf->resv);
 
-	if (attach->sgt == sg_table)
-		return;
-
 	mangle_sg_table(sg_table);
 	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
 
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index cc7398cc17d6..2fa2c9135eac 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -285,7 +285,6 @@ static int end_cpu_udmabuf(struct dma_buf *buf,
 }
 
 static const struct dma_buf_ops udmabuf_ops = {
-	.cache_sgt_mapping = true,
 	.map_dma_buf	   = map_udmabuf,
 	.unmap_dma_buf	   = unmap_udmabuf,
 	.release	   = release_udmabuf,
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 32a8781cfd67..c284f306d597 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -810,7 +810,6 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
 EXPORT_SYMBOL(drm_gem_dmabuf_mmap);
 
 static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
-	.cache_sgt_mapping = true,
 	.attach = drm_gem_map_attach,
 	.detach = drm_gem_map_detach,
 	.map_dma_buf = drm_gem_map_dma_buf,
diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
index fe6a0b018571..c6f3be3cb914 100644
--- a/drivers/gpu/drm/virtio/virtgpu_prime.c
+++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
@@ -75,7 +75,6 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
 
 static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
 	.ops = {
-		.cache_sgt_mapping = true,
 		.attach = virtio_dma_buf_attach,
 		.detach = drm_gem_map_detach,
 		.map_dma_buf = virtgpu_gem_map_dma_buf,
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index c54ff2dda8cb..544f8f8c3f44 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -34,15 +34,6 @@ struct dma_buf_attachment;
  * @vunmap: [optional] unmaps a vmap from the buffer
  */
 struct dma_buf_ops {
-	/**
-	  * @cache_sgt_mapping:
-	  *
-	  * If true the framework will cache the first mapping made for each
-	  * attachment. This avoids creating mappings for attachments multiple
-	  * times.
-	  */
-	bool cache_sgt_mapping;
-
 	/**
 	 * @attach:
 	 *
@@ -493,8 +484,6 @@ struct dma_buf_attach_ops {
  * @dmabuf: buffer for this attachment.
  * @dev: device attached to the buffer.
  * @node: list of dma_buf_attachment, protected by dma_resv lock of the dmabuf.
- * @sgt: cached mapping.
- * @dir: direction of cached mapping.
  * @peer2peer: true if the importer can handle peer resources without pages.
  * @priv: exporter specific attachment data.
  * @importer_ops: importer operations for this attachment, if provided
@@ -514,8 +503,6 @@ struct dma_buf_attachment {
 	struct dma_buf *dmabuf;
 	struct device *dev;
 	struct list_head node;
-	struct sg_table *sgt;
-	enum dma_data_direction dir;
 	bool peer2peer;
 	const struct dma_buf_attach_ops *importer_ops;
 	void *importer_priv;
-- 
2.34.1

Re: [PATCH 4/4] dma-buf: drop caching of sg_tables

[Simona Vetter]

On Tue, Feb 11, 2025 at 05:31:09PM +0100, Christian König wrote:
> That was purely for the transition from static to dynamic dma-buf
> handling and can be removed again now.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Yay!

Might uncover some fun if people have meanwhile started to rely on this
for perf or something. But we'll figure that out when it happens.

Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>


> ---
>  drivers/dma-buf/dma-buf.c              | 34 --------------------------
>  drivers/dma-buf/udmabuf.c              |  1 -
>  drivers/gpu/drm/drm_prime.c            |  1 -
>  drivers/gpu/drm/virtio/virtgpu_prime.c |  1 -
>  include/linux/dma-buf.h                | 13 ----------
>  5 files changed, 50 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 357b94a3dbaa..35221c4ddbf5 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -636,10 +636,6 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
>  		    || !exp_info->ops->release))
>  		return ERR_PTR(-EINVAL);
>  
> -	if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
> -		    (exp_info->ops->pin || exp_info->ops->unpin)))
> -		return ERR_PTR(-EINVAL);
> -
>  	if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
>  		return ERR_PTR(-EINVAL);
>  
> @@ -964,17 +960,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>  		return;
>  
>  	dma_resv_lock(dmabuf->resv, NULL);
> -
> -	if (attach->sgt) {
> -		mangle_sg_table(attach->sgt);
> -		attach->dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
> -						   attach->dir);
> -
> -		if (dma_buf_pin_on_map(attach))
> -			dma_buf_unpin(attach);
> -	}
>  	list_del(&attach->node);
> -
>  	dma_resv_unlock(dmabuf->resv);
>  
>  	if (dmabuf->ops->detach)
> @@ -1069,18 +1055,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  
>  	dma_resv_assert_held(attach->dmabuf->resv);
>  
> -	if (attach->sgt) {
> -		/*
> -		 * Two mappings with different directions for the same
> -		 * attachment are not allowed.
> -		 */
> -		if (attach->dir != direction &&
> -		    attach->dir != DMA_BIDIRECTIONAL)
> -			return ERR_PTR(-EBUSY);
> -
> -		return attach->sgt;
> -	}
> -
>  	if (dma_buf_pin_on_map(attach)) {
>  		ret = attach->dmabuf->ops->pin(attach);
>  		if (ret)
> @@ -1105,11 +1079,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  	}
>  	mangle_sg_table(sg_table);
>  
> -	if (attach->dmabuf->ops->cache_sgt_mapping) {
> -		attach->sgt = sg_table;
> -		attach->dir = direction;
> -	}
> -
>  #ifdef CONFIG_DMA_API_DEBUG
>  	{
>  		struct scatterlist *sg;
> @@ -1190,9 +1159,6 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>  
>  	dma_resv_assert_held(attach->dmabuf->resv);
>  
> -	if (attach->sgt == sg_table)
> -		return;
> -
>  	mangle_sg_table(sg_table);
>  	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
>  
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index cc7398cc17d6..2fa2c9135eac 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -285,7 +285,6 @@ static int end_cpu_udmabuf(struct dma_buf *buf,
>  }
>  
>  static const struct dma_buf_ops udmabuf_ops = {
> -	.cache_sgt_mapping = true,
>  	.map_dma_buf	   = map_udmabuf,
>  	.unmap_dma_buf	   = unmap_udmabuf,
>  	.release	   = release_udmabuf,
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 32a8781cfd67..c284f306d597 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -810,7 +810,6 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>  EXPORT_SYMBOL(drm_gem_dmabuf_mmap);
>  
>  static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
> -	.cache_sgt_mapping = true,
>  	.attach = drm_gem_map_attach,
>  	.detach = drm_gem_map_detach,
>  	.map_dma_buf = drm_gem_map_dma_buf,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index fe6a0b018571..c6f3be3cb914 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -75,7 +75,6 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>  
>  static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>  	.ops = {
> -		.cache_sgt_mapping = true,
>  		.attach = virtio_dma_buf_attach,
>  		.detach = drm_gem_map_detach,
>  		.map_dma_buf = virtgpu_gem_map_dma_buf,
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index c54ff2dda8cb..544f8f8c3f44 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -34,15 +34,6 @@ struct dma_buf_attachment;
>   * @vunmap: [optional] unmaps a vmap from the buffer
>   */
>  struct dma_buf_ops {
> -	/**
> -	  * @cache_sgt_mapping:
> -	  *
> -	  * If true the framework will cache the first mapping made for each
> -	  * attachment. This avoids creating mappings for attachments multiple
> -	  * times.
> -	  */
> -	bool cache_sgt_mapping;
> -
>  	/**
>  	 * @attach:
>  	 *
> @@ -493,8 +484,6 @@ struct dma_buf_attach_ops {
>   * @dmabuf: buffer for this attachment.
>   * @dev: device attached to the buffer.
>   * @node: list of dma_buf_attachment, protected by dma_resv lock of the dmabuf.
> - * @sgt: cached mapping.
> - * @dir: direction of cached mapping.
>   * @peer2peer: true if the importer can handle peer resources without pages.
>   * @priv: exporter specific attachment data.
>   * @importer_ops: importer operations for this attachment, if provided
> @@ -514,8 +503,6 @@ struct dma_buf_attachment {
>  	struct dma_buf *dmabuf;
>  	struct device *dev;
>  	struct list_head node;
> -	struct sg_table *sgt;
> -	enum dma_data_direction dir;
>  	bool peer2peer;
>  	const struct dma_buf_attach_ops *importer_ops;
>  	void *importer_priv;
> -- 
> 2.34.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 4/4] dma-buf: drop caching of sg_tables

[Christian König]

Am 17.02.25 um 17:28 schrieb Simona Vetter:
> On Tue, Feb 11, 2025 at 05:31:09PM +0100, Christian König wrote:
>> That was purely for the transition from static to dynamic dma-buf
>> handling and can be removed again now.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> Yay!
>
> Might uncover some fun if people have meanwhile started to rely on this
> for perf or something. But we'll figure that out when it happens.

Yeah that was exactly the reason why I made this a separate patch and didn't mixed it into patch #3.

On the other hand I'm fine to keep it a bit longer if anybody complains.

Christian.

>
> Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>
>
>
>> ---
>>  drivers/dma-buf/dma-buf.c              | 34 --------------------------
>>  drivers/dma-buf/udmabuf.c              |  1 -
>>  drivers/gpu/drm/drm_prime.c            |  1 -
>>  drivers/gpu/drm/virtio/virtgpu_prime.c |  1 -
>>  include/linux/dma-buf.h                | 13 ----------
>>  5 files changed, 50 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 357b94a3dbaa..35221c4ddbf5 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -636,10 +636,6 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
>>  		    || !exp_info->ops->release))
>>  		return ERR_PTR(-EINVAL);
>>  
>> -	if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
>> -		    (exp_info->ops->pin || exp_info->ops->unpin)))
>> -		return ERR_PTR(-EINVAL);
>> -
>>  	if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
>>  		return ERR_PTR(-EINVAL);
>>  
>> @@ -964,17 +960,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>>  		return;
>>  
>>  	dma_resv_lock(dmabuf->resv, NULL);
>> -
>> -	if (attach->sgt) {
>> -		mangle_sg_table(attach->sgt);
>> -		attach->dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
>> -						   attach->dir);
>> -
>> -		if (dma_buf_pin_on_map(attach))
>> -			dma_buf_unpin(attach);
>> -	}
>>  	list_del(&attach->node);
>> -
>>  	dma_resv_unlock(dmabuf->resv);
>>  
>>  	if (dmabuf->ops->detach)
>> @@ -1069,18 +1055,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>>  
>>  	dma_resv_assert_held(attach->dmabuf->resv);
>>  
>> -	if (attach->sgt) {
>> -		/*
>> -		 * Two mappings with different directions for the same
>> -		 * attachment are not allowed.
>> -		 */
>> -		if (attach->dir != direction &&
>> -		    attach->dir != DMA_BIDIRECTIONAL)
>> -			return ERR_PTR(-EBUSY);
>> -
>> -		return attach->sgt;
>> -	}
>> -
>>  	if (dma_buf_pin_on_map(attach)) {
>>  		ret = attach->dmabuf->ops->pin(attach);
>>  		if (ret)
>> @@ -1105,11 +1079,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>>  	}
>>  	mangle_sg_table(sg_table);
>>  
>> -	if (attach->dmabuf->ops->cache_sgt_mapping) {
>> -		attach->sgt = sg_table;
>> -		attach->dir = direction;
>> -	}
>> -
>>  #ifdef CONFIG_DMA_API_DEBUG
>>  	{
>>  		struct scatterlist *sg;
>> @@ -1190,9 +1159,6 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>>  
>>  	dma_resv_assert_held(attach->dmabuf->resv);
>>  
>> -	if (attach->sgt == sg_table)
>> -		return;
>> -
>>  	mangle_sg_table(sg_table);
>>  	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
>>  
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index cc7398cc17d6..2fa2c9135eac 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -285,7 +285,6 @@ static int end_cpu_udmabuf(struct dma_buf *buf,
>>  }
>>  
>>  static const struct dma_buf_ops udmabuf_ops = {
>> -	.cache_sgt_mapping = true,
>>  	.map_dma_buf	   = map_udmabuf,
>>  	.unmap_dma_buf	   = unmap_udmabuf,
>>  	.release	   = release_udmabuf,
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 32a8781cfd67..c284f306d597 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -810,7 +810,6 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>>  EXPORT_SYMBOL(drm_gem_dmabuf_mmap);
>>  
>>  static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>> -	.cache_sgt_mapping = true,
>>  	.attach = drm_gem_map_attach,
>>  	.detach = drm_gem_map_detach,
>>  	.map_dma_buf = drm_gem_map_dma_buf,
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
>> index fe6a0b018571..c6f3be3cb914 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
>> @@ -75,7 +75,6 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>>  
>>  static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>>  	.ops = {
>> -		.cache_sgt_mapping = true,
>>  		.attach = virtio_dma_buf_attach,
>>  		.detach = drm_gem_map_detach,
>>  		.map_dma_buf = virtgpu_gem_map_dma_buf,
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index c54ff2dda8cb..544f8f8c3f44 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -34,15 +34,6 @@ struct dma_buf_attachment;
>>   * @vunmap: [optional] unmaps a vmap from the buffer
>>   */
>>  struct dma_buf_ops {
>> -	/**
>> -	  * @cache_sgt_mapping:
>> -	  *
>> -	  * If true the framework will cache the first mapping made for each
>> -	  * attachment. This avoids creating mappings for attachments multiple
>> -	  * times.
>> -	  */
>> -	bool cache_sgt_mapping;
>> -
>>  	/**
>>  	 * @attach:
>>  	 *
>> @@ -493,8 +484,6 @@ struct dma_buf_attach_ops {
>>   * @dmabuf: buffer for this attachment.
>>   * @dev: device attached to the buffer.
>>   * @node: list of dma_buf_attachment, protected by dma_resv lock of the dmabuf.
>> - * @sgt: cached mapping.
>> - * @dir: direction of cached mapping.
>>   * @peer2peer: true if the importer can handle peer resources without pages.
>>   * @priv: exporter specific attachment data.
>>   * @importer_ops: importer operations for this attachment, if provided
>> @@ -514,8 +503,6 @@ struct dma_buf_attachment {
>>  	struct dma_buf *dmabuf;
>>  	struct device *dev;
>>  	struct list_head node;
>> -	struct sg_table *sgt;
>> -	enum dma_data_direction dir;
>>  	bool peer2peer;
>>  	const struct dma_buf_attach_ops *importer_ops;
>>  	void *importer_priv;
>> -- 
>> 2.34.1
>>

Re: [PATCH 4/4] dma-buf: drop caching of sg_tables

[Dmitry Osipenko]

On 2/11/25 19:31, Christian König wrote:
> That was purely for the transition from static to dynamic dma-buf
> handling and can be removed again now.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/dma-buf.c              | 34 --------------------------
>  drivers/dma-buf/udmabuf.c              |  1 -
>  drivers/gpu/drm/drm_prime.c            |  1 -
>  drivers/gpu/drm/virtio/virtgpu_prime.c |  1 -
>  include/linux/dma-buf.h                | 13 ----------
>  5 files changed, 50 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 357b94a3dbaa..35221c4ddbf5 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -636,10 +636,6 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
>  		    || !exp_info->ops->release))
>  		return ERR_PTR(-EINVAL);
>  
> -	if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
> -		    (exp_info->ops->pin || exp_info->ops->unpin)))
> -		return ERR_PTR(-EINVAL);
> -
>  	if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
>  		return ERR_PTR(-EINVAL);
>  
> @@ -964,17 +960,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>  		return;
>  
>  	dma_resv_lock(dmabuf->resv, NULL);
> -
> -	if (attach->sgt) {
> -		mangle_sg_table(attach->sgt);
> -		attach->dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
> -						   attach->dir);
> -
> -		if (dma_buf_pin_on_map(attach))
> -			dma_buf_unpin(attach);
> -	}
>  	list_del(&attach->node);
> -
>  	dma_resv_unlock(dmabuf->resv);
>  
>  	if (dmabuf->ops->detach)
> @@ -1069,18 +1055,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  
>  	dma_resv_assert_held(attach->dmabuf->resv);
>  
> -	if (attach->sgt) {
> -		/*
> -		 * Two mappings with different directions for the same
> -		 * attachment are not allowed.
> -		 */
> -		if (attach->dir != direction &&
> -		    attach->dir != DMA_BIDIRECTIONAL)
> -			return ERR_PTR(-EBUSY);
> -
> -		return attach->sgt;
> -	}
> -
>  	if (dma_buf_pin_on_map(attach)) {
>  		ret = attach->dmabuf->ops->pin(attach);
>  		if (ret)
> @@ -1105,11 +1079,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  	}
>  	mangle_sg_table(sg_table);
>  
> -	if (attach->dmabuf->ops->cache_sgt_mapping) {
> -		attach->sgt = sg_table;
> -		attach->dir = direction;
> -	}
> -
>  #ifdef CONFIG_DMA_API_DEBUG
>  	{
>  		struct scatterlist *sg;
> @@ -1190,9 +1159,6 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>  
>  	dma_resv_assert_held(attach->dmabuf->resv);
>  
> -	if (attach->sgt == sg_table)
> -		return;
> -
>  	mangle_sg_table(sg_table);
>  	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
>  
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index cc7398cc17d6..2fa2c9135eac 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -285,7 +285,6 @@ static int end_cpu_udmabuf(struct dma_buf *buf,
>  }
>  
>  static const struct dma_buf_ops udmabuf_ops = {
> -	.cache_sgt_mapping = true,
>  	.map_dma_buf	   = map_udmabuf,
>  	.unmap_dma_buf	   = unmap_udmabuf,
>  	.release	   = release_udmabuf,
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 32a8781cfd67..c284f306d597 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -810,7 +810,6 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>  EXPORT_SYMBOL(drm_gem_dmabuf_mmap);
>  
>  static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
> -	.cache_sgt_mapping = true,
>  	.attach = drm_gem_map_attach,
>  	.detach = drm_gem_map_detach,
>  	.map_dma_buf = drm_gem_map_dma_buf,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index fe6a0b018571..c6f3be3cb914 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -75,7 +75,6 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>  
>  static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>  	.ops = {
> -		.cache_sgt_mapping = true,
>  		.attach = virtio_dma_buf_attach,
>  		.detach = drm_gem_map_detach,
>  		.map_dma_buf = virtgpu_gem_map_dma_buf,
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index c54ff2dda8cb..544f8f8c3f44 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -34,15 +34,6 @@ struct dma_buf_attachment;
>   * @vunmap: [optional] unmaps a vmap from the buffer
>   */
>  struct dma_buf_ops {
> -	/**
> -	  * @cache_sgt_mapping:
> -	  *
> -	  * If true the framework will cache the first mapping made for each
> -	  * attachment. This avoids creating mappings for attachments multiple
> -	  * times.
> -	  */
> -	bool cache_sgt_mapping;
> -
>  	/**
>  	 * @attach:
>  	 *
> @@ -493,8 +484,6 @@ struct dma_buf_attach_ops {
>   * @dmabuf: buffer for this attachment.
>   * @dev: device attached to the buffer.
>   * @node: list of dma_buf_attachment, protected by dma_resv lock of the dmabuf.
> - * @sgt: cached mapping.
> - * @dir: direction of cached mapping.
>   * @peer2peer: true if the importer can handle peer resources without pages.
>   * @priv: exporter specific attachment data.
>   * @importer_ops: importer operations for this attachment, if provided
> @@ -514,8 +503,6 @@ struct dma_buf_attachment {
>  	struct dma_buf *dmabuf;
>  	struct device *dev;
>  	struct list_head node;
> -	struct sg_table *sgt;
> -	enum dma_data_direction dir;
>  	bool peer2peer;
>  	const struct dma_buf_attach_ops *importer_ops;
>  	void *importer_priv;

Recalling being very confused by this cached sgt many years ago when
faced it for the first time :)

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry