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: 6033 bytes --]

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

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

> 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

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