[PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Howard Yen]

Add support for multiple coherent rmems per device. This patch replaces
original dma_mem with dma_mems list in device structure to store multiple
rmems.

These multiple rmems can be assigned to the device one by one by
of_reserved_mem_device_init_by_idx() with the memory-region
declaration in device tree as below and store the rmem to the dma_mems
list.

	device1@0 {
		...
		memory-region = <&reserved_mem0>, <&reserved_mem1>;
		...
	};

When driver tries to allocate memory from the rmems, looks for the first
available rmem and allocates the memory from this rmem.

Then if driver removed, of_reserved_mem_device_release() needs to be
invoked to release all the rmems assigned to the device.

Signed-off-by: Howard Yen <howardyen@google.com>
---
Changelog since v2:
(suggested by Andy Shevchenko <andriy.shevchenko@linux.intel.com>)
- Re-org the members of struct dma_coherent_mem to avoid additional
  pointer arithmetics and the holes inside the structure.
- Use consistent naming of return value.
- Re-write the dev checking statement to be more clear.

 drivers/base/core.c    |  3 ++
 include/linux/device.h |  5 +--
 kernel/dma/coherent.c  | 92 +++++++++++++++++++++++++++---------------
 3 files changed, 64 insertions(+), 36 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14d46af40f9a..d9af38d7b870 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3108,6 +3108,9 @@ void device_initialize(struct device *dev)
     defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
     defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
 	dev->dma_coherent = dma_default_coherent;
+#endif
+#ifdef CONFIG_DMA_DECLARE_COHERENT
+	INIT_LIST_HEAD(&dev->dma_mems);
 #endif
 	swiotlb_dev_init(dev);
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index 97c4b046c09d..5fa15e5adbdc 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -648,7 +648,7 @@ struct device_physical_location {
  * @dma_parms:	A low level driver may set these to teach IOMMU code about
  * 		segment limitations.
  * @dma_pools:	Dma pools (if dma'ble device).
- * @dma_mem:	Internal for coherent mem override.
+ * @dma_mems:	Internal for coherent mems.
  * @cma_area:	Contiguous memory area for dma allocations
  * @dma_io_tlb_mem: Software IO TLB allocator.  Not for driver use.
  * @dma_io_tlb_pools:	List of transient swiotlb memory pools.
@@ -749,8 +749,7 @@ struct device {
 	struct list_head	dma_pools;	/* dma pools (if dma'ble) */
 
 #ifdef CONFIG_DMA_DECLARE_COHERENT
-	struct dma_coherent_mem	*dma_mem; /* internal for coherent mem
-					     override */
+	struct list_head	dma_mems; /* Internal for coherent mems */
 #endif
 #ifdef CONFIG_DMA_CMA
 	struct cma *cma_area;		/* contiguous memory area for dma
diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
index ff5683a57f77..f6748a3a5eb1 100644
--- a/kernel/dma/coherent.c
+++ b/kernel/dma/coherent.c
@@ -11,22 +11,16 @@
 #include <linux/dma-map-ops.h>
 
 struct dma_coherent_mem {
-	void		*virt_base;
-	dma_addr_t	device_base;
-	unsigned long	pfn_base;
-	int		size;
-	unsigned long	*bitmap;
-	spinlock_t	spinlock;
-	bool		use_dev_dma_pfn_offset;
+	struct list_head	node;
+	void			*virt_base;
+	dma_addr_t		device_base;
+	unsigned long		pfn_base;
+	int			size;
+	spinlock_t		spinlock;
+	unsigned long		*bitmap;
+	bool			use_dev_dma_pfn_offset;
 };
 
-static inline struct dma_coherent_mem *dev_get_coherent_memory(struct device *dev)
-{
-	if (dev && dev->dma_mem)
-		return dev->dma_mem;
-	return NULL;
-}
-
 static inline dma_addr_t dma_get_device_base(struct device *dev,
 					     struct dma_coherent_mem * mem)
 {
@@ -61,6 +55,7 @@ static struct dma_coherent_mem *dma_init_coherent_memory(phys_addr_t phys_addr,
 	dma_mem->pfn_base = PFN_DOWN(phys_addr);
 	dma_mem->size = pages;
 	dma_mem->use_dev_dma_pfn_offset = use_dma_pfn_offset;
+	INIT_LIST_HEAD(&dma_mem->node);
 	spin_lock_init(&dma_mem->spinlock);
 
 	return dma_mem;
@@ -90,10 +85,8 @@ static int dma_assign_coherent_memory(struct device *dev,
 	if (!dev)
 		return -ENODEV;
 
-	if (dev->dma_mem)
-		return -EBUSY;
+	list_add_tail(&mem->node, &dev->dma_mems);
 
-	dev->dma_mem = mem;
 	return 0;
 }
 
@@ -118,23 +111,28 @@ int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
 				dma_addr_t device_addr, size_t size)
 {
 	struct dma_coherent_mem *mem;
-	int ret;
+	int retval;
 
 	mem = dma_init_coherent_memory(phys_addr, device_addr, size, false);
 	if (IS_ERR(mem))
 		return PTR_ERR(mem);
 
-	ret = dma_assign_coherent_memory(dev, mem);
-	if (ret)
+	retval = dma_assign_coherent_memory(dev, mem);
+	if (retval)
 		_dma_release_coherent_memory(mem);
-	return ret;
+	return retval;
 }
 
 void dma_release_coherent_memory(struct device *dev)
 {
-	if (dev) {
-		_dma_release_coherent_memory(dev->dma_mem);
-		dev->dma_mem = NULL;
+	struct dma_coherent_mem *mem_tmp, *q;
+
+	if (!dev)
+		return;
+
+	list_for_each_entry_safe(mem_tmp, q, &dev->dma_mems, node) {
+		list_del_init(&mem_tmp->node);
+		_dma_release_coherent_memory(mem_tmp);
 	}
 }
 
@@ -187,12 +185,17 @@ static void *__dma_alloc_from_coherent(struct device *dev,
 int dma_alloc_from_dev_coherent(struct device *dev, ssize_t size,
 		dma_addr_t *dma_handle, void **ret)
 {
-	struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
+	struct dma_coherent_mem *mem_tmp;
 
-	if (!mem)
+	if (list_empty(&dev->dma_mems))
 		return 0;
 
-	*ret = __dma_alloc_from_coherent(dev, mem, size, dma_handle);
+	list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
+		*ret = __dma_alloc_from_coherent(dev, mem_tmp, size, dma_handle);
+		if (*ret)
+			break;
+	}
+
 	return 1;
 }
 
@@ -226,9 +229,16 @@ static int __dma_release_from_coherent(struct dma_coherent_mem *mem,
  */
 int dma_release_from_dev_coherent(struct device *dev, int order, void *vaddr)
 {
-	struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
+	struct dma_coherent_mem *mem_tmp;
+	int retval = 0;
+
+	list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
+		retval = __dma_release_from_coherent(mem_tmp, order, vaddr);
+		if (retval == 1)
+			break;
+	}
 
-	return __dma_release_from_coherent(mem, order, vaddr);
+	return retval;
 }
 
 static int __dma_mmap_from_coherent(struct dma_coherent_mem *mem,
@@ -271,9 +281,16 @@ static int __dma_mmap_from_coherent(struct dma_coherent_mem *mem,
 int dma_mmap_from_dev_coherent(struct device *dev, struct vm_area_struct *vma,
 			   void *vaddr, size_t size, int *ret)
 {
-	struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
+	struct dma_coherent_mem *mem_tmp;
+	int retval = 0;
 
-	return __dma_mmap_from_coherent(mem, vma, vaddr, size, ret);
+	list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
+		retval = __dma_mmap_from_coherent(mem_tmp, vma, vaddr, size, ret);
+		if (retval == 1)
+			break;
+	}
+
+	return retval;
 }
 
 #ifdef CONFIG_DMA_GLOBAL_POOL
@@ -351,8 +368,17 @@ static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev)
 static void rmem_dma_device_release(struct reserved_mem *rmem,
 				    struct device *dev)
 {
-	if (dev)
-		dev->dma_mem = NULL;
+	struct dma_coherent_mem *mem_tmp, *q;
+
+	if (!dev)
+		return;
+
+	list_for_each_entry_safe(mem_tmp, q, &dev->dma_mems, node) {
+		if (mem_tmp == rmem->priv) {
+			list_del_init(&mem_tmp->node);
+			break;
+		}
+	}
 }
 
 static const struct reserved_mem_ops rmem_dma_ops = {
-- 
2.43.0.594.gd9cf4e227d-goog

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Andy Shevchenko]

On Thu, Feb 08, 2024 at 03:28:05PM +0000, Howard Yen wrote:
> Add support for multiple coherent rmems per device. This patch replaces
> original dma_mem with dma_mems list in device structure to store multiple
> rmems.
> 
> These multiple rmems can be assigned to the device one by one by
> of_reserved_mem_device_init_by_idx() with the memory-region
> declaration in device tree as below and store the rmem to the dma_mems
> list.
> 
> 	device1@0 {
> 		...
> 		memory-region = <&reserved_mem0>, <&reserved_mem1>;
> 		...
> 	};
> 
> When driver tries to allocate memory from the rmems, looks for the first
> available rmem and allocates the memory from this rmem.
> 
> Then if driver removed, of_reserved_mem_device_release() needs to be
> invoked to release all the rmems assigned to the device.

...

>  	struct dma_coherent_mem *mem;
> -	int ret;
> +	int retval;
>  
>  	mem = dma_init_coherent_memory(phys_addr, device_addr, size, false);
>  	if (IS_ERR(mem))
>  		return PTR_ERR(mem);
>  
> -	ret = dma_assign_coherent_memory(dev, mem);
> -	if (ret)
> +	retval = dma_assign_coherent_memory(dev, mem);
> +	if (retval)
>  		_dma_release_coherent_memory(mem);
> -	return ret;
> +	return retval;

This is unrelated change.

But why? Do you have retval in the _existing_ code elsewhere?


...

>  int dma_alloc_from_dev_coherent(struct device *dev, ssize_t size,
>  		dma_addr_t *dma_handle, void **ret)
>  {
> -	struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> +	struct dma_coherent_mem *mem_tmp;
>  
> -	if (!mem)
> +	if (list_empty(&dev->dma_mems))
>  		return 0;
>  
> -	*ret = __dma_alloc_from_coherent(dev, mem, size, dma_handle);
> +	list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> +		*ret = __dma_alloc_from_coherent(dev, mem_tmp, size, dma_handle);
> +		if (*ret)
> +			break;

This bails out on the first success. Moreover, if one calls this function
again, it will rewrite the existing allocation. Is this all expected?

OTOH, if you add multiple entries and bailing out on error condition it should
be clear if the previous allocations have to be released.

> +	}

>  	return 1;

>  }

...

>  int dma_release_from_dev_coherent(struct device *dev, int order, void *vaddr)
>  {
> -	struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> +	struct dma_coherent_mem *mem_tmp;
> +	int retval = 0;
> +
> +	list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> +		retval = __dma_release_from_coherent(mem_tmp, order, vaddr);
> +		if (retval == 1)
> +			break;

Same Q here.

> +	}
>  
> -	return __dma_release_from_coherent(mem, order, vaddr);
> +	return retval;
>  }

