- Linaro-mm-sig - lists.linaro.org

Re: [PATCH 1/4] dma-buf/fence: give some reasonable maximum signaling timeout

by Lucas Stach

Am Mittwoch, dem 26.11.2025 um 16:44 +0100 schrieb Philipp Stanner: > On Wed, 2025-11-26 at 16:03 +0100, Christian König wrote: > > > > [...] > > > My hope would be that in the mid-term future we'd get firmware > > > rings > > > that can be preempted through a firmware call for all major > > > hardware. > > > Then a huge share of our problems would disappear. > > > > At least on AMD HW pre-emption is actually horrible unreliable as > > well. > > Do you mean new GPUs with firmware scheduling, or what is "HW pre- > emption"? > > With firmware interfaces, my hope would be that you could simply tell > > stop_running_ring(nr_of_ring) > // time slice for someone else > start_running_ring(nr_of_ring) > > Thereby getting real scheduling and all that. And eliminating many > other problems we know well from drm/sched. It doesn't really matter if you have firmware scheduling or not for preemption to be a hard problem on GPUs. CPUs have limited software visible state that needs to be saved/restored on a context switch and even there people start complaining now that they need to context switch the AVX512 register set. GPUs have megabytes of software visible state. Which needs to be saved/restored on the context switch if you want fine grained preemption with low preemption latency. There might be points in the command execution where you can ignore most of that state, but reaching those points can have basically unbounded latency. So either you can reliably save/restore lots of state or you are limited to very coarse grained preemption with all the usual issues of timeouts and DoS vectors. I'm not totally up to speed with the current state across all relevant GPUs, but until recently NVidia was the only vendor to have real reliable fine-grained preemption. Regards, Lucas

2 months, 1 week

2
1
0 0

Re: [PATCH 1/2] dma-buf: improve sg_table debugging hack v2

by Christian König

On 12/4/25 16:51, Matthew Auld wrote: > On 04/12/2025 14:59, Christian König wrote: >> This debugging hack is important to enforce the rule that importers >> should *never* touch the underlying struct page of the exporter. >> >> Instead of just mangling the page link create a copy of the sg_table >> but only copy over the DMA addresses and not the pages. >> >> This will cause a NULL pointer de-reference if the importer tries to >> touch the struct page. Still quite a hack but this at least allows the >> exporter to properly keeps it's sg_table intact while allowing the >> DMA-buf maintainer to find and fix misbehaving importers and finally >> switch over to using a different data structure in the future. >> >> v2: improve the hack further by using a wrapper structure and explaining >> the background a bit more in the commit message. >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> Reviewed-by: Michael J. Ruhl <michael.j.ruhl(a)intel.com> (v1) >> --- >> drivers/dma-buf/dma-buf.c | 72 +++++++++++++++++++++++++++++++-------- >> 1 file changed, 58 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 2305bb2cc1f1..8c4afd360b72 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -35,6 +35,12 @@ >> #include "dma-buf-sysfs-stats.h" >> +/* Wrapper to hide the sg_table page link from the importer */ >> +struct dma_buf_sg_table_wrapper { >> + struct sg_table *original; >> + struct sg_table wrapper; >> +}; >> + >> static inline int is_dma_buf_file(struct file *); >> static DEFINE_MUTEX(dmabuf_list_mutex); >> @@ -828,21 +834,57 @@ void dma_buf_put(struct dma_buf *dmabuf) >> } >> EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF"); >> -static void mangle_sg_table(struct sg_table *sg_table) >> +static int dma_buf_mangle_sg_table(struct sg_table **sg_table) >> { >> -#ifdef CONFIG_DMABUF_DEBUG >> - int i; >> - struct scatterlist *sg; >> - >> - /* 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 on unmap >> - * before passing the sgt back to the exporter. >> + struct scatterlist *to_sg, *from_sg; >> + struct sg_table *from = *sg_table; >> + struct dma_buf_sg_table_wrapper *to; >> + int i, ret; >> + >> + if (!IS_ENABLED(CONFIG_DMABUF_DEBUG)) >> + return 0; >> + >> + /* >> + * To catch abuse of the underlying struct page by importers copy the >> + * sg_table without copying the page_link and give only the copy back to >> + * the importer. >> */ >> - for_each_sgtable_sg(sg_table, sg, i) >> - sg->page_link ^= ~0xffUL; >> -#endif >> + to = kzalloc(sizeof(*to), GFP_KERNEL); >> + if (!to) >> + return -ENOMEM; >> + >> + ret = sg_alloc_table(&to->wrapper, from->nents, GFP_KERNEL); >> + if (ret) >> + goto free_to; >> + >> + to_sg = to->wrapper.sgl; >> + for_each_sgtable_dma_sg(from, from_sg, i) { >> + sg_set_page(to_sg, NULL, 0, 0); > > Are we still allowed to pass NULL page here? There looks to be the recently added: > > VM_WARN_ON_ONCE(!page_range_contiguous(page, ALIGN(len + offset, PAGE_SIZE) / PAGE_SIZE)); > > And if page_range_contiguous() does not just return true, it potentially wants to dereference the page, like with page_to_pfn()? Good point. It doesn't crash at the moment because page_to_pfn() also works with NULL as page, but it is clearly not the nicest thing to do. I will switch over to using sg_assign_page() instead. > > >> + sg_dma_address(to_sg) = sg_dma_address(from_sg); >> + sg_dma_len(to_sg) = sg_dma_len(from_sg); > > Nit: formatting looks off here. Oh, indeed. Thanks, Christian. > >> + to_sg = sg_next(to_sg); >> + } >> + to->original = from; >> + *sg_table = &to->wrapper; >> + return 0; >> + >> +free_to: >> + kfree(to); >> + return ret; >> +} >> + >> +static void dma_buf_demangle_sg_table(struct sg_table **sg_table) >> +{ >> + struct dma_buf_sg_table_wrapper *copy; >> + >> + if (!IS_ENABLED(CONFIG_DMABUF_DEBUG)) >> + return; >> + >> + copy = container_of(*sg_table, typeof(*copy), wrapper); >> + *sg_table = copy->original; >> + sg_free_table(&copy->wrapper); >> + kfree(copy); >> } >> static inline bool >> @@ -1139,7 +1181,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, >> if (ret < 0) >> goto error_unmap; >> } >> - mangle_sg_table(sg_table); >> + ret = dma_buf_mangle_sg_table(&sg_table); >> + if (ret) >> + goto error_unmap; >> if (IS_ENABLED(CONFIG_DMA_API_DEBUG)) { >> struct scatterlist *sg; >> @@ -1220,7 +1264,7 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, >> dma_resv_assert_held(attach->dmabuf->resv); >> - mangle_sg_table(sg_table); >> + dma_buf_demangle_sg_table(&sg_table); >> attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); >> if (dma_buf_pin_on_map(attach)) >

2 months, 1 week

1
0
0 0

[PATCH 1/2] dma-buf: improve sg_table debugging hack v2

by Christian König

This debugging hack is important to enforce the rule that importers should *never* touch the underlying struct page of the exporter. Instead of just mangling the page link create a copy of the sg_table but only copy over the DMA addresses and not the pages. This will cause a NULL pointer de-reference if the importer tries to touch the struct page. Still quite a hack but this at least allows the exporter to properly keeps it's sg_table intact while allowing the DMA-buf maintainer to find and fix misbehaving importers and finally switch over to using a different data structure in the future. v2: improve the hack further by using a wrapper structure and explaining the background a bit more in the commit message. Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Michael J. Ruhl <michael.j.ruhl(a)intel.com> (v1) --- drivers/dma-buf/dma-buf.c | 72 +++++++++++++++++++++++++++++++-------- 1 file changed, 58 insertions(+), 14 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 2305bb2cc1f1..8c4afd360b72 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -35,6 +35,12 @@ #include "dma-buf-sysfs-stats.h" +/* Wrapper to hide the sg_table page link from the importer */ +struct dma_buf_sg_table_wrapper { + struct sg_table *original; + struct sg_table wrapper; +}; + static inline int is_dma_buf_file(struct file *); static DEFINE_MUTEX(dmabuf_list_mutex); @@ -828,21 +834,57 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF"); -static void mangle_sg_table(struct sg_table *sg_table) +static int dma_buf_mangle_sg_table(struct sg_table **sg_table) { -#ifdef CONFIG_DMABUF_DEBUG - int i; - struct scatterlist *sg; - - /* 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 on unmap - * before passing the sgt back to the exporter. + struct scatterlist *to_sg, *from_sg; + struct sg_table *from = *sg_table; + struct dma_buf_sg_table_wrapper *to; + int i, ret; + + if (!IS_ENABLED(CONFIG_DMABUF_DEBUG)) + return 0; + + /* + * To catch abuse of the underlying struct page by importers copy the + * sg_table without copying the page_link and give only the copy back to + * the importer. */ - for_each_sgtable_sg(sg_table, sg, i) - sg->page_link ^= ~0xffUL; -#endif + to = kzalloc(sizeof(*to), GFP_KERNEL); + if (!to) + return -ENOMEM; + + ret = sg_alloc_table(&to->wrapper, from->nents, GFP_KERNEL); + if (ret) + goto free_to; + + to_sg = to->wrapper.sgl; + for_each_sgtable_dma_sg(from, from_sg, i) { + sg_set_page(to_sg, NULL, 0, 0); + sg_dma_address(to_sg) = sg_dma_address(from_sg); + sg_dma_len(to_sg) = sg_dma_len(from_sg); + to_sg = sg_next(to_sg); + } + to->original = from; + *sg_table = &to->wrapper; + return 0; + +free_to: + kfree(to); + return ret; +} + +static void dma_buf_demangle_sg_table(struct sg_table **sg_table) +{ + struct dma_buf_sg_table_wrapper *copy; + + if (!IS_ENABLED(CONFIG_DMABUF_DEBUG)) + return; + + copy = container_of(*sg_table, typeof(*copy), wrapper); + *sg_table = copy->original; + sg_free_table(&copy->wrapper); + kfree(copy); } static inline bool @@ -1139,7 +1181,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (ret < 0) goto error_unmap; } - mangle_sg_table(sg_table); + ret = dma_buf_mangle_sg_table(&sg_table); + if (ret) + goto error_unmap; if (IS_ENABLED(CONFIG_DMA_API_DEBUG)) { struct scatterlist *sg; @@ -1220,7 +1264,7 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, dma_resv_assert_held(attach->dmabuf->resv); - mangle_sg_table(sg_table); + dma_buf_demangle_sg_table(&sg_table); attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); if (dma_buf_pin_on_map(attach)) -- 2.43.0

2 months, 1 week

1
1
0 0

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

by Christian König

