Re: [PATCH v2 4/6] dma-buf: heaps: Add Coherent heap to dmabuf heaps

[Maxime Ripard <mripard@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6420 bytes --]

On Tue, Mar 03, 2026 at 03:47:14PM +0100, Albert Esteve wrote:
> On Tue, Mar 3, 2026 at 2:20 PM Maxime Ripard <mripard@redhat.com> 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 <aesteve@redhat.com>
> > > ---
> > >  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]