Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

9 participants
3222 discussions

Re: [RFC v2 06/11] nvme-pci: add support for dmabuf reggistration

by Christoph Hellwig

Splitting this trivial stub from the substantial parts in the next patch feels odd. Please merge them. (and better commit logs and comments really would be useful for others to understand what you've done). > +const struct dma_buf_attach_ops nvme_dmabuf_importer_ops = { > + .move_notify = nvme_dmabuf_move_notify, > + .allow_peer2peer = true, > +}; Tab-align the =, please. > +static int nvme_init_dma_token(struct request_queue *q, > + struct blk_mq_dma_token *token) > +{ > + struct dma_buf_attachment *attach; > + struct nvme_ns *ns = q->queuedata; > + struct nvme_dev *dev = to_nvme_dev(ns->ctrl); > + struct dma_buf *dmabuf = token->dmabuf; > + > + if (dmabuf->size % NVME_CTRL_PAGE_SIZE) > + return -EINVAL; Why do you care about alignment to the controller page size? > + for_each_sgtable_dma_sg(sgt, sg, tmp) { > + dma_addr_t dma = sg_dma_address(sg); > + unsigned long sg_len = sg_dma_len(sg); > + > + while (sg_len) { > + dma_list[i++] = dma; > + dma += NVME_CTRL_PAGE_SIZE; > + sg_len -= NVME_CTRL_PAGE_SIZE; > + } > + } Why does this build controller pages sized chunks?

5 days, 18 hours

Re: [RFC v2 05/11] block: add infra to handle dmabuf tokens

by Christoph Hellwig

On Sun, Nov 23, 2025 at 10:51:25PM +0000, Pavel Begunkov wrote: > Add blk-mq infrastructure to handle dmabuf tokens. There are two main Please spell out infrastructure in the subject as well. > +struct dma_token *blkdev_dma_map(struct file *file, > + struct dma_token_params *params) > +{ > + struct request_queue *q = bdev_get_queue(file_bdev(file)); > + > + if (!(file->f_flags & O_DIRECT)) > + return ERR_PTR(-EINVAL); Shouldn't the O_DIRECT check be in the caller? > +++ b/block/blk-mq-dma-token.c Missing SPDX and Copyright statement. > @@ -0,0 +1,236 @@ > +#include <linux/blk-mq-dma-token.h> > +#include <linux/dma-resv.h> > + > +struct blk_mq_dma_fence { > + struct dma_fence base; > + spinlock_t lock; > +}; And a high-level comment explaining the fencing logic would be nice as well. > + struct blk_mq_dma_map *map = container_of(ref, struct blk_mq_dma_map, refs); Overly long line. > +static struct blk_mq_dma_map *blk_mq_alloc_dma_mapping(struct blk_mq_dma_token *token) Another one. Also kinda inconsistent between _map in the data structure and _mapping in the function name. > +static inline > +struct blk_mq_dma_map *blk_mq_get_token_map(struct blk_mq_dma_token *token) Really odd return value / scope formatting. > +{ > + struct blk_mq_dma_map *map; > + > + guard(rcu)(); > + > + map = rcu_dereference(token->map); > + if (unlikely(!map || !percpu_ref_tryget_live_rcu(&map->refs))) > + return NULL; > + return map; Please use good old rcu_read_unlock to make this readable. > + guard(mutex)(&token->mapping_lock); Same. > + > + map = blk_mq_get_token_map(token); > + if (map) > + return map; > + > + map = blk_mq_alloc_dma_mapping(token); > + if (IS_ERR(map)) > + return NULL; > + > + dma_resv_lock(dmabuf->resv, NULL); > + ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_BOOKKEEP, > + true, MAX_SCHEDULE_TIMEOUT); > + ret = ret ? ret : -ETIME; if (!ret) ret = -ETIME; > +blk_status_t blk_rq_assign_dma_map(struct request *rq, > + struct blk_mq_dma_token *token) > +{ > + struct blk_mq_dma_map *map; > + > + map = blk_mq_get_token_map(token); > + if (map) > + goto complete; > + > + if (rq->cmd_flags & REQ_NOWAIT) > + return BLK_STS_AGAIN; > + > + map = blk_mq_create_dma_map(token); > + if (IS_ERR(map)) > + return BLK_STS_RESOURCE; Having a few comments, that say this is creating the map lazily would probably helper the reader. Also why not keep the !map case in the branch, as the map case should be the fast path and thus usually be straight line in the function? > +void blk_mq_dma_map_move_notify(struct blk_mq_dma_token *token) > +{ > + blk_mq_dma_map_remove(token); > +} Is there a good reason for having this blk_mq_dma_map_move_notify wrapper? > + if (bio_flagged(bio, BIO_DMA_TOKEN)) { > + struct blk_mq_dma_token *token; > + blk_status_t ret; > + > + token = dma_token_to_blk_mq(bio->dma_token); > + ret = blk_rq_assign_dma_map(rq, token); > + if (ret) { > + if (ret == BLK_STS_AGAIN) { > + bio_wouldblock_error(bio); > + } else { > + bio->bi_status = BLK_STS_RESOURCE; > + bio_endio(bio); > + } > + goto queue_exit; > + } > + } Any reason to not just keep the dma_token_to_blk_mq? Also why is this overriding non-BLK_STS_AGAIN errors with BLK_STS_RESOURCE? (I really wish we could make all BLK_STS_AGAIN errors be quiet without the explicit setting of BIO_QUIET, which is a bit annoying, but that's not for this patch). > +static inline > +struct blk_mq_dma_token *dma_token_to_blk_mq(struct dma_token *token) More odd formatting.

5 days, 18 hours

Re: [RFC v2 04/11] block: introduce dma token backed bio type

by Christoph Hellwig

> diff --git a/block/bio.c b/block/bio.c > index 7b13bdf72de0..8793f1ee559d 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -843,6 +843,11 @@ static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp) > bio_clone_blkg_association(bio, bio_src); > } > > + if (bio_flagged(bio_src, BIO_DMA_TOKEN)) { > + bio->dma_token = bio_src->dma_token; > + bio_set_flag(bio, BIO_DMA_TOKEN); > + } Historically __bio_clone itself does not clone the payload, just the bio. But we got rid of the callers that want to clone a bio but not the payload long time ago. I'd suggest a prep patch that moves assigning bi_io_vec from bio_alloc_clone and bio_init_clone into __bio_clone, and given that they are the same field that'll take carw of the dma token as well. Alternatively do it in an if/else that the compiler will hopefully optimize away. > @@ -1349,6 +1366,10 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter, > bio_iov_bvec_set(bio, iter); > iov_iter_advance(iter, bio->bi_iter.bi_size); > return 0; > + } else if (iov_iter_is_dma_token(iter)) { No else after an return please. > +++ b/block/blk-merge.c > @@ -328,6 +328,29 @@ int bio_split_io_at(struct bio *bio, const struct queue_limits *lim, > unsigned nsegs = 0, bytes = 0, gaps = 0; > struct bvec_iter iter; > > + if (bio_flagged(bio, BIO_DMA_TOKEN)) { Please split the dmabuf logic into a self-contained helper here. > + int offset = offset_in_page(bio->bi_iter.bi_bvec_done); > + > + nsegs = ALIGN(bio->bi_iter.bi_size + offset, PAGE_SIZE); > + nsegs >>= PAGE_SHIFT; Why are we hardcoding PAGE_SIZE based "segments" here? > + > + if (offset & lim->dma_alignment || bytes & len_align_mask) > + return -EINVAL; > + > + if (bio->bi_iter.bi_size > max_bytes) { > + bytes = max_bytes; > + nsegs = (bytes + offset) >> PAGE_SHIFT; > + goto split; > + } else if (nsegs > lim->max_segments) { No else after a goto either.

5 days, 18 hours

Re: [RFC v2 03/11] block: move around bio flagging helpers

by Christoph Hellwig

On Sun, Nov 23, 2025 at 10:51:23PM +0000, Pavel Begunkov wrote: > We'll need bio_flagged() earlier in bio.h in the next patch, move it > together with all related helpers, and mark the bio_flagged()'s bio > argument as const. > > Signed-off-by: Pavel Begunkov <asml.silence(a)gmail.com> Looks good: Reviewed-by: Christoph Hellwig <hch(a)lst.de> Maybe ask Jens to queue it up ASAP to get it out of the way?

5 days, 18 hours

Re: [RFC v2 02/11] iov_iter: introduce iter type for pre-registered dma

by Christoph Hellwig

On Sun, Nov 23, 2025 at 10:51:22PM +0000, Pavel Begunkov wrote: > diff --git a/include/linux/uio.h b/include/linux/uio.h > index 5b127043a151..1b22594ca35b 100644 > --- a/include/linux/uio.h > +++ b/include/linux/uio.h > @@ -29,6 +29,7 @@ enum iter_type { > ITER_FOLIOQ, > ITER_XARRAY, > ITER_DISCARD, > + ITER_DMA_TOKEN, Please use DMABUF/dmabuf naming everywhere, this is about dmabufs and not dma in general. Otherwise this looks good.

5 days, 18 hours

Re: [RFC v2 01/11] file: add callback for pre-mapping dmabuf

by Christoph Hellwig

On Sun, Nov 23, 2025 at 10:51:21PM +0000, Pavel Begunkov wrote: > +static inline struct dma_token * > +dma_token_create(struct file *file, struct dma_token_params *params) > +{ > + struct dma_token *res; > + > + if (!file->f_op->dma_map) > + return ERR_PTR(-EOPNOTSUPP); > + res = file->f_op->dma_map(file, params); Calling the file operation ->dmap_map feels really misleading. create_token as in the function name is already much better, but it really is not just dma, but dmabuf related, and that should really be encoded in the name. Also why not pass the dmabuf and direction directly instead of wrapping it in the odd params struct making the whole thing hard to follow?

5 days, 18 hours

[PATCH v6 3/8] rust: drm: gem: Add raw_dma_resv() function

by Lyude Paul

For retrieving a pointer to the struct dma_resv for a given GEM object. We also introduce it in a new trait, BaseObjectPrivate, which we automatically implement for all gem objects and don't expose to users outside of the crate. Signed-off-by: Lyude Paul <lyude(a)redhat.com> --- rust/kernel/drm/gem/mod.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs index 5c215e83c1b09..ec3c1b1775196 100644 --- a/rust/kernel/drm/gem/mod.rs +++ b/rust/kernel/drm/gem/mod.rs @@ -199,6 +199,18 @@ fn create_mmap_offset(&self) -> Result<u64> { impl<T: IntoGEMObject> BaseObject for T {} +/// Crate-private base operations shared by all GEM object classes. +#[expect(unused)] +pub(crate) trait BaseObjectPrivate: IntoGEMObject { + /// Return a pointer to this object's dma_resv. + fn raw_dma_resv(&self) -> *mut bindings::dma_resv { + // SAFETY: `as_gem_obj()` always returns a valid pointer to the base DRM gem object + unsafe { (*self.as_raw()).resv } + } +} + +impl<T: IntoGEMObject> BaseObjectPrivate for T {} + /// A base GEM object. /// /// # Invariants -- 2.52.0

1 week

[PATCH v6 2/8] rust: helpers: Add bindings/wrappers for dma_resv_lock

by Lyude Paul

From: Asahi Lina <lina(a)asahilina.net> This is just for basic usage in the DRM shmem abstractions for implied locking, not intended as a full DMA Reservation abstraction yet. Signed-off-by: Asahi Lina <lina(a)asahilina.net> Signed-off-by: Daniel Almeida <daniel.almeida(a)collabora.com> Reviewed-by: Alice Ryhl <aliceryhl(a)google.com> Signed-off-by: Lyude Paul <lyude(a)redhat.com> --- rust/bindings/bindings_helper.h | 1 + rust/helpers/dma-resv.c | 13 +++++++++++++ rust/helpers/helpers.c | 1 + 3 files changed, 15 insertions(+) create mode 100644 rust/helpers/dma-resv.c diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 2e43c66635a2c..07f79e125c329 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -48,6 +48,7 @@ #include <linux/cpumask.h> #include <linux/cred.h> #include <linux/debugfs.h> +#include <linux/dma-resv.h> #include <linux/device/faux.h> #include <linux/dma-direction.h> #include <linux/dma-mapping.h> diff --git a/rust/helpers/dma-resv.c b/rust/helpers/dma-resv.c new file mode 100644 index 0000000000000..05501cb814513 --- /dev/null +++ b/rust/helpers/dma-resv.c @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/dma-resv.h> + +int rust_helper_dma_resv_lock(struct dma_resv *obj, struct ww_acquire_ctx *ctx) +{ + return dma_resv_lock(obj, ctx); +} + +void rust_helper_dma_resv_unlock(struct dma_resv *obj) +{ + dma_resv_unlock(obj); +} diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c index 551da6c9b5064..36d40f911345c 100644 --- a/rust/helpers/helpers.c +++ b/rust/helpers/helpers.c @@ -25,6 +25,7 @@ #include "cred.c" #include "device.c" #include "dma.c" +#include "dma-resv.c" #include "drm.c" #include "err.c" #include "irq.c" -- 2.52.0

1 week

Re: [PATCH] dma-buf: heaps: Clear CMA pages with clear_page()

by T.J. Mercier

On Sun, Nov 30, 2025 at 2:54 AM Linus Walleij <linus.walleij(a)linaro.org> wrote: > > clear_page() translates into memset(*p, 0, PAGE_SIZE) on some > architectures, but on the major architectures it will call > an optimized assembly snippet so use this instead of open > coding a memset(). > > Signed-off-by: Linus Walleij <linus.walleij(a)linaro.org> Reviewed-by: T.J. Mercier <tjmercier(a)google.com> > --- > drivers/dma-buf/heaps/cma_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > index 0df007111975..9eaff80050f2 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -315,7 +315,7 @@ static struct dma_buf *cma_heap_allocate(struct dma_heap *heap, > while (nr_clear_pages > 0) { > void *vaddr = kmap_local_page(page); > > - memset(vaddr, 0, PAGE_SIZE); > + clear_page(vaddr); > kunmap_local(vaddr); > /* > * Avoid wasting time zeroing memory if the process > > --- > base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787 > change-id: 20251129-dma-buf-heap-clear-page-248bb236e4c4 > > Best regards, > -- > Linus Walleij <linus.walleij(a)linaro.org> >

1 week, 2 days

Re: [PATCH 05/18] dma-buf: use inline lock for the stub fence

by Christian König

On 11/27/25 11:50, Philipp Stanner wrote: > On Thu, 2025-11-13 at 15:51 +0100, Christian König wrote: >> Using the inline lock is now the recommended way for dma_fence implementations. >> >> So use this approach for the framework internal fences as well. > > nit: > s/framework/framework's > >> >> Also saves about 4 bytes for the external spinlock. > > On all platforms or just AMD64? I think most if not all platforms. Or is anybody really using 64bits for a spinlock? Christian. > >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> >> --- >> drivers/dma-buf/dma-fence.c | 20 ++++---------------- >> 1 file changed, 4 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c >> index 9a5aa9e44e13..e6d26c2e0391 100644 >> --- a/drivers/dma-buf/dma-fence.c >> +++ b/drivers/dma-buf/dma-fence.c >> @@ -24,7 +24,6 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_emit); >> EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal); >> EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled); >> >> -static DEFINE_SPINLOCK(dma_fence_stub_lock); >> static struct dma_fence dma_fence_stub; >> >> /* >> @@ -123,12 +122,8 @@ static const struct dma_fence_ops dma_fence_stub_ops = { >> >> static int __init dma_fence_init_stub(void) >> { >> - dma_fence_init(&dma_fence_stub, &dma_fence_stub_ops, >> - &dma_fence_stub_lock, 0, 0); >> - >> - set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >> - &dma_fence_stub.flags); > > That change is unrelated, isn't it? Enlarges the diff and breaks git- > blame. > >> - >> + dma_fence_init(&dma_fence_stub, &dma_fence_stub_ops, NULL, 0, 0); >> + set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &dma_fence_stub.flags); >> dma_fence_signal(&dma_fence_stub); >> return 0; >> } >> @@ -160,16 +155,9 @@ struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp) >> if (fence == NULL) >> return NULL; >> >> - dma_fence_init(fence, >> - &dma_fence_stub_ops, >> - &dma_fence_stub_lock, >> - 0, 0); >> - >> - set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >> - &fence->flags); > > Same. > >> - >> + dma_fence_init(fence, &dma_fence_stub_ops, NULL, 0, 0); >> + set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags); >> dma_fence_signal_timestamp(fence, timestamp); >> - > > I wouldn't remove that empty line (nit). > >> return fence; >> } >> EXPORT_SYMBOL(dma_fence_allocate_private_stub); >

1 week, 4 days

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig