Hi Jared,
Thanks for working on this
On Tue, Apr 22, 2025 at 12:19:39PM -0700, Jared Kangas wrote:
The CMA heap's name in devtmpfs can vary depending on how the heap is defined. Its name defaults to "reserved", but if a CMA area is defined in the devicetree, the heap takes on the devicetree node's name, such as "default-pool" or "linux,cma". To simplify naming, just name it "default_cma", and keep a legacy node in place backed by the same underlying structure for backwards compatibility.
Signed-off-by: Jared Kangas jkangas@redhat.com
Documentation/userspace-api/dma-buf-heaps.rst | 11 +++++++---- drivers/dma-buf/heaps/Kconfig | 10 ++++++++++ drivers/dma-buf/heaps/cma_heap.c | 14 +++++++++++++- 3 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst index 535f49047ce64..577de813ba461 100644 --- a/Documentation/userspace-api/dma-buf-heaps.rst +++ b/Documentation/userspace-api/dma-buf-heaps.rst @@ -19,7 +19,10 @@ following heaps:
- The ``cma`` heap allocates physically contiguous, cacheable, buffers. Only present if a CMA region is present. Such a region is usually created either through the kernel commandline through the
- `cma` parameter, a memory region Device-Tree node with the
- `linux,cma-default` property set, or through the `CMA_SIZE_MBYTES` or
- `CMA_SIZE_PERCENTAGE` Kconfig options. Depending on the platform, it
- might be called ``reserved``, ``linux,cma``, or ``default-pool``.
- ``cma`` parameter, a memory region Device-Tree node with the
- ``linux,cma-default`` property set, or through the ``CMA_SIZE_MBYTES`` or
- ``CMA_SIZE_PERCENTAGE`` Kconfig options. The heap's name in devtmpfs is
- ``default_cma``. For backwards compatibility, when the
- ``DMABUF_HEAPS_CMA_LEGACY`` Kconfig option is set, a duplicate node is
- created following legacy naming conventions; the legacy name might be
- ``reserved``, ``linux,cma``, or ``default-pool``.
It looks like, in addition to documenting the new naming, you also changed all the backticks to double backticks. Why did you do so? It seems mostly unrelated to that patch, so it would be better in a separate patch.
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig index a5eef06c42264..83f3770fa820a 100644 --- a/drivers/dma-buf/heaps/Kconfig +++ b/drivers/dma-buf/heaps/Kconfig @@ -12,3 +12,13 @@ config DMABUF_HEAPS_CMA Choose this option to enable dma-buf CMA heap. This heap is backed by the Contiguous Memory Allocator (CMA). If your system has these regions, you should say Y here.
+config DMABUF_HEAPS_CMA_LEGACY
- bool "DMA-BUF CMA Heap"
- default y
- depends on DMABUF_HEAPS_CMA
- help
Add a duplicate CMA-backed dma-buf heap with legacy naming derivedfrom the CMA area's devicetree node, or "reserved" if the area is notdefined in the devicetree. This uses the same underlying allocator asCONFIG_DMABUF_HEAPS_CMA.diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c index e998d8ccd1dc6..cd742c961190d 100644 --- a/drivers/dma-buf/heaps/cma_heap.c +++ b/drivers/dma-buf/heaps/cma_heap.c @@ -22,6 +22,7 @@ #include <linux/slab.h> #include <linux/vmalloc.h> +#define DEFAULT_CMA_NAME "default_cma"
I appreciate this is kind of bikeshed-color territory, but I think "cma" would be a better option here. There's nothing "default" about it.
struct cma_heap { struct dma_heap *heap; @@ -394,15 +395,26 @@ static int __init __add_cma_heap(struct cma *cma, const char *name) static int __init add_default_cma_heap(void) { struct cma *default_cma = dev_get_cma_area(NULL);
- const char *legacy_cma_name; int ret;
if (!default_cma) return 0;
- ret = __add_cma_heap(default_cma, cma_get_name(default_cma));
- ret = __add_cma_heap(default_cma, DEFAULT_CMA_NAME); if (ret) return ret;
- legacy_cma_name = cma_get_name(default_cma);
- if (IS_ENABLED(CONFIG_DMABUF_HEAPS_CMA_LEGACY) &&
strcmp(legacy_cma_name, DEFAULT_CMA_NAME)) {ret = __add_cma_heap(default_cma, legacy_cma_name);if (ret)pr_warn("cma_heap: failed to add legacy heap: %pe\n",ERR_PTR(-ret));- }
It would also simplify this part, since you would always create the legacy heap.
Maxime
On Thu, Apr 24, 2025 at 1:34 AM Maxime Ripard mripard@kernel.org wrote:
On Tue, Apr 22, 2025 at 12:19:39PM -0700, Jared Kangas wrote:
@@ -22,6 +22,7 @@ #include <linux/slab.h> #include <linux/vmalloc.h>
+#define DEFAULT_CMA_NAME "default_cma"
I appreciate this is kind of bikeshed-color territory, but I think "cma" would be a better option here. There's nothing "default" about it.
I disagree. It very much is "default" as it's returning the dma_contiguous_default_area.
There can be multiple CMA areas, and out of tree, vendors do reserve separate areas for specific purposes, exposing multiple CMA dmabuf heaps. There have been patches to expose multiple CMA heaps, but with no upstream drivers using those purpose specific regions, we haven't taken them yet. I do hope as the drivers that utilize these purpose focused heaps go upstream, we can add that logic, so I think being specific that this is default CMA is a good idea.
thanks -john
On Thu, Apr 24, 2025 at 05:13:47PM -0700, John Stultz wrote:
On Thu, Apr 24, 2025 at 1:34 AM Maxime Ripard mripard@kernel.org wrote:
On Tue, Apr 22, 2025 at 12:19:39PM -0700, Jared Kangas wrote:
@@ -22,6 +22,7 @@ #include <linux/slab.h> #include <linux/vmalloc.h>
+#define DEFAULT_CMA_NAME "default_cma"
I appreciate this is kind of bikeshed-color territory, but I think "cma" would be a better option here. There's nothing "default" about it.
I disagree. It very much is "default" as it's returning the dma_contiguous_default_area.
My main concern here is that it's "default" as opposed to what, exactly? We have a single CMA allocator. We could have multiple buffer attributes, but then "cached_cma" would make more sense to me if we expect to have uncached CMA allocations at some point.
There can be multiple CMA areas, and out of tree, vendors do reserve separate areas for specific purposes, exposing multiple CMA dmabuf heaps.
By "CMA areas", I guess you mean carved-out memory regions? If so, how is it relevant to userspace if we use CMA or any other implementation to expose a carved-out region, and thus that we carry that implemenattion detail in the name?
There have been patches to expose multiple CMA heaps, but with no upstream drivers using those purpose specific regions, we haven't taken them yet. I do hope as the drivers that utilize these purpose focused heaps go upstream, we can add that logic, so I think being specific that this is default CMA is a good idea.
If heaps names are supposed to carry the region it exposes, then it should be default_cma_region/area. If heap names are supposed to expose the allocator (but I don't think it's a good idea), it should be cma. If they are meant to carry all that plus some policy, cached_default_cma_region should be used.
Either way, default_cma seems to me either too specific or not specific enough. And we should really document what the policy for those heaps are supposed to be.
Maxime
On Thu, Apr 24, 2025 at 11:58 PM Maxime Ripard mripard@kernel.org wrote:
On Thu, Apr 24, 2025 at 05:13:47PM -0700, John Stultz wrote:
On Thu, Apr 24, 2025 at 1:34 AM Maxime Ripard mripard@kernel.org wrote:
I appreciate this is kind of bikeshed-color territory, but I think "cma" would be a better option here. There's nothing "default" about it.
I disagree. It very much is "default" as it's returning the dma_contiguous_default_area.
My main concern here is that it's "default" as opposed to what, exactly? We have a single CMA allocator. We could have multiple buffer attributes, but then "cached_cma" would make more sense to me if we expect to have uncached CMA allocations at some point.
Well, there may be one CMA allocator, but there can be multiple CMA regions.
So in the kernel, cma_alloc() always takes the cma area as an argument. And dma_alloc_contiguous() lets you do allocations against a device, which may reference a specific cma area. Or if the device doesn't specify a region it will utilize the default region.
There can be multiple CMA areas, and out of tree, vendors do reserve separate areas for specific purposes, exposing multiple CMA dmabuf heaps.
By "CMA areas", I guess you mean carved-out memory regions? If so, how is it relevant to userspace if we use CMA or any other implementation to expose a carved-out region, and thus that we carry that implemenattion detail in the name?
So, no, I don't mean carve-out regions. It's more about dealing with competition between multiple CMA users. In some cases, where there are known fixed buffer sizes, say camera buffers, it's much easier to reserve a separate specific sized region to allocate from so that you know it will always succeed and you don't need to waste much on safety margins. Having this added as a separate CMA region makes it a lot easier to account or reason about, and the kernel can still make (limited) use of the CMA space when it's idle. Then you don't have to worry about some other device having a short term cma allocation that pushes back the alignment for your large allocation, possibly impacting some other devices larger allocations.
And unlike with just using a carveout, you don't end up just wasting all that space when it is unused.
So userland may want to allocate contiguous memory, but it may also be relevant to userland to be able to allocate contiguous memory from a purpose specific pool.
And while not used in Android, you could imagine having separate purpose reserved cma heaps with different permissions on the heap devnodes, allowing less trusted applications to allocate cma from a small pool without having the potential to DoS the system.
There have been patches to expose multiple CMA heaps, but with no upstream drivers using those purpose specific regions, we haven't taken them yet. I do hope as the drivers that utilize these purpose focused heaps go upstream, we can add that logic, so I think being specific that this is default CMA is a good idea.
If heaps names are supposed to carry the region it exposes, then it should be default_cma_region/area. If heap names are supposed to expose the allocator (but I don't think it's a good idea), it should be cma. If they are meant to carry all that plus some policy, cached_default_cma_region should be used.
Either way, default_cma seems to me either too specific or not specific enough. And we should really document what the policy for those heaps are supposed to be.
I don't see it as such a problem. It is clear it is cma, it also is clear conceptually that it is the "default" region that the kernel uses when devices aren't specific. But I wouldn't object to cma_default_region/area as a name either, but I don't see it as particularly improved over cma_default.
To your larger point about policy, I do get the tension that you want to be able to programmatically derive or evaluate heap names, so that applications can consistently derive a pathname to get what they want. But I also think that there is so much variety in both the devices and uses that there is no way that all use cases and all devices can be satisfied with such a static or even programmatic mapping. From my perspective, there just is going to have to be some device specific glue logic that maps use->heap name. Same reason we have fstab and the passwd file. That said, I think advocating for naming conventions is definitely useful, but I'm wary of trying to enforce too specific a schema on the names as the incompleteness theorem will bite us.
thanks -john
linaro-mm-sig@lists.linaro.org
-
John Stultz -
Maxime Ripard