[RFC][PATCH 0/3] Support non-default CMA regions to the dmabuf heaps interface

[RFC][PATCH 0/3] Support non-default CMA regions to the dmabuf heaps interface

[John Stultz]

Now that the dmabuf heaps core code has been queued, I wanted to
submit for initial review some of the changes I have pending.

In previous versions, the dmabuf CMA heap added all CMA areas to
the dmabuf heaps interface. However, Andrew noted this may not
be desirable, so I've come up with a DT binding and code to
allow specified CMA regions to be added to the dmabuf heaps
interface.

This allows additional CMA regions for things like cameras, etc
to be allocated from separately from the default region.

Review and feedback would be greatly appreciated!

thanks
-john

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Pratik Patel <pratikp@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Chenbo Feng <fengc@google.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: Sandeep Patil <sspatil@google.com>
Cc: Hridya Valsaraju <hridya@google.com>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org

John Stultz (3):
  dt-bindings: dma-buf: heaps: Describe CMA regions to be added to
    dmabuf heaps interface.
  dma-buf: heaps: Allow adding specified non-default CMA heaps
  example: dts: hi3660-hikey960: Add dts entries to test cma heap
    binding

 .../bindings/dma/dmabuf-heap-cma.txt          | 31 +++++++++++++++
 .../boot/dts/hisilicon/hi3660-hikey960.dts    | 13 ++++++-
 drivers/dma-buf/heaps/cma_heap.c              | 38 +++++++++++++++++++
 3 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/dma/dmabuf-heap-cma.txt

-- 
2.17.1

[RFC][PATCH 1/3] dt-bindings: dma-buf: heaps: Describe CMA regions to be added to dmabuf heaps interface.

[John Stultz]

This binding specifies which CMA regions should be added to the
dmabuf heaps interface.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Pratik Patel <pratikp@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Chenbo Feng <fengc@google.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: Sandeep Patil <sspatil@google.com>
Cc: Hridya Valsaraju <hridya@google.com>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 .../bindings/dma/dmabuf-heap-cma.txt          | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/dmabuf-heap-cma.txt

diff --git a/Documentation/devicetree/bindings/dma/dmabuf-heap-cma.txt b/Documentation/devicetree/bindings/dma/dmabuf-heap-cma.txt
new file mode 100644
index 000000000000..bde7b1f1c269
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/dmabuf-heap-cma.txt
@@ -0,0 +1,31 @@
+* DMA-BUF CMA Heap
+
+Specifies a CMA region that should be added to the dma-buf heaps
+interface.
+
+Required properties:
+- compatible: Must be "dmabuf-heap-cma"
+- memory-region: phandle to a CMA reserved memory node
+
+Example:
+This example has a camera CMA node in reserved memory, which is then
+referenced by the dmabuf-heap-cma node.
+
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		...
+		cma_camera: cma-camera {
+			compatible = "shared-dma-pool";
+			reg = <0x0 0x24C00000 0x0 0x4000000>;
+			reusable;
+		};
+		...
+	};
+
+       cma_heap {
+               compatible = "dmabuf-heap-cma";
+               memory-region = <&cma_camera>;
+        };
-- 
2.17.1

Re: [RFC][PATCH 1/3] dt-bindings: dma-buf: heaps: Describe CMA regions to be added to dmabuf heaps interface.

[Rob Herring]

On Fri, Oct 25, 2019 at 5:51 PM John Stultz <john.stultz@linaro.org> wrote:
>
> This binding specifies which CMA regions should be added to the
> dmabuf heaps interface.

Is this an ION DT binding in disguise? I thought I killed that. ;)

