2024-02-08 15:28:22

by Howard Yen

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

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 <[email protected]>
---
Changelog since v2:
(suggested by Andy Shevchenko <[email protected]>)
- 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



2024-02-08 18:03:37

by Andy Shevchenko

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

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



2024-02-13 06:00:07

by Christoph Hellwig

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

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

Why?

2024-02-19 11:12:39

by Howard Yen

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

On Tue, Feb 13, 2024 at 1:54 PM Christoph Hellwig <[email protected]> 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

2024-02-19 11:31:46

by Howard Yen

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

On Fri, Feb 9, 2024 at 2:02 AM Andy Shevchenko
<[email protected]> 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

2024-02-19 11:37:32

by Howard Yen

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

On Tue, Feb 13, 2024 at 1:54 PM Christoph Hellwig <[email protected]> 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

2024-02-20 05:53:00

by Christoph Hellwig

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

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.

2024-02-21 09:37:07

by Howard Yen

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

On Tue, Feb 20, 2024 at 1:52 PM Christoph Hellwig <[email protected]> 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

2024-02-23 06:37:34

by Christoph Hellwig

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

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?


2024-02-27 14:10:56

by Howard Yen

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

On Fri, Feb 23, 2024 at 2:37 PM Christoph Hellwig <[email protected]> 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

2024-02-27 14:33:58

by Robin Murphy

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

On 27/02/2024 1:39 pm, Howard Yen wrote:
> On Fri, Feb 23, 2024 at 2:37 PM Christoph Hellwig <[email protected]> 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

2024-03-04 09:48:55

by Howard Yen

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

On Tue, Feb 27, 2024 at 10:31 PM Robin Murphy <[email protected]> wrote:
>
> On 27/02/2024 1:39 pm, Howard Yen wrote:
> > On Fri, Feb 23, 2024 at 2:37 PM Christoph Hellwig <[email protected]> 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