On 11/23/25 23:51, Pavel Begunkov wrote: > Add a file callback that maps a dmabuf for the given file and returns > an opaque token of type struct dma_token representing the mapping. I'm really scratching my head what you mean with that? And why the heck would we need to pass a DMA-buf to a struct file? Regards, Christian. > The > implementation details are hidden from the caller, and the implementors > are normally expected to extend the structure. > > The callback callers will be able to pass the token with an IO request, > which implemented in following patches as a new iterator type. The user > should release the token once it's not needed by calling the provided > release callback via appropriate helpers. > > Signed-off-by: Pavel Begunkov <asml.silence(a)gmail.com> > --- > include/linux/dma_token.h | 35 +++++++++++++++++++++++++++++++++++ > include/linux/fs.h | 4 ++++ > 2 files changed, 39 insertions(+) > create mode 100644 include/linux/dma_token.h > > diff --git a/include/linux/dma_token.h b/include/linux/dma_token.h > new file mode 100644 > index 000000000000..9194b34282c2 > --- /dev/null > +++ b/include/linux/dma_token.h > @@ -0,0 +1,35 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _LINUX_DMA_TOKEN_H > +#define _LINUX_DMA_TOKEN_H > + > +#include <linux/dma-buf.h> > + > +struct dma_token_params { > + struct dma_buf *dmabuf; > + enum dma_data_direction dir; > +}; > + > +struct dma_token { > + void (*release)(struct dma_token *); > +}; > + > +static inline void dma_token_release(struct dma_token *token) > +{ > + token->release(token); > +} > + > +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); > + > + WARN_ON_ONCE(!IS_ERR(res) && !res->release); > + > + return res; > +} > + > +#endif > diff --git a/include/linux/fs.h b/include/linux/fs.h > index c895146c1444..0ce9a53fabec 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2262,6 +2262,8 @@ struct dir_context { > struct iov_iter; > struct io_uring_cmd; > struct offset_ctx; > +struct dma_token; > +struct dma_token_params; > > typedef unsigned int __bitwise fop_flags_t; > > @@ -2309,6 +2311,8 @@ struct file_operations { > int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *, > unsigned int poll_flags); > int (*mmap_prepare)(struct vm_area_desc *); > + struct dma_token *(*dma_map)(struct file *, > + struct dma_token_params *); > } __randomize_layout; > > /* Supports async buffered reads */

2 months, 1 week

2
3
0 0

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: > +struct dma_token *blkdev_dma_map(struct file *file, > + struct dma_token_params *params) Given that this is a direct file operation instance it should be in block/fops.c. If we do want a generic helper below it, it should take a struct block_device instead. But we can probably defer that until a user for that shows up.

2 months, 1 week

1
0
0 0

Re: [RFC v2 07/11] nvme-pci: implement dma_token backed requests

by Christoph Hellwig