...

>  int dma_mmap_from_dev_coherent(struct device *dev, struct vm_area_struct *vma,
>  			   void *vaddr, size_t size, int *ret)
>  {
> -	struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> +	struct dma_coherent_mem *mem_tmp;
> +	int retval = 0;
>  
> -	return __dma_mmap_from_coherent(mem, vma, vaddr, size, ret);
> +	list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> +		retval = __dma_mmap_from_coherent(mem_tmp, vma, vaddr, size, ret);
> +		if (retval == 1)
> +			break;

And here.

> +	}
> +
> +	return retval;
>  }

...

With the above Q in mind, here is another one: Why can't we allocate all at once?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Christoph Hellwig]

On Thu, Feb 08, 2024 at 03:28:05PM +0000, Howard Yen wrote:
> Add support for multiple coherent rmems per device.

Why?

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Howard Yen]

On Tue, Feb 13, 2024 at 1:54 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Feb 08, 2024 at 03:28:05PM +0000, Howard Yen wrote:
> > Add support for multiple coherent rmems per device.
>
> Why?

I tried to upload the patch to support multiple coherent rmems per device
because in some system, the primary memory space for the device might
be limited, so that add multiple coherent rmems support per device to satisfy
the scenario.

-- 
Regards,

Howard

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Howard Yen]

On Tue, Feb 13, 2024 at 1:54 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Feb 08, 2024 at 03:28:05PM +0000, Howard Yen wrote:
> > Add support for multiple coherent rmems per device.
>
> Why?

Resend the message because the previous one contained some HTML.

I tried to upload the patch to support multiple coherent rmems per device
because in some system, the primary memory space for the device might
be limited, so that add multiple coherent rmems support per device to satisfy
the scenario.


-- 
Regards,

Howard

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Howard Yen]

