dma-contiguous: add optional cma_name for cma= kernel parameter

[PATCH] dma-contiguous: add optional cma_name for cma= kernel parameter

Posted by Nate Drude 3 years, 6 months ago

When cma is defined in the device tree, the device tree node
name is used as the cma name. In the following example, the cma
will be named 'linux,cma':

linux,cma {
	compatible = "shared-dma-pool";
	reusable;
	size = <0 0x3c000000>;
	alloc-ranges = <0 0x40000000 0 0xC0000000>;
	linux,cma-default;
};

And a device /dev/dma_heap/linux,cma is created.

However, when cma is provided by command line, a default
name of 'reserved' is used, and the device path changes to
/dev/dma_heap/reserved.

This is problematic because some user space applications,
like gstreamer plugins, are expecting /dev/dma_heap/linux,cma.

This parameter allows overriding the default 'reserved' name.

Signed-off-by: Nate Drude <nate.d@variscite.com>
---
 .../admin-guide/kernel-parameters.txt         |  7 +++++++
 kernel/dma/contiguous.c                       | 21 ++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 51397a320f5e..975ec862d071 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -644,6 +644,13 @@
 			altogether. For more information, see
 			kernel/dma/contiguous.c
 
+	cma_name=	Override the cma heap name
+			Format: <string>
+			When passing the cma kernel parameter, the default
+			cma name is 'reserved'. This parameter allows it to
+			be overriden to align with the device tree name,
+			like 'linux,cma'.
+
 	cma_pernuma=nn[MG]
 			[ARM64,KNL,CMA]
 			Sets the size of kernel per-numa memory area for
diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
index 3d63d91cba5c..e89819ec183e 100644
--- a/kernel/dma/contiguous.c
+++ b/kernel/dma/contiguous.c
@@ -74,6 +74,7 @@ static const phys_addr_t size_bytes __initconst =
 static phys_addr_t  size_cmdline __initdata = -1;
 static phys_addr_t base_cmdline __initdata;
 static phys_addr_t limit_cmdline __initdata;
+static char name_cmdline[CMA_MAX_NAME] = "reserved";
 
 static int __init early_cma(char *p)
 {
@@ -96,6 +97,24 @@ static int __init early_cma(char *p)
 }
 early_param("cma", early_cma);
 
+static int __init early_cma_name(char *p)
+{
+	if (!p) {
+		pr_err("Config string not provided\n");
+		return -EINVAL;
+	}
+
+	if (!strlen(p)) {
+		pr_err("cma_name must have at least one character\n");
+		return -EINVAL;
+	}
+
+	snprintf(name_cmdline, CMA_MAX_NAME, p);
+
+	return 0;
+}
+early_param("cma_name", early_cma_name);
+
 #ifdef CONFIG_DMA_PERNUMA_CMA
 
 static struct cma *dma_contiguous_pernuma_area[MAX_NUMNODES];
@@ -231,7 +250,7 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
 	int ret;
 
 	ret = cma_declare_contiguous(base, size, limit, 0, 0, fixed,
-					"reserved", res_cma);
+					name_cmdline, res_cma);
 	if (ret)
 		return ret;
 
-- 
2.37.3

Re: [PATCH] dma-contiguous: add optional cma_name for cma= kernel parameter

Posted by Robin Murphy 3 years, 6 months ago

On 2022-09-12 17:38, Nate Drude wrote:
> When cma is defined in the device tree, the device tree node
> name is used as the cma name. In the following example, the cma
> will be named 'linux,cma':
> 
> linux,cma {
> 	compatible = "shared-dma-pool";
> 	reusable;
> 	size = <0 0x3c000000>;
> 	alloc-ranges = <0 0x40000000 0 0xC0000000>;
> 	linux,cma-default;
> };
> 
> And a device /dev/dma_heap/linux,cma is created.
> 
> However, when cma is provided by command line, a default
> name of 'reserved' is used, and the device path changes to
> /dev/dma_heap/reserved.

If userspace expects the CMA heap driver to expose a consistent name for 
CMA heaps, shouldn't it be the CMA heap driver's responsibility to 
expose a consistent name for CMA heaps? Tinkering with the core CMA code 
doesn't feel like the right approach.

Furthermore, given that DT reserved-memory nodes carrying the 
"linux-cma-default" property equally can (and do) have different names 
as well, that or fixing userspace really are the only robust options.

Thanks,
Robin.

> This is problematic because some user space applications,
> like gstreamer plugins, are expecting /dev/dma_heap/linux,cma.
> 
> This parameter allows overriding the default 'reserved' name.
> 
> Signed-off-by: Nate Drude <nate.d@variscite.com>
> ---
>   .../admin-guide/kernel-parameters.txt         |  7 +++++++
>   kernel/dma/contiguous.c                       | 21 ++++++++++++++++++-
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 51397a320f5e..975ec862d071 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -644,6 +644,13 @@
>   			altogether. For more information, see
>   			kernel/dma/contiguous.c
>   
> +	cma_name=	Override the cma heap name
> +			Format: <string>
> +			When passing the cma kernel parameter, the default
> +			cma name is 'reserved'. This parameter allows it to
> +			be overriden to align with the device tree name,
> +			like 'linux,cma'.
> +
>   	cma_pernuma=nn[MG]
>   			[ARM64,KNL,CMA]
>   			Sets the size of kernel per-numa memory area for
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index 3d63d91cba5c..e89819ec183e 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -74,6 +74,7 @@ static const phys_addr_t size_bytes __initconst =
>   static phys_addr_t  size_cmdline __initdata = -1;
>   static phys_addr_t base_cmdline __initdata;
>   static phys_addr_t limit_cmdline __initdata;
> +static char name_cmdline[CMA_MAX_NAME] = "reserved";
>   
>   static int __init early_cma(char *p)
>   {
> @@ -96,6 +97,24 @@ static int __init early_cma(char *p)
>   }
>   early_param("cma", early_cma);
>   
> +static int __init early_cma_name(char *p)
> +{
> +	if (!p) {
> +		pr_err("Config string not provided\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!strlen(p)) {
> +		pr_err("cma_name must have at least one character\n");
> +		return -EINVAL;
> +	}
> +
> +	snprintf(name_cmdline, CMA_MAX_NAME, p);
> +
> +	return 0;
> +}
> +early_param("cma_name", early_cma_name);
> +
>   #ifdef CONFIG_DMA_PERNUMA_CMA
>   
>   static struct cma *dma_contiguous_pernuma_area[MAX_NUMNODES];
> @@ -231,7 +250,7 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
>   	int ret;
>   
>   	ret = cma_declare_contiguous(base, size, limit, 0, 0, fixed,
> -					"reserved", res_cma);
> +					name_cmdline, res_cma);
>   	if (ret)
>   		return ret;
>

Re: [PATCH] dma-contiguous: add optional cma_name for cma= kernel parameter

Posted by Christoph Hellwig 3 years, 6 months ago

On Tue, Sep 13, 2022 at 11:45:17AM +0100, Robin Murphy wrote:
> If userspace expects the CMA heap driver to expose a consistent name for 
> CMA heaps, shouldn't it be the CMA heap driver's responsibility to expose a 
> consistent name for CMA heaps? Tinkering with the core CMA code doesn't 
> feel like the right approach.

Agreed.  In fact I think exposing this name in a uapi seems like a really
bad idea that is asking for a lot of trouble.