> +static void nvme_sync_dma(struct nvme_dev *nvme_dev, struct request *req, > + enum dma_data_direction dir) > +{ > + struct blk_mq_dma_map *map = req->dma_map; > + int length = blk_rq_payload_bytes(req); > + bool for_cpu = dir == DMA_FROM_DEVICE; > + struct device *dev = nvme_dev->dev; > + dma_addr_t *dma_list = map->private; > + struct bio *bio = req->bio; > + int offset, map_idx; > + > + offset = bio->bi_iter.bi_bvec_done; > + map_idx = offset / NVME_CTRL_PAGE_SIZE; > + length += offset & (NVME_CTRL_PAGE_SIZE - 1); > + > + while (length > 0) { > + u64 dma_addr = dma_list[map_idx++]; > + > + if (for_cpu) > + __dma_sync_single_for_cpu(dev, dma_addr, > + NVME_CTRL_PAGE_SIZE, dir); > + else > + __dma_sync_single_for_device(dev, dma_addr, > + NVME_CTRL_PAGE_SIZE, dir); > + length -= NVME_CTRL_PAGE_SIZE; > + } This looks really inefficient. Usually the ranges in the dmabuf should be much larger than a controller page. > +static void nvme_unmap_premapped_data(struct nvme_dev *dev, > + struct request *req) > +{ > + struct nvme_iod *iod = blk_mq_rq_to_pdu(req); > + > + if (rq_data_dir(req) == READ) > + nvme_sync_dma(dev, req, DMA_FROM_DEVICE); > + if (!(iod->flags & IOD_SINGLE_SEGMENT)) > + nvme_free_descriptors(req); > +} This doesn't really unmap anything :) Also the dma ownership rules say that you always need to call the sync_to_device helpers before I/O and the sync_to_cpu helpers after I/O, no matters if it is a read or write. The implementations then makes them a no-op where possible. > + > + offset = bio->bi_iter.bi_bvec_done; > + map_idx = offset / NVME_CTRL_PAGE_SIZE; > + offset &= (NVME_CTRL_PAGE_SIZE - 1); > + > + prp1_dma = dma_list[map_idx++] + offset; > + > + length -= (NVME_CTRL_PAGE_SIZE - offset); > + if (length <= 0) { > + prp2_dma = 0; Urgg, why is this building PRPs instead of SGLs? Yes, SGLs are an optional feature, but for devices where you want to micro-optimize like this I think we should simply require them. This should cut down on both the memory use and the amount of special mapping code.

2 months, 1 week

1
0
0 0

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?

2 months, 1 week

1
0
0 0

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.

2 months, 1 week

1
0
0 0

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.

2 months, 1 week

1
0
0 0

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?

2 months, 1 week

1
0
0 0

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.

2 months, 1 week

1
0
0 0

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?

2 months, 1 week

1
0
0 0

[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

2 months, 2 weeks

1
0
0 0

[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

2 months, 2 weeks

1
0
0 0

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> >

2 months, 2 weeks

1
0
0 0

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); >

2 months, 2 weeks

1
0
0 0

Re: [PATCH 13/18] drm/amdgpu: independence for the amdkfd_fence! v2

by Christian König

On 11/28/25 11:10, Philipp Stanner wrote: > On Fri, 2025-11-28 at 11:06 +0100, Christian König wrote: >> On 11/27/25 12:10, Philipp Stanner wrote: >>> On Thu, 2025-11-13 at 15:51 +0100, Christian König wrote: >>>> This should allow amdkfd_fences to outlive the amdgpu module. >>>> >>>> v2: implement Felix suggestion to lock the fence while signaling it. >>>> >>>> Signed-off-by: Christian König <christian.koenig(a)amd.com> >>>> --- >>>> >>>> > > […] > >>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>>> index a085faac9fe1..8fac70b839ed 100644 >>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>>> @@ -1173,7 +1173,7 @@ static void kfd_process_wq_release(struct work_struct *work) >>>> synchronize_rcu(); >>>> ef = rcu_access_pointer(p->ef); >>>> if (ef) >>>> - dma_fence_signal(ef); >>>> + amdkfd_fence_signal(ef); >>>> >>>> kfd_process_remove_sysfs(p); >>>> kfd_debugfs_remove_process(p); >>>> @@ -1990,7 +1990,6 @@ kfd_process_gpuid_from_node(struct kfd_process *p, struct kfd_node *node, >>>> static int signal_eviction_fence(struct kfd_process *p) >>>> { >>>> struct dma_fence *ef; >>>> - int ret; >>>> >>>> rcu_read_lock(); >>>> ef = dma_fence_get_rcu_safe(&p->ef); >>>> @@ -1998,10 +1997,10 @@ static int signal_eviction_fence(struct kfd_process *p) >>>> if (!ef) >>>> return -EINVAL; >>>> >>>> - ret = dma_fence_signal(ef); >>>> + amdkfd_fence_signal(ef); >>>> dma_fence_put(ef); >>>> >>>> - return ret; >>>> + return 0; >>> >>> Oh wait, that's the code I'm also touching in my return code series! >>> >>> https://lore.kernel.org/dri-devel/cef83fed-5994-4c77-962c-9c7aac9f7306@amd.… >>> >>> >>> Does this series then solve the problem Felix pointed out in >>> evict_process_worker()? >> >> No it doesn't, I wasn't aware that the higher level code actually needs the status. After all Felix is the maintainer of this part. >> >> This patch here needs to be rebased on top of yours and changed accordingly to still return the fence status correctly. >> >> But thanks for pointing that out. > > > Alright, so my (repaired, v2) status-code-removal series shall enter drm-misc-next first, and then your series here. ACK? Works for me, I just need both to re-base the amdgpu patches on top. Christian. > > > P.

2 months, 2 weeks

1
0
0 0

Re: [PATCH 13/18] drm/amdgpu: independence for the amdkfd_fence! v2

by Christian König

On 11/27/25 12:10, Philipp Stanner wrote: > On Thu, 2025-11-13 at 15:51 +0100, Christian König wrote: >> This should allow amdkfd_fences to outlive the amdgpu module. >> >> v2: implement Felix suggestion to lock the fence while signaling it. >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 6 +++ >> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 39 ++++++++----------- >> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 7 ++-- >> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 +- >> 4 files changed, 27 insertions(+), 29 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h >> index 8bdfcde2029b..6254cef04213 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h >> @@ -196,6 +196,7 @@ int kfd_debugfs_kfd_mem_limits(struct seq_file *m, void *data); >> #endif >> #if IS_ENABLED(CONFIG_HSA_AMD) >> bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm); >> +void amdkfd_fence_signal(struct dma_fence *f); >> struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f); >> void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo); >> int amdgpu_amdkfd_evict_userptr(struct mmu_interval_notifier *mni, >> @@ -210,6 +211,11 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) >> return false; >> } >> >> +static inline >> +void amdkfd_fence_signal(struct dma_fence *f) >> +{ > > I would add a short comment here: "Empty function because …" > >> +} >> + >> static inline >> struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f) >> { >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> index 09c919f72b6c..f76c3c52a2a1 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> @@ -127,29 +127,9 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f) >> if (!svm_range_schedule_evict_svm_bo(fence)) >> return true; >> } >> - return false; >> -} >> - >> -/** >> - * amdkfd_fence_release - callback that fence can be freed >> - * >> - * @f: dma_fence >> - * >> - * This function is called when the reference count becomes zero. >> - * Drops the mm_struct reference and RCU schedules freeing up the fence. >> - */ >> -static void amdkfd_fence_release(struct dma_fence *f) >> -{ >> - struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f); >> - >> - /* Unconditionally signal the fence. The process is getting >> - * terminated. >> - */ >> - if (WARN_ON(!fence)) >> - return; /* Not an amdgpu_amdkfd_fence */ >> - >> mmdrop(fence->mm); >> - kfree_rcu(f, rcu); >> + fence->mm = NULL; > > That the storage actually takes place is guaranteed by the lock taken > when calling the fence ops? > >> + return false; >> } >> >> /** >> @@ -174,9 +154,22 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) >> return false; >> } >> >> +void amdkfd_fence_signal(struct dma_fence *f) >> +{ >> + struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f); >> + long flags; >> + >> + dma_fence_lock_irqsafe(f, flags) >> + if (fence->mm) { >> + mmdrop(fence->mm); >> + fence->mm = NULL; >> + } >> + dma_fence_signal_locked(f); >> + dma_fence_unlock_irqrestore(f, flags) >> +} >> + >> static const struct dma_fence_ops amdkfd_fence_ops = { >> .get_driver_name = amdkfd_fence_get_driver_name, >> .get_timeline_name = amdkfd_fence_get_timeline_name, >> .enable_signaling = amdkfd_fence_enable_signaling, >> - .release = amdkfd_fence_release, >> }; >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> index a085faac9fe1..8fac70b839ed 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> @@ -1173,7 +1173,7 @@ static void kfd_process_wq_release(struct work_struct *work) >> synchronize_rcu(); >> ef = rcu_access_pointer(p->ef); >> if (ef) >> - dma_fence_signal(ef); >> + amdkfd_fence_signal(ef); >> >> kfd_process_remove_sysfs(p); >> kfd_debugfs_remove_process(p); >> @@ -1990,7 +1990,6 @@ kfd_process_gpuid_from_node(struct kfd_process *p, struct kfd_node *node, >> static int signal_eviction_fence(struct kfd_process *p) >> { >> struct dma_fence *ef; >> - int ret; >> >> rcu_read_lock(); >> ef = dma_fence_get_rcu_safe(&p->ef); >> @@ -1998,10 +1997,10 @@ static int signal_eviction_fence(struct kfd_process *p) >> if (!ef) >> return -EINVAL; >> >> - ret = dma_fence_signal(ef); >> + amdkfd_fence_signal(ef); >> dma_fence_put(ef); >> >> - return ret; >> + return 0; > > Oh wait, that's the code I'm also touching in my return code series! > > https://lore.kernel.org/dri-devel/cef83fed-5994-4c77-962c-9c7aac9f7306@amd.… > > > Does this series then solve the problem Felix pointed out in > evict_process_worker()? No it doesn't, I wasn't aware that the higher level code actually needs the status. After all Felix is the maintainer of this part. This patch here needs to be rebased on top of yours and changed accordingly to still return the fence status correctly. But thanks for pointing that out. Regards, Christian. > > > P. > > >> } >> >> static void evict_process_worker(struct work_struct *work) >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c >> index c30dfb8ec236..566950702b7d 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c >> @@ -428,7 +428,7 @@ static void svm_range_bo_release(struct kref *kref) >> >> if (!dma_fence_is_signaled(&svm_bo->eviction_fence->base)) >> /* We're not in the eviction worker. Signal the fence. */ >> - dma_fence_signal(&svm_bo->eviction_fence->base); >> + amdkfd_fence_signal(&svm_bo->eviction_fence->base); >> dma_fence_put(&svm_bo->eviction_fence->base); >> amdgpu_bo_unref(&svm_bo->bo); >> kfree(svm_bo); >> @@ -3628,7 +3628,7 @@ static void svm_range_evict_svm_bo_worker(struct work_struct *work) >> mmap_read_unlock(mm); >> mmput(mm); >> >> - dma_fence_signal(&svm_bo->eviction_fence->base); >> + amdkfd_fence_signal(&svm_bo->eviction_fence->base); >> >> /* This is the last reference to svm_bo, after svm_range_vram_node_free >> * has been called in svm_migrate_vram_to_ram >

2 months, 2 weeks

1
0
0 0

Re: [PATCH 09/18] drm/amdgpu: fix KFD eviction fence enable_signaling path

by Christian König

On 11/27/25 11:57, Philipp Stanner wrote: > On Thu, 2025-11-13 at 15:51 +0100, Christian König wrote: >> Calling dma_fence_is_signaled() here is illegal! > > OK, but why is that patch in this series? Because the next patch depends on it, otherwise the series won't compile. My plan is to push the amdgpu patches through amd-staging-drm-next as soon as Alex rebased that branch on drm-next during the next cycle. Regards, Christian. > > P. > >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> index 1ef758ac5076..09c919f72b6c 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> @@ -120,12 +120,6 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f) >> { >> struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f); >> >> - if (!fence) >> - return false; >> - >> - if (dma_fence_is_signaled(f)) >> - return true; >> - >> if (!fence->svm_bo) { >> if (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, f)) >> return true; >

2 months, 2 weeks

1
0
0 0

Re: [PATCH 2/6] amd/amdkfd: Ignore return code of dma_fence_signal()

by Christian König

On 11/27/25 10:48, Philipp Stanner wrote: > On Wed, 2025-11-26 at 16:24 -0500, Kuehling, Felix wrote: >> >> On 2025-11-26 08:19, Philipp Stanner wrote: >>> The return code of dma_fence_signal() is not really useful as there is >>> nothing reasonable to do if a fence was already signaled. That return >>> code shall be removed from the kernel. >>> >>> Ignore dma_fence_signal()'s return code. >> >> I think this is not correct. Looking at the comment in >> evict_process_worker, we use the return value to decide a race >> conditions where multiple threads are trying to signal the eviction >> fence. Only one of them should schedule the restore work. And the other >> ones need to increment the reference count to keep evictions balanced. > > Thank you for pointing that out. Seems then amdkfd is the only user who > actually relies on the feature. See below > >> >> Regards, >> Felix >> >> >>> >>> Suggested-by: Christian König <christian.koenig(a)amd.com> >>> Signed-off-by: Philipp Stanner <phasta(a)kernel.org> >>> --- >>> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> index ddfe30c13e9d..950fafa4b3c3 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> @@ -1986,7 +1986,6 @@ kfd_process_gpuid_from_node(struct kfd_process *p, struct kfd_node *node, >>> static int signal_eviction_fence(struct kfd_process *p) >>> { >>> struct dma_fence *ef; >>> - int ret; >>> >>> rcu_read_lock(); >>> ef = dma_fence_get_rcu_safe(&p->ef); >>> @@ -1994,10 +1993,10 @@ static int signal_eviction_fence(struct kfd_process *p) >>> if (!ef) >>> return -EINVAL; >>> >>> - ret = dma_fence_signal(ef); >>> + dma_fence_signal(ef); > > The issue now is that dma_fence_signal()'s return code is actually non- > racy, because check + bit-set are protected by lock. > > Christian's new spinlock series would add a lock function for fences: > https://lore.kernel.org/dri-devel/20251113145332.16805-5-christian.koenig@a… > > > So I suppose this should work: > > dma_fence_lock_irqsave(ef, flags); > if (dma_fence_test_signaled_flag(ef)) { > dma_fence_unlock_irqrestore(ef, flags); > return true; > } > dma_fence_signal_locked(ef); > dma_fence_unlock_irqrestore(ef, flags); > > return false; > > > + some cosmetic adjustments for the boolean of course. > > > Would that fly and be reasonable? @Felix, Christian. I was just about to reply with the same idea when your mail arrived. So yes looks totally reasonable to me. Regards, Christian. > > > P.

2 months, 2 weeks

2
1
0 0

Re: [PATCH] dma-buf: add no-op stubs when CONFIG_DMA_SHARED_BUFFER is disabled

by Christian König

On 11/27/25 09:23, Viresh Kumar wrote: > On 27-11-25, 09:07, Christian König wrote: >> On 11/27/25 08:40, Viresh Kumar wrote: >>> Move several dma-buf function declarations under >>> CONFIG_DMA_SHARED_BUFFER and provide static inline no-op implementations >>> for the disabled case to allow the callers to build when the feature is >>> not compiled in. >> >> Good point, but which driver actually needs that? > > This broke some WIP stuff [1] which isn't posted upstream yet. That's why I > didn't mention anything in the commit log, though I could have added a comment > about that in the non-commit-log part. Well then better send that out with the full patch set. >> In other words there should be a concrete example of what breaks in the commit message. > > There is time for those changes to be posted and not sure if they will be > accepted or not. But either way, this change made sense in general and so I > thought there is nothing wrong to get this upstream right away. Yeah when it is unused intermediately then that is usually a no-go even if I agree that it makes sense. >>> +static inline struct dma_buf *dma_buf_get(int fd) >>> +{ >>> + return NULL; >> >> And here ERR_PTR(-EINVAL). > > I am not really sure if this should be EINVAL in this case. EOPNOTSUPP still > makes sense as the API isn't supported. When the API isn't compiled in the fd can't be valid (because you can't create a dma_buf object in the first place). So returning -EINVAL still makes a lot of sense. Regards, Christian. > >>> +static inline struct dma_buf *dma_buf_iter_begin(void) >>> +{ >>> + return NULL; >>> +} >>> + >>> +static inline struct dma_buf *dma_buf_iter_next(struct dma_buf *dmbuf) >>> +{ >>> + return NULL; >>> +} >> >> Those two are only for BPF and not driver use. > > Will drop them. >

2 months, 2 weeks

1
0
0 0

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

by Christian König

On 11/27/25 10:16, Philipp Stanner wrote: > On Thu, 2025-11-27 at 09:11 +0100, Christian König wrote: >> On 11/26/25 17:55, Matthew Brost wrote: >>> On Wed, Nov 26, 2025 at 08:41:27AM -0800, Matthew Brost wrote: >>>> On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote: >>>>> The dma_fence framework checks at many places whether the signaled flag >>>>> of a fence is already set. The code can be simplified and made more >>>>> readable by providing a helper function for that. >>>>> >>>>> Add dma_fence_test_signaled_flag(), which only checks whether a fence is >>>>> signaled. Use it internally. >>>>> >>>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> >>>>> Signed-off-by: Philipp Stanner <phasta(a)kernel.org> >>>> >>>> This is a nice cleanp: >>>> Reviewed-by: Matthew Brost <matthew.brost(a)intel.com> >>>> >>>>> --- >>>>> drivers/dma-buf/dma-fence.c | 19 +++++++++---------- >>>>> include/linux/dma-fence.h | 24 ++++++++++++++++++++++-- >>>>> 2 files changed, 31 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c >>>>> index 39e6f93dc310..25117a906846 100644 >>>>> --- a/drivers/dma-buf/dma-fence.c >>>>> +++ b/drivers/dma-buf/dma-fence.c >>>>> @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence, >>>>> >>>>> lockdep_assert_held(fence->lock); >>>>> >>>>> - if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >>>>> - &fence->flags))) >>> >>> I need to read a little better, I think this change isn't quite right. >>> The original code is test and set, the updated code is test only (i.e., >>> you are missing the set step). So maybe just leave this line as is. >> >> Oh, good point! I've totally missed that as well. > > Oh dear; I also just saw it when opening the mail client ._. > >> >> But that means that this patch set hasn't even been smoke tested. > > I've built it and did some basic testing with my Nouveau system. Any > suggestions? Do you have a CI that one can trigger? DMA-buf has CONFIG_DMABUF_SELFTESTS which should be able to catch things like that. But even running Nouveau should have found this since basically no fence at would signal any more. Regards, Christian. > > Thx > P.

2 months, 2 weeks

1
0
0 0

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

by Christian König

On 11/26/25 17:55, Matthew Brost wrote: > On Wed, Nov 26, 2025 at 08:41:27AM -0800, Matthew Brost wrote: >> On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote: >>> The dma_fence framework checks at many places whether the signaled flag >>> of a fence is already set. The code can be simplified and made more >>> readable by providing a helper function for that. >>> >>> Add dma_fence_test_signaled_flag(), which only checks whether a fence is >>> signaled. Use it internally. >>> >>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> >>> Signed-off-by: Philipp Stanner <phasta(a)kernel.org> >> >> This is a nice cleanp: >> Reviewed-by: Matthew Brost <matthew.brost(a)intel.com> >> >>> --- >>> drivers/dma-buf/dma-fence.c | 19 +++++++++---------- >>> include/linux/dma-fence.h | 24 ++++++++++++++++++++++-- >>> 2 files changed, 31 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c >>> index 39e6f93dc310..25117a906846 100644 >>> --- a/drivers/dma-buf/dma-fence.c >>> +++ b/drivers/dma-buf/dma-fence.c >>> @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence, >>> >>> lockdep_assert_held(fence->lock); >>> >>> - if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >>> - &fence->flags))) > > I need to read a little better, I think this change isn't quite right. > The original code is test and set, the updated code is test only (i.e., > you are missing the set step). So maybe just leave this line as is. Oh, good point! I've totally missed that as well. But that means that this patch set hasn't even been smoke tested. Regards, Christian. > > Matt > >>> + if (unlikely(dma_fence_test_signaled_flag(fence))) >>> return -EINVAL; >>> >>> /* Stash the cb_list before replacing it with the timestamp */ >>> @@ -545,7 +544,7 @@ void dma_fence_release(struct kref *kref) >>> trace_dma_fence_destroy(fence); >>> >>> if (!list_empty(&fence->cb_list) && >>> - !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { >>> + !dma_fence_test_signaled_flag(fence)) { >>> const char __rcu *timeline; >>> const char __rcu *driver; >>> unsigned long flags; >>> @@ -602,7 +601,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence) >>> was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >>> &fence->flags); >>> >>> - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >>> + if (dma_fence_test_signaled_flag(fence)) >>> return false; >>> >>> if (!was_set && fence->ops->enable_signaling) { >>> @@ -666,7 +665,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, >>> if (WARN_ON(!fence || !func)) >>> return -EINVAL; >>> >>> - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { >>> + if (dma_fence_test_signaled_flag(fence)) { >>> INIT_LIST_HEAD(&cb->node); >>> return -ENOENT; >>> } >>> @@ -783,7 +782,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) >>> >>> spin_lock_irqsave(fence->lock, flags); >>> >>> - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >>> + if (dma_fence_test_signaled_flag(fence)) >>> goto out; >>> >>> if (intr && signal_pending(current)) { >>> @@ -800,7 +799,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) >>> cb.task = current; >>> list_add(&cb.base.node, &fence->cb_list); >>> >>> - while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) { >>> + while (!dma_fence_test_signaled_flag(fence) && ret > 0) { >>> if (intr) >>> __set_current_state(TASK_INTERRUPTIBLE); >>> else >>> @@ -832,7 +831,7 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count, >>> >>> for (i = 0; i < count; ++i) { >>> struct dma_fence *fence = fences[i]; >>> - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { >>> + if (dma_fence_test_signaled_flag(fence)) { >>> if (idx) >>> *idx = i; >>> return true; >>> @@ -1108,7 +1107,7 @@ const char __rcu *dma_fence_driver_name(struct dma_fence *fence) >>> RCU_LOCKDEP_WARN(!rcu_read_lock_held(), >>> "RCU protection is required for safe access to returned string"); >>> >>> - if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >>> + if (!dma_fence_test_signaled_flag(fence)) >>> return fence->ops->get_driver_name(fence); >>> else >>> return "detached-driver"; >>> @@ -1140,7 +1139,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence) >>> RCU_LOCKDEP_WARN(!rcu_read_lock_held(), >>> "RCU protection is required for safe access to returned string"); >>> >>> - if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >>> + if (!dma_fence_test_signaled_flag(fence)) >>> return fence->ops->get_timeline_name(fence); >>> else >>> return "signaled-timeline"; >>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h >>> index 64639e104110..19972f5d176f 100644 >>> --- a/include/linux/dma-fence.h >>> +++ b/include/linux/dma-fence.h >>> @@ -401,6 +401,26 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence); >>> const char __rcu *dma_fence_driver_name(struct dma_fence *fence); >>> const char __rcu *dma_fence_timeline_name(struct dma_fence *fence); >>> >>> +/* >>> + * dma_fence_test_signaled_flag - Only check whether a fence is signaled yet. >>> + * @fence: the fence to check >>> + * >>> + * This function just checks whether @fence is signaled, without interacting >>> + * with the fence in any way. The user must, therefore, ensure through other >>> + * means that fences get signaled eventually. >>> + * >>> + * This function uses test_bit(), which is thread-safe. Naturally, this function >>> + * should be used opportunistically; a fence could get signaled at any moment >>> + * after the check is done. >>> + * >>> + * Return: true if signaled, false otherwise. >>> + */ >>> +static inline bool >>> +dma_fence_test_signaled_flag(struct dma_fence *fence) >>> +{ >>> + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); >>> +} >>> + >>> /** >>> * dma_fence_is_signaled_locked - Return an indication if the fence >>> * is signaled yet. >>> @@ -418,7 +438,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence); >>> static inline bool >>> dma_fence_is_signaled_locked(struct dma_fence *fence) >>> { >>> - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >>> + if (dma_fence_test_signaled_flag(fence)) >>> return true; >>> >>> if (fence->ops->signaled && fence->ops->signaled(fence)) { >>> @@ -448,7 +468,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence) >>> static inline bool >>> dma_fence_is_signaled(struct dma_fence *fence) >>> { >>> - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >>> + if (dma_fence_test_signaled_flag(fence)) >>> return true; >>> >>> if (fence->ops->signaled && fence->ops->signaled(fence)) { >>> -- >>> 2.49.0 >>>