On Fri, Feb 9, 2024 at 2:02 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 08, 2024 at 03:28:05PM +0000, Howard Yen wrote:
> > Add support for multiple coherent rmems per device. This patch replaces
> > original dma_mem with dma_mems list in device structure to store multiple
> > rmems.
> >
> > These multiple rmems can be assigned to the device one by one by
> > of_reserved_mem_device_init_by_idx() with the memory-region
> > declaration in device tree as below and store the rmem to the dma_mems
> > list.
> >
> >       device1@0 {
> >               ...
> >               memory-region = <&reserved_mem0>, <&reserved_mem1>;
> >               ...
> >       };
> >
> > When driver tries to allocate memory from the rmems, looks for the first
> > available rmem and allocates the memory from this rmem.
> >
> > Then if driver removed, of_reserved_mem_device_release() needs to be
> > invoked to release all the rmems assigned to the device.
>
> ...
>
> >       struct dma_coherent_mem *mem;
> > -     int ret;
> > +     int retval;
> >
> >       mem = dma_init_coherent_memory(phys_addr, device_addr, size, false);
> >       if (IS_ERR(mem))
> >               return PTR_ERR(mem);
> >
> > -     ret = dma_assign_coherent_memory(dev, mem);
> > -     if (ret)
> > +     retval = dma_assign_coherent_memory(dev, mem);
> > +     if (retval)
> >               _dma_release_coherent_memory(mem);
> > -     return ret;
> > +     return retval;
>
> This is unrelated change.
>
> But why? Do you have retval in the _existing_ code elsewhere?

Rename the 'ret' variable to 'retval' because, in the
dma_mmap_from_dev_coherent(),
there is an argument named 'ret', and also I add 'retval' for the return value.
So I try to rename it here to be consistent.

>
>
> ...
>
> >  int dma_alloc_from_dev_coherent(struct device *dev, ssize_t size,
> >               dma_addr_t *dma_handle, void **ret)
> >  {
> > -     struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> > +     struct dma_coherent_mem *mem_tmp;
> >
> > -     if (!mem)
> > +     if (list_empty(&dev->dma_mems))
> >               return 0;
> >
> > -     *ret = __dma_alloc_from_coherent(dev, mem, size, dma_handle);
> > +     list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> > +             *ret = __dma_alloc_from_coherent(dev, mem_tmp, size, dma_handle);
> > +             if (*ret)
> > +                     break;
>
> This bails out on the first success. Moreover, if one calls this function
> again, it will rewrite the existing allocation. Is this all expected?
>
> OTOH, if you add multiple entries and bailing out on error condition it should
> be clear if the previous allocations have to be released.

If it's not able to allocate required memory from the first entry,
then tries to allocate
from the following entry, and so on.

>
> > +     }
>
> >       return 1;
>
> >  }
>
> ...
>
> >  int dma_release_from_dev_coherent(struct device *dev, int order, void *vaddr)
> >  {
> > -     struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> > +     struct dma_coherent_mem *mem_tmp;
> > +     int retval = 0;
> > +
> > +     list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> > +             retval = __dma_release_from_coherent(mem_tmp, order, vaddr);
> > +             if (retval == 1)
> > +                     break;
>
> Same Q here.
>
> > +     }
> >
> > -     return __dma_release_from_coherent(mem, order, vaddr);
> > +     return retval;
> >  }
>
> ...
>
> >  int dma_mmap_from_dev_coherent(struct device *dev, struct vm_area_struct *vma,
> >                          void *vaddr, size_t size, int *ret)
> >  {
> > -     struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> > +     struct dma_coherent_mem *mem_tmp;
> > +     int retval = 0;
> >
> > -     return __dma_mmap_from_coherent(mem, vma, vaddr, size, ret);
> > +     list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> > +             retval = __dma_mmap_from_coherent(mem_tmp, vma, vaddr, size, ret);
> > +             if (retval == 1)
> > +                     break;
>
> And here.
>
> > +     }
> > +
> > +     return retval;
> >  }
>
> ...
>
> With the above Q in mind, here is another one: Why can't we allocate all at once?

Not sure if I misunderstand your meaning, It tries to allocate the
memory from the first
entry that satisfies the allocation requirement, but not separately.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Regards,

Howard

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Christoph Hellwig]

On Mon, Feb 19, 2024 at 07:12:18PM +0800, Howard Yen wrote:
> I tried to upload the patch to support multiple coherent rmems per device
> because in some system, the primary memory space for the device might
> be limited, so that add multiple coherent rmems support per device to satisfy
> the scenario.

I'm not sure what the means.

If you have non-trivial patches you really need to explain why you're
doing in detail as you need to convince maintainers that it is useful.

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Howard Yen]

On Tue, Feb 20, 2024 at 1:52 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Feb 19, 2024 at 07:12:18PM +0800, Howard Yen wrote:
> > I tried to upload the patch to support multiple coherent rmems per device
> > because in some system, the primary memory space for the device might
> > be limited, so that add multiple coherent rmems support per device to satisfy
> > the scenario.
>
> I'm not sure what the means.
>
> If you have non-trivial patches you really need to explain why you're
> doing in detail as you need to convince maintainers that it is useful.

Thanks for the response, let me explain more.

The reason why I tried to propose this patch is that in the system I'm
working on, where the driver utilizes the coherent reserved memory in
the subsystem for DMA, which has limited memory space as its primary
usage. During the execution of the driver, there is a possibility of
encountering memory depletion scenarios with the primary one.

To address this issue, I tried to create a patch that enables the
coherent reserved memory driver to support multiple coherent reserved
memory regions per device. This modification aims to provide the
driver with the ability to search for memory from a secondary region
if the primary memory is exhausted, and so on.

--
Regards,

Howard

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Christoph Hellwig]

On Wed, Feb 21, 2024 at 05:27:58PM +0800, Howard Yen wrote:
> The reason why I tried to propose this patch is that in the system I'm
> working on, where the driver utilizes the coherent reserved memory in
> the subsystem for DMA, which has limited memory space as its primary
> usage. During the execution of the driver, there is a possibility of
> encountering memory depletion scenarios with the primary one.
> 
> To address this issue, I tried to create a patch that enables the
> coherent reserved memory driver to support multiple coherent reserved
> memory regions per device. This modification aims to provide the
> driver with the ability to search for memory from a secondary region
> if the primary memory is exhausted, and so on.

This all seems pretty vague.  Can you point to your driver submission
and explain why it can't just use a larger region instead of multiple
smaller ones?

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Howard Yen]

On Fri, Feb 23, 2024 at 2:37 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Feb 21, 2024 at 05:27:58PM +0800, Howard Yen wrote:
> > The reason why I tried to propose this patch is that in the system I'm
> > working on, where the driver utilizes the coherent reserved memory in
> > the subsystem for DMA, which has limited memory space as its primary
> > usage. During the execution of the driver, there is a possibility of
> > encountering memory depletion scenarios with the primary one.
> >
> > To address this issue, I tried to create a patch that enables the
> > coherent reserved memory driver to support multiple coherent reserved
> > memory regions per device. This modification aims to provide the
> > driver with the ability to search for memory from a secondary region
> > if the primary memory is exhausted, and so on.
>
> This all seems pretty vague.  Can you point to your driver submission
> and explain why it can't just use a larger region instead of multiple
> smaller ones?
>

The reason why it needs multiple regions is that in my system there is
an always-on subsystem which includes a small size memory, and several
functions need to run and occupy the memory from the small memory if
they need to run on the always-on subsystem. These functions must
allocate the memory from the small memory region, so that they can get
benefit from the always-on subsystem. So the small memory is split for
multiple functions which are satisfied with their generic use cases.
But in specific use cases, they required more memory than their
pre-allocated memory region, so I tried to propose this patch to give
it the ability to get the memory from another larger memory to solve
the issue.

I'll upload the next version to show the modification in the driver.

---
Best Regards,

Howard

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Robin Murphy]

On 27/02/2024 1:39 pm, Howard Yen wrote:
> On Fri, Feb 23, 2024 at 2:37 PM Christoph Hellwig <hch@lst.de> wrote:
>>
>> On Wed, Feb 21, 2024 at 05:27:58PM +0800, Howard Yen wrote:
>>> The reason why I tried to propose this patch is that in the system I'm
>>> working on, where the driver utilizes the coherent reserved memory in
>>> the subsystem for DMA, which has limited memory space as its primary
>>> usage. During the execution of the driver, there is a possibility of
>>> encountering memory depletion scenarios with the primary one.
>>>
>>> To address this issue, I tried to create a patch that enables the
>>> coherent reserved memory driver to support multiple coherent reserved
>>> memory regions per device. This modification aims to provide the
>>> driver with the ability to search for memory from a secondary region
>>> if the primary memory is exhausted, and so on.
>>
>> This all seems pretty vague.  Can you point to your driver submission
>> and explain why it can't just use a larger region instead of multiple
>> smaller ones?
>>
> 
> The reason why it needs multiple regions is that in my system there is
> an always-on subsystem which includes a small size memory, and several
> functions need to run and occupy the memory from the small memory if
> they need to run on the always-on subsystem. These functions must
> allocate the memory from the small memory region, so that they can get
> benefit from the always-on subsystem. So the small memory is split for
> multiple functions which are satisfied with their generic use cases.