>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Pratik Patel <pratikp@codeaurora.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Chenbo Feng <fengc@google.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Cc: Sandeep Patil <sspatil@google.com>
> Cc: Hridya Valsaraju <hridya@google.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  .../bindings/dma/dmabuf-heap-cma.txt          | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/dmabuf-heap-cma.txt
>
> diff --git a/Documentation/devicetree/bindings/dma/dmabuf-heap-cma.txt b/Documentation/devicetree/bindings/dma/dmabuf-heap-cma.txt
> new file mode 100644
> index 000000000000..bde7b1f1c269
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/dmabuf-heap-cma.txt
> @@ -0,0 +1,31 @@
> +* DMA-BUF CMA Heap
> +
> +Specifies a CMA region that should be added to the dma-buf heaps
> +interface.
> +
> +Required properties:
> +- compatible: Must be "dmabuf-heap-cma"
> +- memory-region: phandle to a CMA reserved memory node
> +
> +Example:
> +This example has a camera CMA node in reserved memory, which is then
> +referenced by the dmabuf-heap-cma node.
> +
> +
> +       reserved-memory {
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;
> +               ...
> +               cma_camera: cma-camera {
> +                       compatible = "shared-dma-pool";
> +                       reg = <0x0 0x24C00000 0x0 0x4000000>;
> +                       reusable;
> +               };
> +               ...
> +       };
> +
> +       cma_heap {
> +               compatible = "dmabuf-heap-cma";
> +               memory-region = <&cma_camera>;

Why the indirection here? Can't you just add a flag property to
reserved-memory nodes like we do to flag CMA nodes?

As I suspected, it's because in patch 2 you're just abusing DT to
instantiate platform devices. We already support binding drivers to
reserved-memory nodes directly.

Rob

Re: [RFC][PATCH 1/3] dt-bindings: dma-buf: heaps: Describe CMA regions to be added to dmabuf heaps interface.

[John Stultz]

On Fri, Oct 25, 2019 at 4:32 PM Rob Herring <rob.e.herring@gmail.com> wrote:
>
> On Fri, Oct 25, 2019 at 5:51 PM John Stultz <john.stultz@linaro.org> wrote:
> >
> > This binding specifies which CMA regions should be added to the
> > dmabuf heaps interface.
>
> Is this an ION DT binding in disguise? I thought I killed that. ;)

Maybe? I may not have been paying attention back then.  :)

> > +Example:
> > +This example has a camera CMA node in reserved memory, which is then
> > +referenced by the dmabuf-heap-cma node.
> > +
> > +
> > +       reserved-memory {
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               ranges;
> > +               ...
> > +               cma_camera: cma-camera {
> > +                       compatible = "shared-dma-pool";
> > +                       reg = <0x0 0x24C00000 0x0 0x4000000>;
> > +                       reusable;
> > +               };
> > +               ...
> > +       };
> > +
> > +       cma_heap {
> > +               compatible = "dmabuf-heap-cma";
> > +               memory-region = <&cma_camera>;
>
> Why the indirection here? Can't you just add a flag property to
> reserved-memory nodes like we do to flag CMA nodes?

Happy to try. Do you mean like with the "reuasable" tag?  Or more like
the "linux,cma-default" tag?

Do you have a preference for the flag name here?

> As I suspected, it's because in patch 2 you're just abusing DT to
> instantiate platform devices. We already support binding drivers to
> reserved-memory nodes directly.

Sorry, one of those "when all you know how to do is hammer, everything
looks like a nail" issues.
Is there a specific example for binding drivers to reserved-memory
nodes I can try to follow?

Appreciate the review and feedback!

thanks
-john

Re: [RFC][PATCH 1/3] dt-bindings: dma-buf: heaps: Describe CMA regions to be added to dmabuf heaps interface.

[Rob Herring]

On Fri, Oct 25, 2019 at 04:55:35PM -0700, John Stultz wrote:
> On Fri, Oct 25, 2019 at 4:32 PM Rob Herring <rob.e.herring@gmail.com> wrote:
> >
> > On Fri, Oct 25, 2019 at 5:51 PM John Stultz <john.stultz@linaro.org> wrote:
> > >
> > > This binding specifies which CMA regions should be added to the
> > > dmabuf heaps interface.
> >
> > Is this an ION DT binding in disguise? I thought I killed that. ;)
> 
> Maybe? I may not have been paying attention back then.  :)
> 
> > > +Example:
> > > +This example has a camera CMA node in reserved memory, which is then
> > > +referenced by the dmabuf-heap-cma node.
> > > +
> > > +
> > > +       reserved-memory {
> > > +               #address-cells = <2>;
> > > +               #size-cells = <2>;
> > > +               ranges;
> > > +               ...
> > > +               cma_camera: cma-camera {
> > > +                       compatible = "shared-dma-pool";
> > > +                       reg = <0x0 0x24C00000 0x0 0x4000000>;
> > > +                       reusable;
> > > +               };
> > > +               ...
> > > +       };
> > > +
> > > +       cma_heap {
> > > +               compatible = "dmabuf-heap-cma";
> > > +               memory-region = <&cma_camera>;
> >
> > Why the indirection here? Can't you just add a flag property to
> > reserved-memory nodes like we do to flag CMA nodes?
> 
> Happy to try. Do you mean like with the "reuasable" tag?  Or more like
> the "linux,cma-default" tag?

Probably like "linux,cma-default" as it is a hint for who to manage it 
rather than a characteristic of the region.
 
> Do you have a preference for the flag name here?

Not really.


> > As I suspected, it's because in patch 2 you're just abusing DT to
> > instantiate platform devices. We already support binding drivers to
> > reserved-memory nodes directly.
> 
> Sorry, one of those "when all you know how to do is hammer, everything
> looks like a nail" issues.
> Is there a specific example for binding drivers to reserved-memory
> nodes I can try to follow?

ramoops and I think there's a QCom driver.

Rob

[RFC][PATCH 2/3] dma-buf: heaps: Allow adding specified non-default CMA heaps

[John Stultz]

In earlier versions of the dmabuf CMA heap, we added all CMA
areas as CMA heaps. Andrew noted this might not be desired,
and so we changed the code to only add the default CMA area.

This patch extends the earlier effort so that devices can
specifiy which CMA areas they want to add as dmabuf heaps via
DT, and we'll only add those.

This allows multiple CMA areas to be exported via the dmabuf
heaps interface.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Pratik Patel <pratikp@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Chenbo Feng <fengc@google.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: Sandeep Patil <sspatil@google.com>
Cc: Hridya Valsaraju <hridya@google.com>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/dma-buf/heaps/cma_heap.c | 38 ++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 064926b5d735..0d5231a1e561 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -15,6 +15,9 @@
 #include <linux/errno.h>
 #include <linux/highmem.h>
 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/scatterlist.h>
 #include <linux/sched/signal.h>
@@ -174,5 +177,40 @@ static int add_default_cma_heap(void)
 	return ret;
 }
 module_init(add_default_cma_heap);
+
+static int cma_heaps_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct cma *cma_area;
+	int ret;
+
+	ret = of_reserved_mem_device_init_by_idx(&pdev->dev, np, 0);
+	if (ret) {
+		pr_err("Error %s(): of_reserved_mem_device_init_by_idx failed!\n", __func__);
+		return ret;
+	}
+
+	cma_area = dev_get_cma_area(&pdev->dev);
+	if (cma_area)
+		ret = __add_cma_heap(cma_area, NULL);
+
+	return ret;
+}
+
+static const struct of_device_id cma_heap_dt_ids[] = {
+	{ .compatible = "dmabuf-heap-cma" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cma_heap_dt_ids);
+
+static struct platform_driver cma_heaps_driver = {
+	.driver	= {
+		.name		= "CMA Heaps",
+		.of_match_table	= cma_heap_dt_ids,
+	},
+	.probe	= cma_heaps_probe,
+};
+
+module_platform_driver(cma_heaps_driver);
 MODULE_DESCRIPTION("DMA-BUF CMA Heap");
 MODULE_LICENSE("GPL v2");
-- 
2.17.1

[RFC][PATCH 3/3] example: dts: hi3660-hikey960: Add dts entries to test cma heap binding

[John Stultz]

Adds example test entry to create and expose a 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 <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Pratik Patel <pratikp@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Chenbo Feng <fengc@google.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: Sandeep Patil <sspatil@google.com>
Cc: Hridya Valsaraju <hridya@google.com>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
index 190ac29b9e2f..783c5b2abd2d 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
@@ -75,12 +75,23 @@
 		};
 
 		/* global autoconfigured region for contiguous allocations */
-		linux,cma {
+		cma_default: linux,cma {
 			compatible = "shared-dma-pool";
 			reg = <0x0 0x20C00000 0x0 0x4000000>;
 			reusable;
 			linux,cma-default;
 		};
+
+		cma_camera: cma-camera {
+			compatible = "shared-dma-pool";
+			reg = <0x0 0x24C00000 0x0 0x4000000>;
+			reusable;
+		};
+	};
+
+	cma_heap {
+		compatible = "dmabuf-heap-cma";
+		memory-region = <&cma_camera>;
 	};
 
 	reboot-mode-syscon@32100000 {
-- 
2.17.1