On Tue, Mar 3, 2026 at 2:20 PM Maxime Ripard <[email protected]> wrote:
>
> Hi,
>
> 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.
>
> > +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.
>
> > diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> > index 1b0ea43ba66c3..77e6cb66ffce1 100644
> > --- a/include/linux/dma-heap.h
> > +++ b/include/linux/dma-heap.h
> > @@ -9,10 +9,12 @@
> > #ifndef _DMA_HEAPS_H
> > #define _DMA_HEAPS_H
> >
> > +#include <linux/errno.h>
> > #include <linux/types.h>
> >
> > struct dma_heap;
> > struct device;
> > +struct reserved_mem;
> >
> > /**
> > * struct dma_heap_ops - ops to operate on a given heap
> > @@ -53,4 +55,13 @@ struct dma_heap *dma_heap_add(const struct
> > dma_heap_export_info *exp_info);
> >
> > extern bool mem_accounting;
> >
> > +#if IS_ENABLED(CONFIG_DMABUF_HEAPS_COHERENT)
> > +int dma_heap_coherent_register(struct reserved_mem *rmem);
> > +#else
> > +static inline int dma_heap_coherent_register(struct reserved_mem *rmem)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +#endif
> > +
> > #endif /* _DMA_HEAPS_H */
>
> Do you still need that now that you switched to an iterator-like
> function?
>
> > diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
> > index 60b63756df821..c87e5e44e5383 100644
> > --- a/include/linux/dma-map-ops.h
> > +++ b/include/linux/dma-map-ops.h
> > @@ -12,6 +12,7 @@
> >
> > struct cma;
> > struct iommu_ops;
> > +struct reserved_mem;
> >
> > struct dma_map_ops {
> > void *(*alloc)(struct device *dev, size_t size,
> > @@ -161,6 +162,7 @@ int dma_alloc_from_dev_coherent(struct device *dev,
> > ssize_t size,
> > int dma_release_from_dev_coherent(struct device *dev, int order, void
> > *vaddr);
> > int dma_mmap_from_dev_coherent(struct device *dev, struct vm_area_struct
> > *vma,
> > void *cpu_addr, size_t size, int *ret);
> > +struct reserved_mem *dma_coherent_get_reserved_region(unsigned int idx);
> > #else
> > static inline int dma_declare_coherent_memory(struct device *dev,
> > phys_addr_t phys_addr, dma_addr_t device_addr, size_t size)
> > @@ -172,6 +174,11 @@ static inline int dma_declare_coherent_memory(struct
> > device *dev,
> > #define dma_release_from_dev_coherent(dev, order, vaddr) (0)
> > #define dma_mmap_from_dev_coherent(dev, vma, vaddr, order, ret) (0)
> > static inline void dma_release_coherent_memory(struct device *dev) { }
> > +static inline
> > +struct reserved_mem *dma_coherent_get_reserved_region(unsigned int idx)
> > +{
> > + return NULL;
> > +}
> > #endif /* CONFIG_DMA_DECLARE_COHERENT */
> >
> > #ifdef CONFIG_DMA_GLOBAL_POOL
>
> To preserve bisectability, you shouldn't do it that way. Introduce this
> function into a preliminary patch, and then use it in this one.
>
> Maxime