2 months, 2 weeks

1
0
0 0

Re: [PATCH] dma-buf: add no-op stubs when CONFIG_DMA_SHARED_BUFFER is disabled

by Christian König

On 11/27/25 08:40, Viresh Kumar wrote: > Move several dma-buf function declarations under > CONFIG_DMA_SHARED_BUFFER and provide static inline no-op implementations > for the disabled case to allow the callers to build when the feature is > not compiled in. Good point, but which driver actually needs that? At least the whole DRM subsystem selects CONFIG_DMA_SHARED_BUFFER and others (like V4L) usually don't compile their whole infrastructure when that option isn't selected. In other words there should be a concrete example of what breaks in the commit message. > > Signed-off-by: Viresh Kumar <viresh.kumar(a)linaro.org> > --- > include/linux/dma-buf.h | 116 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 116 insertions(+) > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index d58e329ac0e7..06e494d8f6b0 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -568,6 +568,7 @@ static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf) > return !!dmabuf->ops->pin; > } > > +#ifdef CONFIG_DMA_SHARED_BUFFER > struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, > struct device *dev); > struct dma_buf_attachment * > @@ -609,4 +610,119 @@ int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map); > void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map); > struct dma_buf *dma_buf_iter_begin(void); > struct dma_buf *dma_buf_iter_next(struct dma_buf *dmbuf); > + > +#else > +static inline struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, > + struct device *dev) > +{ > + return NULL; This should probably be an ERR_PTR(-EOPNOTSUPP); > +} > + > +static inline struct dma_buf_attachment * > +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > + const struct dma_buf_attach_ops *importer_ops, > + void *importer_priv) > +{ > + return NULL; Same here, ERR_PTR(-EOPNOTSUPP). > +} > + > +static inline void dma_buf_detach(struct dma_buf *dmabuf, > + struct dma_buf_attachment *attach) { } > + > +static inline int dma_buf_pin(struct dma_buf_attachment *attach) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline void dma_buf_unpin(struct dma_buf_attachment *attach) { } > + > +static inline struct dma_buf * > +dma_buf_export(const struct dma_buf_export_info *exp_info) > +{ > + return NULL; > +} > + > + > +static inline int dma_buf_fd(struct dma_buf *dmabuf, int flags) > +{ > + return -EOPNOTSUPP; > +} > + > + > +static inline struct dma_buf *dma_buf_get(int fd) > +{ > + return NULL; And here ERR_PTR(-EINVAL). > +} > + > +static inline void dma_buf_put(struct dma_buf *dmabuf) { } > + > +static inline struct sg_table * > +dma_buf_map_attachment(struct dma_buf_attachment *, enum dma_data_direction) > +{ > + return NULL; ERR_PTR(-EINVAL) > +} > + > +static inline void dma_buf_unmap_attachment(struct dma_buf_attachment *, > + struct sg_table *, > + enum dma_data_direction) { } > + > +static inline void dma_buf_move_notify(struct dma_buf *dma_buf) { } > + > +static inline int dma_buf_begin_cpu_access(struct dma_buf *dma_buf, > + enum dma_data_direction dir) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int dma_buf_end_cpu_access(struct dma_buf *dma_buf, > + enum dma_data_direction dir) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline struct sg_table * > +dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach, > + enum dma_data_direction direction) > +{ > + return NULL; ERR_PTR(-EINVAL) > +} > + > +static inline void > +dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *attach, > + struct sg_table *sg_table, > + enum dma_data_direction direction) { } > + > +static inline int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, > + unsigned long) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map) > +{ } > + > +static inline int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, > + struct iosys_map *map) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, > + struct iosys_map *map) { } > + > +static inline struct dma_buf *dma_buf_iter_begin(void) > +{ > + return NULL; > +} > + > +static inline struct dma_buf *dma_buf_iter_next(struct dma_buf *dmbuf) > +{ > + return NULL; > +} Those two are only for BPF and not driver use. Regards, Christian. > +#endif /* CONFIG_DMA_SHARED_BUFFER */ > #endif /* __DMA_BUF_H__ */

