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.
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
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.
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: *
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
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
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 *
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
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;
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,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
linaro-mm-sig@lists.linaro.org
-
Christian König -
Dmitry Osipenko