2020-05-01 07:41:37

by John Stultz

[permalink] [raw]
Subject: [RFC][PATCH 0/4] Support non-default CMA regions to the dmabuf heaps interface

This is a much belated second stab at allowing non-default CMA
regions to be exposed via the dmabuf heaps interface.

Previous attempt was here:
https://lore.kernel.org/lkml/[email protected]/T/

This pass tried to take Rob's earlier suggestion to use a flag
property.

Feedback would be greatly welcome!

thanks
-john

Cc: Rob Herring <[email protected]>
Cc: Sumit Semwal <[email protected]>
Cc: "Andrew F. Davis" <[email protected]>
Cc: Benjamin Gaignard <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Pratik Patel <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Chenbo Feng <[email protected]>
Cc: Alistair Strachan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

John Stultz (4):
devicetree: bindings: Add linux,cma-heap tag for reserved memory
mm: cma: Add dma_heap flag to cma structure
dma-buf: cma_heap: Extend logic to export CMA regions tagged with
"linux,cma-heap"
example: dts: hi3660-hikey960: Add dts entries to test cma heap
binding

.../reserved-memory/reserved-memory.txt | 3 +++
.../boot/dts/hisilicon/hi3660-hikey960.dts | 7 +++++++
drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
include/linux/cma.h | 3 +++
kernel/dma/contiguous.c | 3 +++
mm/cma.c | 11 +++++++++++
mm/cma.h | 1 +
7 files changed, 37 insertions(+), 9 deletions(-)

--
2.17.1


2020-05-01 07:41:46

by John Stultz

[permalink] [raw]
Subject: [RFC][PATCH 1/4] devicetree: bindings: Add linux,cma-heap tag for reserved memory

This patch adds a linux,cma-heap property for CMA reserved memory
regions, which will be used to allow the region to be exposed via
the DMA-BUF Heaps interface

Cc: Rob Herring <[email protected]>
Cc: Sumit Semwal <[email protected]>
Cc: "Andrew F. Davis" <[email protected]>
Cc: Benjamin Gaignard <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Pratik Patel <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Chenbo Feng <[email protected]>
Cc: Alistair Strachan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
.../devicetree/bindings/reserved-memory/reserved-memory.txt | 3 +++
1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
index bac4afa3b197..e97b6a4c3bc0 100644
--- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
@@ -68,6 +68,9 @@ Linux implementation note:
- If a "linux,cma-default" property is present, then Linux will use the
region for the default pool of the contiguous memory allocator.

+- If a "linux,cma-heap" property is present, then Linux will expose the
+ the CMA region via the DMA-BUF Heaps interface.
+
- If a "linux,dma-default" property is present, then Linux will use the
region for the default pool of the consistent DMA allocator.

--
2.17.1

2020-05-01 07:41:51

by John Stultz

[permalink] [raw]
Subject: [RFC][PATCH 2/4] mm: cma: Add dma_heap flag to cma structure

This patch adds a dma_heap flag on the cma structure,
along with accessors to set and read the flag.

This is then used to process and store the "linux,cma-heap"
property documented in the previous patch.

Cc: Rob Herring <[email protected]>
Cc: Sumit Semwal <[email protected]>
Cc: "Andrew F. Davis" <[email protected]>
Cc: Benjamin Gaignard <benjamin.gaignard[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Pratik Patel <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Chenbo Feng <[email protected]>
Cc: Alistair Strachan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
include/linux/cma.h | 3 +++
kernel/dma/contiguous.c | 3 +++
mm/cma.c | 11 +++++++++++
mm/cma.h | 1 +
4 files changed, 18 insertions(+)

diff --git a/include/linux/cma.h b/include/linux/cma.h
index 6ff79fefd01f..d8b8e6ce221c 100644
--- a/include/linux/cma.h
+++ b/include/linux/cma.h
@@ -25,6 +25,9 @@ extern phys_addr_t cma_get_base(const struct cma *cma);
extern unsigned long cma_get_size(const struct cma *cma);
extern const char *cma_get_name(const struct cma *cma);

+extern void __init cma_enable_dma_heap(struct cma *cma, bool enabled);
+extern bool cma_dma_heap_enabled(struct cma *cma);
+
extern int __init cma_declare_contiguous_nid(phys_addr_t base,
phys_addr_t size, phys_addr_t limit,
phys_addr_t alignment, unsigned int order_per_bit,
diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
index 8bc6f2d670f9..f667fd51daa2 100644
--- a/kernel/dma/contiguous.c
+++ b/kernel/dma/contiguous.c
@@ -303,6 +303,7 @@ static int __init rmem_cma_setup(struct reserved_mem *rmem)
phys_addr_t mask = align - 1;
unsigned long node = rmem->fdt_node;
bool default_cma = of_get_flat_dt_prop(node, "linux,cma-default", NULL);
+ bool heap_exported = of_get_flat_dt_prop(node, "linux,cma-heap", NULL);
struct cma *cma;
int err;

@@ -332,6 +333,8 @@ static int __init rmem_cma_setup(struct reserved_mem *rmem)
if (default_cma)
dma_contiguous_set_default(cma);

+ cma_enable_dma_heap(cma, heap_exported);
+
rmem->ops = &rmem_cma_ops;
rmem->priv = cma;

diff --git a/mm/cma.c b/mm/cma.c
index 0463ad2ce06b..ec671bd8f66e 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -55,6 +55,16 @@ const char *cma_get_name(const struct cma *cma)
return cma->name ? cma->name : "(undefined)";
}

+void __init cma_enable_dma_heap(struct cma *cma, bool enabled)
+{
+ cma->dma_heap = enabled;
+}
+
+bool cma_dma_heap_enabled(struct cma *cma)
+{
+ return !!cma->dma_heap;
+}
+
static unsigned long cma_bitmap_aligned_mask(const struct cma *cma,
unsigned int align_order)
{
@@ -157,6 +167,7 @@ static int __init cma_init_reserved_areas(void)
}
core_initcall(cma_init_reserved_areas);

+
/**
* cma_init_reserved_mem() - create custom contiguous area from reserved memory
* @base: Base address of the reserved area
diff --git a/mm/cma.h b/mm/cma.h
index 33c0b517733c..6fe2242c724f 100644
--- a/mm/cma.h
+++ b/mm/cma.h
@@ -13,6 +13,7 @@ struct cma {
spinlock_t mem_head_lock;
#endif
const char *name;
+ bool dma_heap;
};

extern struct cma cma_areas[MAX_CMA_AREAS];
--
2.17.1

2020-05-01 07:44:48

by John Stultz

[permalink] [raw]
Subject: [RFC][PATCH 4/4] example: dts: hi3660-hikey960: Add dts entries to test cma heap binding

Adds example test entry to create and expose a dummy "camera"
cma region via the dmabuf heaps interface

This isn't a patch I'm submitting to merge, but just an example
of how this functionality can be used, which I've used for
testing.

Cc: Rob Herring <[email protected]>
Cc: Sumit Semwal <[email protected]>
Cc: "Andrew F. Davis" <[email protected]>
Cc: Benjamin Gaignard <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Pratik Patel <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Chenbo Feng <[email protected]>
Cc: Alistair Strachan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
index c0a6aad9593f..5eef1a76d51a 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
@@ -81,6 +81,13 @@
reusable;
linux,cma-default;
};
+
+ cma_camera: cma-camera {
+ compatible = "shared-dma-pool";
+ reg = <0x0 0x24C00000 0x0 0x4000000>;
+ reusable;
+ linux,cma-heap;
+ };
};

[email protected] {
--
2.17.1

2020-05-01 07:45:22

by John Stultz

[permalink] [raw]
Subject: [RFC][PATCH 3/4] dma-buf: cma_heap: Extend logic to export CMA regions tagged with "linux,cma-heap"

This patch reworks the cma_heap initialization so that
we expose both the default CMA region and any CMA regions
tagged with "linux,cma-heap" in the device-tree.

Cc: Rob Herring <[email protected]>
Cc: Sumit Semwal <[email protected]>
Cc: "Andrew F. Davis" <[email protected]>
Cc: Benjamin Gaignard <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Pratik Patel <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Chenbo Feng <[email protected]>
Cc: Alistair Strachan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 626cf7fd033a..dd154e2db101 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
{
struct cma_heap *cma_heap;
struct dma_heap_export_info exp_info;
+ struct cma *default_cma = dev_get_cma_area(NULL);
+
+ /* We only add the default heap and explicitly tagged heaps */
+ if (cma != default_cma && !cma_dma_heap_enabled(cma))
+ return 0;

cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
if (!cma_heap)
@@ -162,16 +167,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
return 0;
}

-static int add_default_cma_heap(void)
+static int cma_heaps_init(void)
{
- struct cma *default_cma = dev_get_cma_area(NULL);
- int ret = 0;
-
- if (default_cma)
- ret = __add_cma_heap(default_cma, NULL);
-
- return ret;
+ cma_for_each_area(__add_cma_heap, NULL);
+ return 0;
}
-module_init(add_default_cma_heap);
+module_init(cma_heaps_init);
MODULE_DESCRIPTION("DMA-BUF CMA Heap");
MODULE_LICENSE("GPL v2");
--
2.17.1

2020-05-01 10:23:46

by Brian Starkey

[permalink] [raw]
Subject: Re: [RFC][PATCH 3/4] dma-buf: cma_heap: Extend logic to export CMA regions tagged with "linux,cma-heap"

Hi John,

On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
> This patch reworks the cma_heap initialization so that
> we expose both the default CMA region and any CMA regions
> tagged with "linux,cma-heap" in the device-tree.
>
> Cc: Rob Herring <[email protected]>
> Cc: Sumit Semwal <[email protected]>
> Cc: "Andrew F. Davis" <[email protected]>
> Cc: Benjamin Gaignard <[email protected]>
> Cc: Liam Mark <[email protected]>
> Cc: Pratik Patel <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: Brian Starkey <[email protected]>
> Cc: Chenbo Feng <[email protected]>
> Cc: Alistair Strachan <[email protected]>
> Cc: Sandeep Patil <[email protected]>
> Cc: Hridya Valsaraju <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> index 626cf7fd033a..dd154e2db101 100644
> --- a/drivers/dma-buf/heaps/cma_heap.c
> +++ b/drivers/dma-buf/heaps/cma_heap.c
> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
> {
> struct cma_heap *cma_heap;
> struct dma_heap_export_info exp_info;
> + struct cma *default_cma = dev_get_cma_area(NULL);
> +
> + /* We only add the default heap and explicitly tagged heaps */
> + if (cma != default_cma && !cma_dma_heap_enabled(cma))
> + return 0;

Thinking about the pl111 thread[1], I'm wondering if we should also
let drivers call this directly to expose their CMA pools, even if they
aren't tagged for dma-heaps in DT. But perhaps that's too close to
policy.

Cheers,
-Brian

[1] https://lists.freedesktop.org/archives/dri-devel/2020-April/264358.html

>
> cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
> if (!cma_heap)
> @@ -162,16 +167,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
> return 0;
> }
>
> -static int add_default_cma_heap(void)
> +static int cma_heaps_init(void)
> {
> - struct cma *default_cma = dev_get_cma_area(NULL);
> - int ret = 0;
> -
> - if (default_cma)
> - ret = __add_cma_heap(default_cma, NULL);
> -
> - return ret;
> + cma_for_each_area(__add_cma_heap, NULL);
> + return 0;
> }
> -module_init(add_default_cma_heap);
> +module_init(cma_heaps_init);
> MODULE_DESCRIPTION("DMA-BUF CMA Heap");
> MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>

2020-05-01 10:46:44

by Brian Starkey

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/4] devicetree: bindings: Add linux,cma-heap tag for reserved memory

Hi,

On Fri, May 01, 2020 at 07:39:46AM +0000, John Stultz wrote:
> This patch adds a linux,cma-heap property for CMA reserved memory
> regions, which will be used to allow the region to be exposed via
> the DMA-BUF Heaps interface
>
> Cc: Rob Herring <[email protected]>
> Cc: Sumit Semwal <[email protected]>
> Cc: "Andrew F. Davis" <[email protected]>
> Cc: Benjamin Gaignard <[email protected]>
> Cc: Liam Mark <[email protected]>
> Cc: Pratik Patel <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: Brian Starkey <[email protected]>
> Cc: Chenbo Feng <[email protected]>
> Cc: Alistair Strachan <[email protected]>
> Cc: Sandeep Patil <[email protected]>
> Cc: Hridya Valsaraju <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> .../devicetree/bindings/reserved-memory/reserved-memory.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> index bac4afa3b197..e97b6a4c3bc0 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> @@ -68,6 +68,9 @@ Linux implementation note:
> - If a "linux,cma-default" property is present, then Linux will use the
> region for the default pool of the contiguous memory allocator.
>
> +- If a "linux,cma-heap" property is present, then Linux will expose the
> + the CMA region via the DMA-BUF Heaps interface.
> +

Would it be useful or even possible to give some indication of what
the heap will end up being called? I'm afraid I don't remember what if
any conclusions came out of previous discussions on UAPI for heap
enumeration.

I suppose CMA names haven't been relevant to userspace before, but
they perhaps would be with this change.

Alternatively, leaving it effectively undefined doesn't tie us down,
and something like links in sysfs can be added as a richer API in the
future.

Cheers,
-Brian

> - If a "linux,dma-default" property is present, then Linux will use the
> region for the default pool of the consistent DMA allocator.
>
> --
> 2.17.1
>

2020-05-01 10:50:12

by Brian Starkey

[permalink] [raw]
Subject: Re: [RFC][PATCH 2/4] mm: cma: Add dma_heap flag to cma structure

On Fri, May 01, 2020 at 07:39:47AM +0000, John Stultz wrote:
> This patch adds a dma_heap flag on the cma structure,
> along with accessors to set and read the flag.
>
> This is then used to process and store the "linux,cma-heap"
> property documented in the previous patch.
>
> Cc: Rob Herring <[email protected]>
> Cc: Sumit Semwal <[email protected]>
> Cc: "Andrew F. Davis" <[email protected]>
> Cc: Benjamin Gaignard <[email protected]>
> Cc: Liam Mark <[email protected]>
> Cc: Pratik Patel <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: Brian Starkey <[email protected]>
> Cc: Chenbo Feng <[email protected]>
> Cc: Alistair Strachan <[email protected]>
> Cc: Sandeep Patil <[email protected]>
> Cc: Hridya Valsaraju <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> include/linux/cma.h | 3 +++
> kernel/dma/contiguous.c | 3 +++
> mm/cma.c | 11 +++++++++++
> mm/cma.h | 1 +
> 4 files changed, 18 insertions(+)
>
> diff --git a/include/linux/cma.h b/include/linux/cma.h
> index 6ff79fefd01f..d8b8e6ce221c 100644
> --- a/include/linux/cma.h
> +++ b/include/linux/cma.h
> @@ -25,6 +25,9 @@ extern phys_addr_t cma_get_base(const struct cma *cma);
> extern unsigned long cma_get_size(const struct cma *cma);
> extern const char *cma_get_name(const struct cma *cma);
>
> +extern void __init cma_enable_dma_heap(struct cma *cma, bool enabled);
> +extern bool cma_dma_heap_enabled(struct cma *cma);
> +
> extern int __init cma_declare_contiguous_nid(phys_addr_t base,
> phys_addr_t size, phys_addr_t limit,
> phys_addr_t alignment, unsigned int order_per_bit,
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index 8bc6f2d670f9..f667fd51daa2 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -303,6 +303,7 @@ static int __init rmem_cma_setup(struct reserved_mem *rmem)
> phys_addr_t mask = align - 1;
> unsigned long node = rmem->fdt_node;
> bool default_cma = of_get_flat_dt_prop(node, "linux,cma-default", NULL);
> + bool heap_exported = of_get_flat_dt_prop(node, "linux,cma-heap", NULL);
> struct cma *cma;
> int err;
>
> @@ -332,6 +333,8 @@ static int __init rmem_cma_setup(struct reserved_mem *rmem)
> if (default_cma)
> dma_contiguous_set_default(cma);
>
> + cma_enable_dma_heap(cma, heap_exported);
> +
> rmem->ops = &rmem_cma_ops;
> rmem->priv = cma;
>
> diff --git a/mm/cma.c b/mm/cma.c
> index 0463ad2ce06b..ec671bd8f66e 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -55,6 +55,16 @@ const char *cma_get_name(const struct cma *cma)
> return cma->name ? cma->name : "(undefined)";
> }
>
> +void __init cma_enable_dma_heap(struct cma *cma, bool enabled)
> +{
> + cma->dma_heap = enabled;
> +}
> +
> +bool cma_dma_heap_enabled(struct cma *cma)
> +{
> + return !!cma->dma_heap;

Stylistic thing, but I don't think the !! is really necessary. It's
already a bool anyway.

> +}
> +
> static unsigned long cma_bitmap_aligned_mask(const struct cma *cma,
> unsigned int align_order)
> {
> @@ -157,6 +167,7 @@ static int __init cma_init_reserved_areas(void)
> }
> core_initcall(cma_init_reserved_areas);
>
> +

nit: spurious newline

Cheers,
-Brian

> /**
> * cma_init_reserved_mem() - create custom contiguous area from reserved memory
> * @base: Base address of the reserved area
> diff --git a/mm/cma.h b/mm/cma.h
> index 33c0b517733c..6fe2242c724f 100644
> --- a/mm/cma.h
> +++ b/mm/cma.h
> @@ -13,6 +13,7 @@ struct cma {
> spinlock_t mem_head_lock;
> #endif
> const char *name;
> + bool dma_heap;
> };
>
> extern struct cma cma_areas[MAX_CMA_AREAS];
> --
> 2.17.1
>

2020-05-01 11:10:47

by Robin Murphy

[permalink] [raw]
Subject: Re: [RFC][PATCH 3/4] dma-buf: cma_heap: Extend logic to export CMA regions tagged with "linux,cma-heap"

On 2020-05-01 11:21 am, Brian Starkey wrote:
> Hi John,
>
> On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
>> This patch reworks the cma_heap initialization so that
>> we expose both the default CMA region and any CMA regions
>> tagged with "linux,cma-heap" in the device-tree.
>>
>> Cc: Rob Herring <[email protected]>
>> Cc: Sumit Semwal <[email protected]>
>> Cc: "Andrew F. Davis" <[email protected]>
>> Cc: Benjamin Gaignard <[email protected]>
>> Cc: Liam Mark <[email protected]>
>> Cc: Pratik Patel <[email protected]>
>> Cc: Laura Abbott <[email protected]>
>> Cc: Brian Starkey <[email protected]>
>> Cc: Chenbo Feng <[email protected]>
>> Cc: Alistair Strachan <[email protected]>
>> Cc: Sandeep Patil <[email protected]>
>> Cc: Hridya Valsaraju <[email protected]>
>> Cc: Christoph Hellwig <[email protected]>
>> Cc: Marek Szyprowski <[email protected]>
>> Cc: Robin Murphy <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: John Stultz <[email protected]>
>> ---
>> drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
>> index 626cf7fd033a..dd154e2db101 100644
>> --- a/drivers/dma-buf/heaps/cma_heap.c
>> +++ b/drivers/dma-buf/heaps/cma_heap.c
>> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
>> {
>> struct cma_heap *cma_heap;
>> struct dma_heap_export_info exp_info;
>> + struct cma *default_cma = dev_get_cma_area(NULL);
>> +
>> + /* We only add the default heap and explicitly tagged heaps */
>> + if (cma != default_cma && !cma_dma_heap_enabled(cma))
>> + return 0;
>
> Thinking about the pl111 thread[1], I'm wondering if we should also
> let drivers call this directly to expose their CMA pools, even if they
> aren't tagged for dma-heaps in DT. But perhaps that's too close to
> policy.

That sounds much like what my first thoughts were - apologies if I'm
wildly off-base here, but as far as I understand:

- Device drivers know whether they have their own "memory-region" or not.
- Device drivers already have to do *something* to participate in dma-buf.
- Device drivers know best how they make use of both the above.
- Therefore couldn't it be left to drivers to choose whether to register
their CMA regions as heaps, without having to mess with DT at all?

Robin.

>
> Cheers,
> -Brian
>
> [1] https://lists.freedesktop.org/archives/dri-devel/2020-April/264358.html
>
>>
>> cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
>> if (!cma_heap)
>> @@ -162,16 +167,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
>> return 0;
>> }
>>
>> -static int add_default_cma_heap(void)
>> +static int cma_heaps_init(void)
>> {
>> - struct cma *default_cma = dev_get_cma_area(NULL);
>> - int ret = 0;
>> -
>> - if (default_cma)
>> - ret = __add_cma_heap(default_cma, NULL);
>> -
>> - return ret;
>> + cma_for_each_area(__add_cma_heap, NULL);
>> + return 0;
>> }
>> -module_init(add_default_cma_heap);
>> +module_init(cma_heaps_init);
>> MODULE_DESCRIPTION("DMA-BUF CMA Heap");
>> MODULE_LICENSE("GPL v2");
>> --
>> 2.17.1
>>

2020-05-01 18:45:01

by John Stultz

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/4] devicetree: bindings: Add linux,cma-heap tag for reserved memory

On Fri, May 1, 2020 at 3:42 AM Brian Starkey <[email protected]> wrote:
>
> Hi,
>
> On Fri, May 01, 2020 at 07:39:46AM +0000, John Stultz wrote:
> > This patch adds a linux,cma-heap property for CMA reserved memory
> > regions, which will be used to allow the region to be exposed via
> > the DMA-BUF Heaps interface
> >
> > Cc: Rob Herring <[email protected]>
> > Cc: Sumit Semwal <[email protected]>
> > Cc: "Andrew F. Davis" <[email protected]>
> > Cc: Benjamin Gaignard <[email protected]>
> > Cc: Liam Mark <[email protected]>
> > Cc: Pratik Patel <[email protected]>
> > Cc: Laura Abbott <[email protected]>
> > Cc: Brian Starkey <[email protected]>
> > Cc: Chenbo Feng <[email protected]>
> > Cc: Alistair Strachan <[email protected]>
> > Cc: Sandeep Patil <[email protected]>
> > Cc: Hridya Valsaraju <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Marek Szyprowski <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > .../devicetree/bindings/reserved-memory/reserved-memory.txt | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > index bac4afa3b197..e97b6a4c3bc0 100644
> > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > @@ -68,6 +68,9 @@ Linux implementation note:
> > - If a "linux,cma-default" property is present, then Linux will use the
> > region for the default pool of the contiguous memory allocator.
> >
> > +- If a "linux,cma-heap" property is present, then Linux will expose the
> > + the CMA region via the DMA-BUF Heaps interface.
> > +
>
> Would it be useful or even possible to give some indication of what
> the heap will end up being called? I'm afraid I don't remember what if
> any conclusions came out of previous discussions on UAPI for heap
> enumeration.

So the name we expose is the CMA name itself. So with dt it will be
the name of the reserved memory node that the flag property is added
to.

> I suppose CMA names haven't been relevant to userspace before, but
> they perhaps would be with this change.
>
> Alternatively, leaving it effectively undefined doesn't tie us down,
> and something like links in sysfs can be added as a richer API in the
> future.

Hrm. Mind expanding on what you're thinking here?

thanks
-john

2020-05-01 18:46:24

by John Stultz

[permalink] [raw]
Subject: Re: [RFC][PATCH 2/4] mm: cma: Add dma_heap flag to cma structure

On Fri, May 1, 2020 at 3:48 AM Brian Starkey <[email protected]> wrote:
> On Fri, May 01, 2020 at 07:39:47AM +0000, John Stultz wrote:
> > +bool cma_dma_heap_enabled(struct cma *cma)
> > +{
> > + return !!cma->dma_heap;
>
> Stylistic thing, but I don't think the !! is really necessary. It's
> already a bool anyway.

Yea, I was using a bit field earlier and then moved to a bool for
simplicity and left this. I saw it as soon as I sent the patch, so
it's already fixed up.

> > @@ -157,6 +167,7 @@ static int __init cma_init_reserved_areas(void)
> > }
> > core_initcall(cma_init_reserved_areas);
> >
> > +
>
> nit: spurious newline

Yep. Same. The things you only see once the mail is sent. :)

Thanks so much for the review though!
-john

2020-05-01 19:04:14

by John Stultz

[permalink] [raw]
Subject: Re: [RFC][PATCH 3/4] dma-buf: cma_heap: Extend logic to export CMA regions tagged with "linux,cma-heap"

On Fri, May 1, 2020 at 4:08 AM Robin Murphy <[email protected]> wrote:
>
> On 2020-05-01 11:21 am, Brian Starkey wrote:
> > Hi John,
> >
> > On Fri, May 01, 2020 at 07:39:48AM +0000, John Stultz wrote:
> >> This patch reworks the cma_heap initialization so that
> >> we expose both the default CMA region and any CMA regions
> >> tagged with "linux,cma-heap" in the device-tree.
> >>
> >> Cc: Rob Herring <[email protected]>
> >> Cc: Sumit Semwal <[email protected]>
> >> Cc: "Andrew F. Davis" <[email protected]>
> >> Cc: Benjamin Gaignard <[email protected]>
> >> Cc: Liam Mark <[email protected]>
> >> Cc: Pratik Patel <[email protected]>
> >> Cc: Laura Abbott <[email protected]>
> >> Cc: Brian Starkey <[email protected]>
> >> Cc: Chenbo Feng <[email protected]>
> >> Cc: Alistair Strachan <[email protected]>
> >> Cc: Sandeep Patil <[email protected]>
> >> Cc: Hridya Valsaraju <[email protected]>
> >> Cc: Christoph Hellwig <[email protected]>
> >> Cc: Marek Szyprowski <[email protected]>
> >> Cc: Robin Murphy <[email protected]>
> >> Cc: Andrew Morton <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Signed-off-by: John Stultz <[email protected]>
> >> ---
> >> drivers/dma-buf/heaps/cma_heap.c | 18 +++++++++---------
> >> 1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> >> index 626cf7fd033a..dd154e2db101 100644
> >> --- a/drivers/dma-buf/heaps/cma_heap.c
> >> +++ b/drivers/dma-buf/heaps/cma_heap.c
> >> @@ -141,6 +141,11 @@ static int __add_cma_heap(struct cma *cma, void *data)
> >> {
> >> struct cma_heap *cma_heap;
> >> struct dma_heap_export_info exp_info;
> >> + struct cma *default_cma = dev_get_cma_area(NULL);
> >> +
> >> + /* We only add the default heap and explicitly tagged heaps */
> >> + if (cma != default_cma && !cma_dma_heap_enabled(cma))
> >> + return 0;
> >
> > Thinking about the pl111 thread[1], I'm wondering if we should also
> > let drivers call this directly to expose their CMA pools, even if they
> > aren't tagged for dma-heaps in DT. But perhaps that's too close to
> > policy.
>
> That sounds much like what my first thoughts were - apologies if I'm
> wildly off-base here, but as far as I understand:
>
> - Device drivers know whether they have their own "memory-region" or not.
> - Device drivers already have to do *something* to participate in dma-buf.
> - Device drivers know best how they make use of both the above.
> - Therefore couldn't it be left to drivers to choose whether to register
> their CMA regions as heaps, without having to mess with DT at all?

I guess I'm not opposed to this. But I guess I'd like to see some more
details? You're thinking the pl111 driver would add the
"memory-region" node itself?

Assuming that's the case, my only worry is what if that memory-region
node isn't a CMA area, but instead something like a carveout? Does the
driver need to parse enough of the dt to figure out where to register
the region as a heap?

thanks
-john

2020-05-04 09:03:57

by Brian Starkey

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/4] devicetree: bindings: Add linux,cma-heap tag for reserved memory

On Fri, May 01, 2020 at 11:40:16AM -0700, John Stultz wrote:
> On Fri, May 1, 2020 at 3:42 AM Brian Starkey <[email protected]> wrote:
> >
> > Hi,
> >
> > On Fri, May 01, 2020 at 07:39:46AM +0000, John Stultz wrote:
> > > This patch adds a linux,cma-heap property for CMA reserved memory
> > > regions, which will be used to allow the region to be exposed via
> > > the DMA-BUF Heaps interface
> > >
> > > Cc: Rob Herring <[email protected]>
> > > Cc: Sumit Semwal <[email protected]>
> > > Cc: "Andrew F. Davis" <[email protected]>
> > > Cc: Benjamin Gaignard <[email protected]>
> > > Cc: Liam Mark <[email protected]>
> > > Cc: Pratik Patel <[email protected]>
> > > Cc: Laura Abbott <[email protected]>
> > > Cc: Brian Starkey <[email protected]>
> > > Cc: Chenbo Feng <[email protected]>
> > > Cc: Alistair Strachan <[email protected]>
> > > Cc: Sandeep Patil <[email protected]>
> > > Cc: Hridya Valsaraju <[email protected]>
> > > Cc: Christoph Hellwig <[email protected]>
> > > Cc: Marek Szyprowski <[email protected]>
> > > Cc: Robin Murphy <[email protected]>
> > > Cc: Andrew Morton <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Signed-off-by: John Stultz <[email protected]>
> > > ---
> > > .../devicetree/bindings/reserved-memory/reserved-memory.txt | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > index bac4afa3b197..e97b6a4c3bc0 100644
> > > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > @@ -68,6 +68,9 @@ Linux implementation note:
> > > - If a "linux,cma-default" property is present, then Linux will use the
> > > region for the default pool of the contiguous memory allocator.
> > >
> > > +- If a "linux,cma-heap" property is present, then Linux will expose the
> > > + the CMA region via the DMA-BUF Heaps interface.
> > > +
> >
> > Would it be useful or even possible to give some indication of what
> > the heap will end up being called? I'm afraid I don't remember what if
> > any conclusions came out of previous discussions on UAPI for heap
> > enumeration.
>
> So the name we expose is the CMA name itself. So with dt it will be
> the name of the reserved memory node that the flag property is added
> to.
>

Yeah I'm just wondering if that's "stable" so we can say "the heap
will use the node name", or if saying that would cause us a headache
in the future.

> > I suppose CMA names haven't been relevant to userspace before, but
> > they perhaps would be with this change.
> >
> > Alternatively, leaving it effectively undefined doesn't tie us down,
> > and something like links in sysfs can be added as a richer API in the
> > future.
>
> Hrm. Mind expanding on what you're thinking here?

Super hand-wavy, something like:

/sys/devices/blah/[email protected]/cma_region is a symlink to
/sys/class/dma_heaps/heap_display

I think danvet had some thoughts in this vein.

Cheers,
-Brian

>
> thanks
> -john