diff mbox series

[4/4] dma-heap: Devicetree binding for chunk heap

Message ID 20201117181935.3613581-5-minchan@kernel.org
State Needs Review / ACK
Headers show
Series Chunk Heap Support on DMA-HEAP | expand

Checks

Context Check Description
robh/dt-meta-schema success
robh/checkpatch success

Commit Message

Minchan Kim Nov. 17, 2020, 6:19 p.m. UTC
From: Hyesoo Yu <hyesoo.yu@samsung.com>

Document devicetree binding for chunk heap on dma heap framework

Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 .../bindings/dma-buf/chunk_heap.yaml          | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml

Comments

John Stultz Nov. 18, 2020, 3 a.m. UTC | #1
On Tue, Nov 17, 2020 at 10:19 AM Minchan Kim <minchan@kernel.org> wrote:
>
> From: Hyesoo Yu <hyesoo.yu@samsung.com>
>
> Document devicetree binding for chunk heap on dma heap framework
>
> Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  .../bindings/dma-buf/chunk_heap.yaml          | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml
>
> diff --git a/Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml b/Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml
> new file mode 100644
> index 000000000000..f382bee02778
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma-buf/chunk_heap.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Device tree binding for chunk heap on DMA HEAP FRAMEWORK
> +
> +maintainers:
> +  - Sumit Semwal <sumit.semwal@linaro.org>
> +
> +description: |
> +  The chunk heap is backed by the Contiguous Memory Allocator (CMA) and
> +  allocates the buffers that are made up to a list of fixed size chunks
> +  taken from CMA. Chunk sizes are configurated when the heaps are created.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - dma_heap,chunk
> +
> +  memory-region:
> +    maxItems: 1
> +
> +  alignment:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - memory-region
> +  - alignment
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    reserved-memory {
> +        #address-cells = <2>;
> +        #size-cells = <1>;
> +
> +        chunk_memory: chunk_memory {
> +            compatible = "shared-dma-pool";
> +            reusable;
> +            size = <0x10000000>;
> +        };
> +    };
> +
> +    chunk_default_heap: chunk_default_heap {
> +        compatible = "dma_heap,chunk";
> +        memory-region = <&chunk_memory>;
> +        alignment = <0x10000>;
> +    };


So I suspect Rob will push back on this as he has for other dt
bindings related to ion/dmabuf heaps (I tried to push a similar
solution to exporting multiple CMA areas via dmabuf heaps).

The proposal he seemed to like best was having an in-kernel function
that a driver would call to initialize the heap (associated with the
CMA region the driver is interested in). Similar to Kunihiko Hayashi's
patch here:
  - https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunihiko@socionext.com/

The one sticking point for that patch (which I think is a good one),
is that we don't have any in-tree users, so it couldn't be merged yet.

A similar approach might be good here, but again we probably need to
have at least one in-tree user which could call such a registration
function.

thanks
-john
Hyesoo Yu Nov. 19, 2020, 1:14 a.m. UTC | #2
On Tue, Nov 17, 2020 at 07:00:54PM -0800, John Stultz wrote:
> On Tue, Nov 17, 2020 at 10:19 AM Minchan Kim <minchan@kernel.org> wrote:
> >
> > From: Hyesoo Yu <hyesoo.yu@samsung.com>
> >
> > Document devicetree binding for chunk heap on dma heap framework
> >
> > Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  .../bindings/dma-buf/chunk_heap.yaml          | 52 +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml b/Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml
> > new file mode 100644
> > index 000000000000..f382bee02778
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: https://protect2.fireeye.com/v1/url?k=9020a1f6-cfbb98fd-90212ab9-002590f5b904-5057bc6b174b6a8e&q=1&e=76ff8b54-517c-4389-81b9-fa1446ad08bf&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fdma-buf%2Fchunk_heap.yaml%23
> > +$schema: https://protect2.fireeye.com/v1/url?k=876fa02f-d8f49924-876e2b60-002590f5b904-e220c9cf0d714704&q=1&e=76ff8b54-517c-4389-81b9-fa1446ad08bf&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> > +
> > +title: Device tree binding for chunk heap on DMA HEAP FRAMEWORK
> > +
> > +maintainers:
> > +  - Sumit Semwal <sumit.semwal@linaro.org>
> > +
> > +description: |
> > +  The chunk heap is backed by the Contiguous Memory Allocator (CMA) and
> > +  allocates the buffers that are made up to a list of fixed size chunks
> > +  taken from CMA. Chunk sizes are configurated when the heaps are created.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - dma_heap,chunk
> > +
> > +  memory-region:
> > +    maxItems: 1
> > +
> > +  alignment:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - memory-region
> > +  - alignment
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    reserved-memory {
> > +        #address-cells = <2>;
> > +        #size-cells = <1>;
> > +
> > +        chunk_memory: chunk_memory {
> > +            compatible = "shared-dma-pool";
> > +            reusable;
> > +            size = <0x10000000>;
> > +        };
> > +    };
> > +
> > +    chunk_default_heap: chunk_default_heap {
> > +        compatible = "dma_heap,chunk";
> > +        memory-region = <&chunk_memory>;
> > +        alignment = <0x10000>;
> > +    };
> 
> 
> So I suspect Rob will push back on this as he has for other dt
> bindings related to ion/dmabuf heaps (I tried to push a similar
> solution to exporting multiple CMA areas via dmabuf heaps).
> 
> The proposal he seemed to like best was having an in-kernel function
> that a driver would call to initialize the heap (associated with the
> CMA region the driver is interested in). Similar to Kunihiko Hayashi's
> patch here:
>   - https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunihiko@socionext.com/
> 
> The one sticking point for that patch (which I think is a good one),
> is that we don't have any in-tree users, so it couldn't be merged yet.
> 
> A similar approach might be good here, but again we probably need to
> have at least one in-tree user which could call such a registration
> function.
> 
> thanks
> -john
>

