On Tue, Mar 03, 2026 at 03:47:14PM +0100, Albert Esteve wrote: > On Tue, Mar 3, 2026 at 2:20 PM Maxime Ripard <[email protected]> wrote: > > On Tue, Mar 03, 2026 at 01:33:47PM +0100, Albert Esteve wrote: > > > Add a dma-buf heap for DT coherent reserved-memory > > > (i.e., 'shared-dma-pool' without 'reusable' property), > > > exposing one heap per region for userspace buffers. > > > > > > The heap binds the heap device to each memory region so > > > coherent allocations use the correct dev->dma_mem, and > > > it defers registration until module_init when normal > > > allocators are available. > > > > > > Signed-off-by: Albert Esteve <[email protected]> > > > --- > > > drivers/dma-buf/dma-heap.c | 4 +- > > > drivers/dma-buf/heaps/Kconfig | 9 + > > > drivers/dma-buf/heaps/Makefile | 1 + > > > drivers/dma-buf/heaps/coherent_heap.c | 426 > > > ++++++++++++++++++++++++++++++++++ > > > include/linux/dma-heap.h | 11 + > > > include/linux/dma-map-ops.h | 7 + > > > 6 files changed, 456 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > > > index 88189d4e48561..ba87e5ac16ae2 100644 > > > --- a/drivers/dma-buf/dma-heap.c > > > +++ b/drivers/dma-buf/dma-heap.c > > > @@ -390,8 +390,8 @@ struct dma_heap *dma_heap_add(const struct > > > dma_heap_export_info *exp_info) > > > > > > heap = dma_heap_create(exp_info); > > > if (IS_ERR(heap)) { > > > - pr_err("dma_heap: failed to create heap (%d)\n", > > > PTR_ERR(heap)); > > > - return PTR_ERR(heap); > > > + pr_err("dma_heap: failed to create heap (%ld)\n", > > > PTR_ERR(heap)); > > > + return ERR_CAST(heap); > > > > This looks unrelated and should possibly be squashed into the previous > > patch that introduces dma_heap_create()? > > > > > +static int coherent_heap_init_dma_mask(struct device *dev) > > > +{ > > > + int ret; > > > + > > > + ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); > > > + if (!ret) > > > + return 0; > > > + > > > + /* Fallback to 32-bit DMA mask */ > > > + return dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > > > +} > > > > Why do you need to mess with the DMA mask? I'd expect that device to be > > able to access everything. > > When I tested I was getting: "reserved memory is beyond device's set > DMA address range", so I tested if it was fixed with > dma_coerce_mask_and_coherent() and/or dma_set_mask_coherent(). I did > not debug the value of coherent_dma_mask, but given the error I assume > it was not set properly? Ultimately, using the 64 bit mask fixed it, > and I added a 32-bit fallback to ensure support for 32-bit systems.
So you don't need to handle the fallback because dma_coerce_mask_and_coherent will truncate the generated mask to dma_addr_t, which is 64bits on 64 bits platforms, and 32 bits on 32 bits platforms. https://elixir.bootlin.com/linux/v6.19.3/source/kernel/dma/mapping.c#L908 But I think my point was more than there's nothing specific to the coherent heap itself: the device allocated for the heap should have the right mask for any heap, so it's something I'd rather put in dma_heap_add. > > > +static int __coherent_heap_register(struct reserved_mem *rmem) > > > +{ > > > + struct dma_heap_export_info exp_info; > > > + struct coherent_heap *coh_heap; > > > + struct device *heap_dev; > > > + int ret; > > > + > > > + if (!rmem || !rmem->name) > > > + return -EINVAL; > > > + > > > + coh_heap = kzalloc_obj(*coh_heap); > > > + if (!coh_heap) > > > + return -ENOMEM; > > > + > > > + coh_heap->rmem = rmem; > > > + coh_heap->name = kstrdup(rmem->name, GFP_KERNEL); > > > + if (!coh_heap->name) { > > > + ret = -ENOMEM; > > > + goto free_coherent_heap; > > > + } > > > + > > > + exp_info.name = coh_heap->name; > > > + exp_info.ops = &coherent_heap_ops; > > > + exp_info.priv = coh_heap; > > > + > > > + coh_heap->heap = dma_heap_create(&exp_info); > > > + if (IS_ERR(coh_heap->heap)) { > > > + ret = PTR_ERR(coh_heap->heap); > > > + goto free_name; > > > + } > > > + > > > + heap_dev = dma_heap_get_dev(coh_heap->heap); > > > + ret = coherent_heap_init_dma_mask(heap_dev); > > > + if (ret) { > > > + pr_err("coherent_heap: failed to set DMA mask (%d)\n", ret); > > > + goto destroy_heap; > > > + } > > > + > > > + ret = of_reserved_mem_device_init_with_mem(heap_dev, rmem); > > > + if (ret) { > > > + pr_err("coherent_heap: failed to initialize memory (%d)\n", > > > ret); > > > + goto destroy_heap; > > > + } > > > + > > > + ret = dma_heap_register(coh_heap->heap); > > > + if (ret) { > > > + pr_err("coherent_heap: failed to register heap (%d)\n", > > > ret); > > > + goto destroy_heap; > > > + } > > > > I guess it's more of a comment about your previous patch, but it's not > > clear to me why you needed to split dma_heap_add into dma_heap_create / > > _register. Can you expand a bit? > > So first I tried to just use dma_heap_add() and then use the heap_dev > afterward to call of_reserved_mem_device_init_with_mem(), but if that > call failed, the error path required some kind dma_heap_remove() > function as the heap was already registered by then. > > In the CMA heap for example, dma_heap_add() is invoked at the end of > the `init` function. Therefore, you do not have this issue, if it > failed it means the heap was not added and you just need to clean > everything else. > > However, performing a remove() does not sound like something that can > be done safely. I've spent some time thinking on alternatives, but > splitting felt the best pattern. > > This way I can: > 1. Create the device > 2. Call of_reserved_mem_device_init_with_mem > 3. Register the heap > > This places registration at the end, making every error path and > cleanup easy to handle. > > Also, the `dma_heap_add()` code already seemed to handle these two > parts/phases implicitly with device_create(), so splitting felt > architecturally sound. That makes sense, thanks! Maxime
signature.asc
Description: PGP signature