I don't really see how that aligns with what you've implemented, though. 
The coherent allocator doesn't have any notion of the caller's use-case, 
it's just going to allocate from wherever it happens to find space 
first. Thus even the calls which would somehow benefit from allocating 
from the "primary" region will have no way to guarantee that they will 
actually allocate from there if it's already been consumed by callers 
who didn't need that benefit but just happened to get there first.

Really that sounds like a case for having specific named memory-regions 
and managing them directly from the relevant driver, rather than trying 
to convince the generic dma-coherent abstraction to do things that don't 
really fit it. Otherwise I'd be slightly concerned that you're expecting 
to bake secret undocumented ABI into DMA API implementations where some 
particular order of allocations must guarantee a particular 
deterministic behaviour, which is really not something we want.

Thanks,
Robin.

> But in specific use cases, they required more memory than their
> pre-allocated memory region, so I tried to propose this patch to give
> it the ability to get the memory from another larger memory to solve
> the issue.
> 
> I'll upload the next version to show the modification in the driver.
> 
> ---
> Best Regards,
> 
> Howard

Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Howard Yen]

On Tue, Feb 27, 2024 at 10:31 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 27/02/2024 1:39 pm, Howard Yen wrote:
> > On Fri, Feb 23, 2024 at 2:37 PM Christoph Hellwig <hch@lst.de> wrote:
> >>
> >> On Wed, Feb 21, 2024 at 05:27:58PM +0800, Howard Yen wrote:
> >>> The reason why I tried to propose this patch is that in the system I'm
> >>> working on, where the driver utilizes the coherent reserved memory in
> >>> the subsystem for DMA, which has limited memory space as its primary
> >>> usage. During the execution of the driver, there is a possibility of
> >>> encountering memory depletion scenarios with the primary one.
> >>>
> >>> To address this issue, I tried to create a patch that enables the
> >>> coherent reserved memory driver to support multiple coherent reserved
> >>> memory regions per device. This modification aims to provide the
> >>> driver with the ability to search for memory from a secondary region
> >>> if the primary memory is exhausted, and so on.
> >>
> >> This all seems pretty vague.  Can you point to your driver submission
> >> and explain why it can't just use a larger region instead of multiple
> >> smaller ones?
> >>
> >
> > The reason why it needs multiple regions is that in my system there is
> > an always-on subsystem which includes a small size memory, and several
> > functions need to run and occupy the memory from the small memory if
> > they need to run on the always-on subsystem. These functions must
> > allocate the memory from the small memory region, so that they can get
> > benefit from the always-on subsystem. So the small memory is split for
> > multiple functions which are satisfied with their generic use cases.
>
> I don't really see how that aligns with what you've implemented, though.
> The coherent allocator doesn't have any notion of the caller's use-case,
> it's just going to allocate from wherever it happens to find space
> first. Thus even the calls which would somehow benefit from allocating
> from the "primary" region will have no way to guarantee that they will
> actually allocate from there if it's already been consumed by callers
> who didn't need that benefit but just happened to get there first.
>
> Really that sounds like a case for having specific named memory-regions
> and managing them directly from the relevant driver, rather than trying
> to convince the generic dma-coherent abstraction to do things that don't
> really fit it. Otherwise I'd be slightly concerned that you're expecting
> to bake secret undocumented ABI into DMA API implementations where some
> particular order of allocations must guarantee a particular
> deterministic behaviour, which is really not something we want.

Thanks for the response.

The original problem I tried to resolve is the use-case explained in
the previous reply, and I was thinking of implementing it in a generic
way. Then I tried to submit this patch. As you mentioned here, it
provides the benefit that if somehow the "primary" region has no way
to guarantee for the callers. And my use-case is one of its uses.


---
Best Regards,

Howard

>
> Thanks,
> Robin.
>
> > But in specific use cases, they required more memory than their
> > pre-allocated memory region, so I tried to propose this patch to give
> > it the ability to get the memory from another larger memory to solve
> > the issue.
> >
> > I'll upload the next version to show the modification in the driver.
> >
> > ---
> > Best Regards,
> >
> > Howard



-- 
Best Regards,

Howard