Thanks for your review.

The chunk heap is not considered for device-specific reserved memory and specific driver.
It is similar to system heap, but it only collects high-order pages by using specific cma-area for performance.

It is strange that there is in-tree user who registers chunk heap.
(Wouldn't it be strange for some users to register the system heap?)

Is there a reason to use dma-heap framework to add cma-area for specific device ?

Even if some in-tree users register dma-heap with cma-area, the buffers could be allocated in user-land and these could be shared among other devices.
For exclusive access, I guess, the device don't need to register dma-heap for cma area.

Please let me know if I misunderstood what you said.

Thanks,
Regards.
John Stultz Nov. 19, 2020, 3:19 a.m. UTC | #3
On Wed, Nov 18, 2020 at 5:22 PM Hyesoo Yu <hyesoo.yu@samsung.com> wrote:
>
> On Tue, Nov 17, 2020 at 07:00:54PM -0800, John Stultz wrote:
> > So I suspect Rob will push back on this as he has for other dt
> > bindings related to ion/dmabuf heaps (I tried to push a similar
> > solution to exporting multiple CMA areas via dmabuf heaps).
> >
> > The proposal he seemed to like best was having an in-kernel function
> > that a driver would call to initialize the heap (associated with the
> > CMA region the driver is interested in). Similar to Kunihiko Hayashi's
> > patch here:
> >   - https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunihiko@socionext.com/
> >
> > The one sticking point for that patch (which I think is a good one),
> > is that we don't have any in-tree users, so it couldn't be merged yet.
> >
> > A similar approach might be good here, but again we probably need to
> > have at least one in-tree user which could call such a registration
> > function.
>
> Thanks for your review.
>
> The chunk heap is not considered for device-specific reserved memory and specific driver.
> It is similar to system heap, but it only collects high-order pages by using specific cma-area for performance.

So, yes I agree, the chunk heap isn't device specific. It's just that
the CMA regions usually are tied to devices.

The main objection to this style of solution has been due to the fact
that the DTS is supposed to describe the physical hardware (in an OS
agnostic way), rather than define configuration info for Linux
software drivers.

Obviously this can be quibbled about, as the normal way of tying
devices to CMA has some assumptions of what the driver will use that
region for, rather than somehow representing a physical tie between a
memory reservation and a device. Nonetheless, Rob has been hesitant to
take any sort of ION/DmaBuf Heap DT devices, and has been more
interested in some device having the memory reservation reference and
the driver for that doing the registration.

> It is strange that there is in-tree user who registers chunk heap.
> (Wouldn't it be strange for some users to register the system heap?)

Well, as there's no reservation/configuration needed, the system heap
can register itself.

The CMA heap currently only registers the default CMA heap, as we
didn't want to expose all CMA regions and there's otherwise no way to
pick and choose.

> Is there a reason to use dma-heap framework to add cma-area for specific device ?
>
> Even if some in-tree users register dma-heap with cma-area, the buffers could be allocated in user-land and these could be shared among other devices.
> For exclusive access, I guess, the device don't need to register dma-heap for cma area.
>

It's not really about exclusive access. More just that if you want to
bind a memory reservation/region (cma or otherwise), at least for DTS,
it needs to bind with some device in DT.

Then the device driver can register that region with a heap driver.
This avoids adding new Linux-specific software bindings to DT. It
becomes a driver implementation detail instead. The primary user of
the heap type would probably be a practical pick (ie the display or
isp driver).

The other potential solution Rob has suggested is that we create some
tag for the memory reservation (ie: like we do with cma: "reusable"),
which can be used to register the region to a heap. But this has the
problem that each tag has to be well defined and map to a known heap.

thanks
-john
Hyesoo Yu Nov. 19, 2020, 6:30 a.m. UTC | #4
On Wed, Nov 18, 2020 at 07:19:07PM -0800, John Stultz wrote:
> On Wed, Nov 18, 2020 at 5:22 PM Hyesoo Yu <hyesoo.yu@samsung.com> wrote:
> >
> > On Tue, Nov 17, 2020 at 07:00:54PM -0800, John Stultz wrote:
> > > So I suspect Rob will push back on this as he has for other dt
> > > bindings related to ion/dmabuf heaps (I tried to push a similar
> > > solution to exporting multiple CMA areas via dmabuf heaps).
> > >
> > > The proposal he seemed to like best was having an in-kernel function
> > > that a driver would call to initialize the heap (associated with the
> > > CMA region the driver is interested in). Similar to Kunihiko Hayashi's
> > > patch here:
> > >   - https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunihiko@socionext.com/
> > >
> > > The one sticking point for that patch (which I think is a good one),
> > > is that we don't have any in-tree users, so it couldn't be merged yet.
> > >
> > > A similar approach might be good here, but again we probably need to
> > > have at least one in-tree user which could call such a registration
> > > function.
> >
> > Thanks for your review.
> >
> > The chunk heap is not considered for device-specific reserved memory and specific driver.
> > It is similar to system heap, but it only collects high-order pages by using specific cma-area for performance.
> 
> So, yes I agree, the chunk heap isn't device specific. It's just that
> the CMA regions usually are tied to devices.
> 
> The main objection to this style of solution has been due to the fact
> that the DTS is supposed to describe the physical hardware (in an OS
> agnostic way), rather than define configuration info for Linux
> software drivers.
> 
> Obviously this can be quibbled about, as the normal way of tying
> devices to CMA has some assumptions of what the driver will use that
> region for, rather than somehow representing a physical tie between a
> memory reservation and a device. Nonetheless, Rob has been hesitant to
> take any sort of ION/DmaBuf Heap DT devices, and has been more
> interested in some device having the memory reservation reference and
> the driver for that doing the registration.
> 
> > It is strange that there is in-tree user who registers chunk heap.
> > (Wouldn't it be strange for some users to register the system heap?)
> 
> Well, as there's no reservation/configuration needed, the system heap
> can register itself.
> 
> The CMA heap currently only registers the default CMA heap, as we
> didn't want to expose all CMA regions and there's otherwise no way to
> pick and choose.
> 
> > Is there a reason to use dma-heap framework to add cma-area for specific device ?
> >
> > Even if some in-tree users register dma-heap with cma-area, the buffers could be allocated in user-land and these could be shared among other devices.
> > For exclusive access, I guess, the device don't need to register dma-heap for cma area.
> >
> 
> It's not really about exclusive access. More just that if you want to
> bind a memory reservation/region (cma or otherwise), at least for DTS,
> it needs to bind with some device in DT.
> 
> Then the device driver can register that region with a heap driver.
> This avoids adding new Linux-specific software bindings to DT. It
> becomes a driver implementation detail instead. The primary user of
> the heap type would probably be a practical pick (ie the display or
> isp driver).
> 
> The other potential solution Rob has suggested is that we create some
> tag for the memory reservation (ie: like we do with cma: "reusable"),
> which can be used to register the region to a heap. But this has the
> problem that each tag has to be well defined and map to a known heap.
> 
> thanks
> -john
>

Thanks for the detailed reply.

I understood what you mean exactly.
I agree with your opinion that avoids software bindings to DT.

The way to register the heap by specific device driver, makes dependency
between heap and some device drivers that we pick practically.
If that device driver changed or removed whenever H/W changed,
the chunk heap is affected regardless of our intentions.

As you said, the other solution that add tags need to be well defined.
I guess, that will be a long-term solution.

First of all, we just want to register chunk heap to allocate high-order pages.
I'm going to change to a simple solution that uses default cma like cma heap, not using DT.

Thanks.
Regards.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml b/Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml
new file mode 100644
index 000000000000..f382bee02778
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml
@@ -0,0 +1,52 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma-buf/chunk_heap.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Device tree binding for chunk heap on DMA HEAP FRAMEWORK
+
+maintainers:
+  - Sumit Semwal <sumit.semwal@linaro.org>
+
+description: |
+  The chunk heap is backed by the Contiguous Memory Allocator (CMA) and
+  allocates the buffers that are made up to a list of fixed size chunks
+  taken from CMA. Chunk sizes are configurated when the heaps are created.
+
+properties:
+  compatible:
+    enum:
+      - dma_heap,chunk
+
+  memory-region:
+    maxItems: 1
+
+  alignment:
+    maxItems: 1
+
+required:
+  - compatible
+  - memory-region
+  - alignment
+
+additionalProperties: false
+
+examples:
+  - |
+    reserved-memory {
+        #address-cells = <2>;
+        #size-cells = <1>;
+
+        chunk_memory: chunk_memory {
+            compatible = "shared-dma-pool";
+            reusable;
+            size = <0x10000000>;
+        };
+    };
+
+    chunk_default_heap: chunk_default_heap {
+        compatible = "dma_heap,chunk";
+        memory-region = <&chunk_memory>;
+        alignment = <0x10000>;
+    };