2 months, 2 weeks

1
0
0 0

Re: [PATCH 2/6] amd/amdkfd: Ignore return code of dma_fence_signal()

by Kuehling, Felix

On 2025-11-26 08:19, Philipp Stanner wrote: > The return code of dma_fence_signal() is not really useful as there is > nothing reasonable to do if a fence was already signaled. That return > code shall be removed from the kernel. > > Ignore dma_fence_signal()'s return code. I think this is not correct. Looking at the comment in evict_process_worker, we use the return value to decide a race conditions where multiple threads are trying to signal the eviction fence. Only one of them should schedule the restore work. And the other ones need to increment the reference count to keep evictions balanced. Regards, Felix > > Suggested-by: Christian König <christian.koenig(a)amd.com> > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > index ddfe30c13e9d..950fafa4b3c3 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > @@ -1986,7 +1986,6 @@ kfd_process_gpuid_from_node(struct kfd_process *p, struct kfd_node *node, > static int signal_eviction_fence(struct kfd_process *p) > { > struct dma_fence *ef; > - int ret; > > rcu_read_lock(); > ef = dma_fence_get_rcu_safe(&p->ef); > @@ -1994,10 +1993,10 @@ static int signal_eviction_fence(struct kfd_process *p) > if (!ef) > return -EINVAL; > > - ret = dma_fence_signal(ef); > + dma_fence_signal(ef); > dma_fence_put(ef); > > - return ret; > + return 0; > } > > static void evict_process_worker(struct work_struct *work)

2 months, 2 weeks

1
0
0 0

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig