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
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
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;
+}
+
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
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;
+ };
};
reboot-mode-syscon@32100000 {
--
2.17.1
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
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
>
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
>
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
>
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
>>
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
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
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
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/display@2f000000/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
On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> 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?
My thinking was more like there would already be a reserved-memory
node in DT for the chunk of memory, appropriately tagged so that it
gets added as a CMA region.
The device's node would have "memory-region=<&blah>;" and would use
of_reserved_mem_device_init() to link up dev->cma_area to the
corresponding cma region.
So far, that's all in-place already. The bit that's missing is
exposing that dev->cma_area to userspace as a dma_heap - so we could
just have "int cma_heap_add(struct cma *cma)" or "int
cma_heap_dev_add(struct device *dev)" or something exported for
drivers to expose their device-assigned cma region if they wanted to.
I don't think this runs into the lifetime problems of generalised
heaps-as-modules either, because the CMA region will never go away
even if the driver does.
Alongside that, I do think the completely DT-driven approach can be
useful too - because there may be regions which aren't associated with
any specific device driver, that we want exported as heaps.
Hope that makes sense,
-Brian
>
> thanks
> -john
On 5/4/20 4:50 AM, Brian Starkey wrote:
> 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.
The issue is going to be this causes the node name in DT to become a
kind of ABI. Right now until we have some userspace lib that enumerates
the heaps in a stable way programs will hard-code the full heap name,
which right now would look like:
char *heap = "/dev/dma_heap/dma_heap_mem@89000000";
Yuk.. we might want to look into exporting heap properties to make them
searchable based on something other than name here soon. Or this will be
a mess to cleanup in the future.
Andrew
>
>>> 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/display@2f000000/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
On Wed, May 6, 2020 at 9:04 AM Andrew F. Davis <[email protected]> wrote:
> On 5/4/20 4:50 AM, Brian Starkey wrote:
> > On Fri, May 01, 2020 at 11:40:16AM -0700, John Stultz wrote:
> >> 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.
>
>
> The issue is going to be this causes the node name in DT to become a
> kind of ABI. Right now until we have some userspace lib that enumerates
> the heaps in a stable way programs will hard-code the full heap name,
> which right now would look like:
>
> char *heap = "/dev/dma_heap/dma_heap_mem@89000000";
>
If that's what the device chose to export.
> Yuk.. we might want to look into exporting heap properties to make them
> searchable based on something other than name here soon. Or this will be
> a mess to cleanup in the future.
Eh. I don't see this as such an issue. On different systems we have
different device nodes. Some boards have more or fewer NICs, or
various partitions, etc. There has to be some device specific userland
config that determines which partitions are mounted where (this is my
"gralloc is fstab" thesis :)
I think with the heaps, qualities other than name are going to be
poorly specified or unenumerable, so any generic query interface is
going to fall down there (and be awful to use).
thanks
-john
On 5/6/20 12:30 PM, John Stultz wrote:
> On Wed, May 6, 2020 at 9:04 AM Andrew F. Davis <[email protected]> wrote:
>> On 5/4/20 4:50 AM, Brian Starkey wrote:
>>> On Fri, May 01, 2020 at 11:40:16AM -0700, John Stultz wrote:
>>>> 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.
>>
>>
>> The issue is going to be this causes the node name in DT to become a
>> kind of ABI. Right now until we have some userspace lib that enumerates
>> the heaps in a stable way programs will hard-code the full heap name,
>> which right now would look like:
>>
>> char *heap = "/dev/dma_heap/dma_heap_mem@89000000";
>>
>
> If that's what the device chose to export.
>
Well no "device" exported it, we did mostly automatically using only DT
information. When making a DT I don't want to be thinking about how
names will break userspace, for instance if node naming guidance is
updated do apps suddenly stop working? That worries me a bit.
>> Yuk.. we might want to look into exporting heap properties to make them
>> searchable based on something other than name here soon. Or this will be
>> a mess to cleanup in the future.
>
> Eh. I don't see this as such an issue. On different systems we have
> different device nodes. Some boards have more or fewer NICs, or
> various partitions, etc. There has to be some device specific userland
> config that determines which partitions are mounted where (this is my
> "gralloc is fstab" thesis :)
>
Oh I agree here, net interface names and /dev/<hd> names have a history
of changing, but those did both break a lot of apps. It could be argued
they were abusing the API by making assumptions about the names, but we
still have old scripts floating assuming "eth0" is going to just work..
So the sooner we get this fstab scheme in place and in practice, the
fewer apps in the wild will hard-code names.
> I think with the heaps, qualities other than name are going to be
> poorly specified or unenumerable, so any generic query interface is
> going to fall down there (and be awful to use).
>
Sure, so this "fstab" style config will have to be a mapping of names
(which we will have to make static per heap in kernel) to properties
that interest the current users of a system. For now I can only think of
cached/uncached, contiguous/sg, and secure/mappable. Then maybe a list
of devices that can consume buffers of that variety, should allow for
simple constraint matching. I'll need to think on this a bit more as the
use-cases show up..
Andrew
> thanks
> -john
>
On Wed, May 6, 2020 at 10:35 AM Andrew F. Davis <[email protected]> wrote:
> On 5/6/20 12:30 PM, John Stultz wrote:
> > On Wed, May 6, 2020 at 9:04 AM Andrew F. Davis <[email protected]> wrote:
> >> On 5/4/20 4:50 AM, Brian Starkey wrote:
> >>> On Fri, May 01, 2020 at 11:40:16AM -0700, John Stultz wrote:
> >>>> 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.
> >>
> >>
> >> The issue is going to be this causes the node name in DT to become a
> >> kind of ABI. Right now until we have some userspace lib that enumerates
> >> the heaps in a stable way programs will hard-code the full heap name,
> >> which right now would look like:
> >>
> >> char *heap = "/dev/dma_heap/dma_heap_mem@89000000";
> >>
> >
> > If that's what the device chose to export.
> >
>
>
> Well no "device" exported it, we did mostly automatically using only DT
Sorry. By "device" I meant the board/phone/system.
> information. When making a DT I don't want to be thinking about how
> names will break userspace, for instance if node naming guidance is
> updated do apps suddenly stop working? That worries me a bit.
So when folks change an existing board/system's DT, that can cause
userland breakage. Be it firmware paths, or when folks moved things
under an soc{ } node. But at the same time, just like each system has
a different partition layout, each system may have different heaps,
and its up to a system level config in userland to provide the policy
of what is used where.
> > Eh. I don't see this as such an issue. On different systems we have
> > different device nodes. Some boards have more or fewer NICs, or
> > various partitions, etc. There has to be some device specific userland
> > config that determines which partitions are mounted where (this is my
> > "gralloc is fstab" thesis :)
> >
>
> Oh I agree here, net interface names and /dev/<hd> names have a history
> of changing, but those did both break a lot of apps. It could be argued
> they were abusing the API by making assumptions about the names, but we
> still have old scripts floating assuming "eth0" is going to just work..
>
> So the sooner we get this fstab scheme in place and in practice, the
> fewer apps in the wild will hard-code names.
Gralloc already exists on Android devices, you ask to allocate for a
use case, and it picks the heap. It could be *much* simpler (rather
than per-device implementations, I'm hoping to get to a single
implementation with a fstab like config file), but it's already widely
used.
> > I think with the heaps, qualities other than name are going to be
> > poorly specified or unenumerable, so any generic query interface is
> > going to fall down there (and be awful to use).
>
> Sure, so this "fstab" style config will have to be a mapping of names
> (which we will have to make static per heap in kernel) to properties
I'm not sure I'm following this static per-heap requirement bit . Can
you clarify?
> that interest the current users of a system. For now I can only think of
> cached/uncached, contiguous/sg, and secure/mappable. Then maybe a list
> of devices that can consume buffers of that variety, should allow for
> simple constraint matching. I'll need to think on this a bit more as the
> use-cases show up..
There's a lot of other cases that are common on Android. One CMA heap
might be sized and reserved for camera usage, so it doesn't have to
compete with other CMA users to quickly get a bunch of frames. Where
as another CMA heap might be specified for a HWFB if that has to be
contiguous. Again, it's less about the specific attributes
(contiguous/secure/etc - though those are important considerations
when creating the mapping for it to work properly), and more of a
higher level "for this use case or this pipeline, we use this heap"
mapping.
Just like an application might store data to /home/ which maps to to a
specific partition configured on a specific system, instead of
looking for things like "what partition has the most space".
thanks
-john
On Mon, May 04, 2020 at 10:06:28AM +0100, Brian Starkey wrote:
> On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> > 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?
+1, but I'm biased toward any solution not using DT. :)
> > 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?
>
> My thinking was more like there would already be a reserved-memory
> node in DT for the chunk of memory, appropriately tagged so that it
> gets added as a CMA region.
>
> The device's node would have "memory-region=<&blah>;" and would use
> of_reserved_mem_device_init() to link up dev->cma_area to the
> corresponding cma region.
>
> So far, that's all in-place already. The bit that's missing is
> exposing that dev->cma_area to userspace as a dma_heap - so we could
> just have "int cma_heap_add(struct cma *cma)" or "int
> cma_heap_dev_add(struct device *dev)" or something exported for
> drivers to expose their device-assigned cma region if they wanted to.
>
> I don't think this runs into the lifetime problems of generalised
> heaps-as-modules either, because the CMA region will never go away
> even if the driver does.
>
> Alongside that, I do think the completely DT-driven approach can be
> useful too - because there may be regions which aren't associated with
> any specific device driver, that we want exported as heaps.
And they are associated with the hardware description rather than the
userspace environment?
Rob
Hi Rob,
On Tue, May 12, 2020 at 11:37:14AM -0500, Rob Herring wrote:
> On Mon, May 04, 2020 at 10:06:28AM +0100, Brian Starkey wrote:
> > On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> > > 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?
>
> +1, but I'm biased toward any solution not using DT. :)
>
> > > 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?
> >
> > My thinking was more like there would already be a reserved-memory
> > node in DT for the chunk of memory, appropriately tagged so that it
> > gets added as a CMA region.
> >
> > The device's node would have "memory-region=<&blah>;" and would use
> > of_reserved_mem_device_init() to link up dev->cma_area to the
> > corresponding cma region.
> >
> > So far, that's all in-place already. The bit that's missing is
> > exposing that dev->cma_area to userspace as a dma_heap - so we could
> > just have "int cma_heap_add(struct cma *cma)" or "int
> > cma_heap_dev_add(struct device *dev)" or something exported for
> > drivers to expose their device-assigned cma region if they wanted to.
> >
> > I don't think this runs into the lifetime problems of generalised
> > heaps-as-modules either, because the CMA region will never go away
> > even if the driver does.
> >
> > Alongside that, I do think the completely DT-driven approach can be
> > useful too - because there may be regions which aren't associated with
> > any specific device driver, that we want exported as heaps.
>
> And they are associated with the hardware description rather than the
> userspace environment?
I'm not sure how to answer that. We already have CMA regions being
created from device-tree, so we're only talking about explicitly
exposing those to userspace.
Are you thinking that userspace should be deciding whether they get
exposed or not? I don't know how userspace would discover them in
order to make that decision.
Thanks,
-Brian
>
> Rob
On Wed, May 13, 2020 at 5:44 AM Brian Starkey <[email protected]> wrote:
>
> Hi Rob,
>
> On Tue, May 12, 2020 at 11:37:14AM -0500, Rob Herring wrote:
> > On Mon, May 04, 2020 at 10:06:28AM +0100, Brian Starkey wrote:
> > > On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> > > > 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?
> >
> > +1, but I'm biased toward any solution not using DT. :)
> >
> > > > 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?
> > >
> > > My thinking was more like there would already be a reserved-memory
> > > node in DT for the chunk of memory, appropriately tagged so that it
> > > gets added as a CMA region.
> > >
> > > The device's node would have "memory-region=<&blah>;" and would use
> > > of_reserved_mem_device_init() to link up dev->cma_area to the
> > > corresponding cma region.
> > >
> > > So far, that's all in-place already. The bit that's missing is
> > > exposing that dev->cma_area to userspace as a dma_heap - so we could
> > > just have "int cma_heap_add(struct cma *cma)" or "int
> > > cma_heap_dev_add(struct device *dev)" or something exported for
> > > drivers to expose their device-assigned cma region if they wanted to.
> > >
> > > I don't think this runs into the lifetime problems of generalised
> > > heaps-as-modules either, because the CMA region will never go away
> > > even if the driver does.
> > >
> > > Alongside that, I do think the completely DT-driven approach can be
> > > useful too - because there may be regions which aren't associated with
> > > any specific device driver, that we want exported as heaps.
> >
> > And they are associated with the hardware description rather than the
> > userspace environment?
>
> I'm not sure how to answer that. We already have CMA regions being
> created from device-tree, so we're only talking about explicitly
> exposing those to userspace.
It's easier to argue that how much CMA memory a system/device needs is
h/w description as that's more a function of devices and frame sizes.
But exposing to userspace or not is an OS architecture decision. It's
not really any different than a kernel vs. userspace driver question.
What's exposed by UIO or spi-dev is purely a kernel thing.
> Are you thinking that userspace should be deciding whether they get
> exposed or not? I don't know how userspace would discover them in
> order to make that decision.
Or perhaps the kernel should be deciding. Expose to userspace what the
kernel doesn't need or drivers decide?
It's hard to argue against 1 new property. It's death by a 1000 cuts though.
Rob
On Thu, May 14, 2020 at 09:52:35AM -0500, Rob Herring wrote:
> On Wed, May 13, 2020 at 5:44 AM Brian Starkey <[email protected]> wrote:
> >
> > Hi Rob,
> >
> > On Tue, May 12, 2020 at 11:37:14AM -0500, Rob Herring wrote:
> > > On Mon, May 04, 2020 at 10:06:28AM +0100, Brian Starkey wrote:
> > > > On Fri, May 01, 2020 at 12:01:40PM -0700, John Stultz wrote:
> > > > > 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?
> > >
> > > +1, but I'm biased toward any solution not using DT. :)
> > >
> > > > > 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?
> > > >
> > > > My thinking was more like there would already be a reserved-memory
> > > > node in DT for the chunk of memory, appropriately tagged so that it
> > > > gets added as a CMA region.
> > > >
> > > > The device's node would have "memory-region=<&blah>;" and would use
> > > > of_reserved_mem_device_init() to link up dev->cma_area to the
> > > > corresponding cma region.
> > > >
> > > > So far, that's all in-place already. The bit that's missing is
> > > > exposing that dev->cma_area to userspace as a dma_heap - so we could
> > > > just have "int cma_heap_add(struct cma *cma)" or "int
> > > > cma_heap_dev_add(struct device *dev)" or something exported for
> > > > drivers to expose their device-assigned cma region if they wanted to.
> > > >
> > > > I don't think this runs into the lifetime problems of generalised
> > > > heaps-as-modules either, because the CMA region will never go away
> > > > even if the driver does.
> > > >
> > > > Alongside that, I do think the completely DT-driven approach can be
> > > > useful too - because there may be regions which aren't associated with
> > > > any specific device driver, that we want exported as heaps.
> > >
> > > And they are associated with the hardware description rather than the
> > > userspace environment?
> >
> > I'm not sure how to answer that. We already have CMA regions being
> > created from device-tree, so we're only talking about explicitly
> > exposing those to userspace.
>
> It's easier to argue that how much CMA memory a system/device needs is
> h/w description as that's more a function of devices and frame sizes.
> But exposing to userspace or not is an OS architecture decision. It's
> not really any different than a kernel vs. userspace driver question.
> What's exposed by UIO or spi-dev is purely a kernel thing.
>
> > Are you thinking that userspace should be deciding whether they get
> > exposed or not? I don't know how userspace would discover them in
> > order to make that decision.
>
> Or perhaps the kernel should be deciding. Expose to userspace what the
> kernel doesn't need or drivers decide?
If not via device-tree how would the kernel make that decision? For
regions which get "claimed" by a device/driver, obviously that driver
can make the decision, and I totally think we should provide a way for
them to expose it.
But for something like a shared pool amongst the media subsystem,
there may not be any specific single device/driver - and creating a
dummy driver with the sole purpose of exposing the heap doesn't seem
like an improvement over a simple "linux,cma-heap".
Cheers,
-Brian
>
> It's hard to argue against 1 new property. It's death by a 1000 cuts though.
>
> Rob