| Message ID | 771534be8dfa2a3bdc3876502752f518224b9298.1738228114.git.florent.tomasin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/5] dt-bindings: dma: Add CMA Heap bindings | expand |
Hi, On Thu, Jan 30, 2025 at 01:08:57PM +0000, Florent Tomasin wrote: > Introduce a CMA Heap dt-binding allowing custom > CMA heap registrations. > > * Note to the reviewers: > The patch was used for the development of the protected mode > feature in Panthor CSF kernel driver and is not initially thought > to land in the Linux kernel. It is mostly relevant if someone > wants to reproduce the environment of testing. Please, raise > interest if you think the patch has value in the Linux kernel. > > Signed-off-by: Florent Tomasin <florent.tomasin@arm.com> > --- > .../devicetree/bindings/dma/linux,cma.yml | 43 +++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/dma/linux,cma.yml > > diff --git a/Documentation/devicetree/bindings/dma/linux,cma.yml b/Documentation/devicetree/bindings/dma/linux,cma.yml > new file mode 100644 > index 000000000000..c532e016bbe5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/dma/linux,cma.yml > @@ -0,0 +1,43 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/dma/linux-cma.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Custom Linux CMA heap > + > +description: > + The custom Linux CMA heap device tree node allows registering > + of multiple CMA heaps. > + > + The CMA heap name will match the node name of the "memory-region". > + > +properties: > + compatible: > + enum: > + - linux,cma > + > + memory-region: > + maxItems: 1 > + description: | > + Phandle to the reserved memory node associated with the CMA Heap. > + The reserved memory node must follow this binding convention: > + - Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt > + > +examples: > + - | > + reserved-memory { > + #address-cells = <2>; > + #size-cells = <2>; > + > + custom_cma_heap: custom-cma-heap { > + compatible = "shared-dma-pool"; > + reg = <0x0 0x90600000 0x0 0x1000000>; > + reusable; > + }; > + }; > + > + device_cma_heap: device-cma-heap { > + compatible = "linux,cma"; > + memory-region = <&custom_cma_heap>; > + }; Isn't it redundant with the linux,cma-default shared-dma-pool property already? Maxime
On Thu, Jan 30, 2025 at 01:08:57PM +0000, Florent Tomasin wrote: > Introduce a CMA Heap dt-binding allowing custom > CMA heap registrations. > > * Note to the reviewers: > The patch was used for the development of the protected mode > feature in Panthor CSF kernel driver and is not initially thought > to land in the Linux kernel. It is mostly relevant if someone > wants to reproduce the environment of testing. Please, raise > interest if you think the patch has value in the Linux kernel. Why would panthor need CMA, it has an MMU. In any case, I agree with Maxime that this is redundant. Rob
On 30/01/2025 13:28, Maxime Ripard wrote: > Hi, > > On Thu, Jan 30, 2025 at 01:08:57PM +0000, Florent Tomasin wrote: >> Introduce a CMA Heap dt-binding allowing custom >> CMA heap registrations. >> >> * Note to the reviewers: >> The patch was used for the development of the protected mode >> feature in Panthor CSF kernel driver and is not initially thought >> to land in the Linux kernel. It is mostly relevant if someone >> wants to reproduce the environment of testing. Please, raise >> interest if you think the patch has value in the Linux kernel. >> >> Signed-off-by: Florent Tomasin <florent.tomasin@arm.com> >> --- >> .../devicetree/bindings/dma/linux,cma.yml | 43 +++++++++++++++++++ >> 1 file changed, 43 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/dma/linux,cma.yml >> >> diff --git a/Documentation/devicetree/bindings/dma/linux,cma.yml b/Documentation/devicetree/bindings/dma/linux,cma.yml >> new file mode 100644 >> index 000000000000..c532e016bbe5 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/dma/linux,cma.yml >> @@ -0,0 +1,43 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/dma/linux-cma.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Custom Linux CMA heap >> + >> +description: >> + The custom Linux CMA heap device tree node allows registering >> + of multiple CMA heaps. >> + >> + The CMA heap name will match the node name of the "memory-region". >> + >> +properties: >> + compatible: >> + enum: >> + - linux,cma >> + >> + memory-region: >> + maxItems: 1 >> + description: | >> + Phandle to the reserved memory node associated with the CMA Heap. >> + The reserved memory node must follow this binding convention: >> + - Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt >> + >> +examples: >> + - | >> + reserved-memory { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + custom_cma_heap: custom-cma-heap { >> + compatible = "shared-dma-pool"; >> + reg = <0x0 0x90600000 0x0 0x1000000>; >> + reusable; >> + }; >> + }; >> + >> + device_cma_heap: device-cma-heap { >> + compatible = "linux,cma"; >> + memory-region = <&custom_cma_heap>; >> + }; > > Isn't it redundant with the linux,cma-default shared-dma-pool property > already? > > Maxime Hi Maxime, Please correct me if my understanding is wrong, The existing properties: linux,cma-default and shared-dma-pool, do not allow the creations of multiple standalone CMA heaps, those will create a single CMA heap: `dma_contiguous_default_area`? Other CMA heaps will be bound to a driver. I introduced the "linux,cma" to allow creating multiple standalone CMA heaps, with the intention of validating the protected mode support on Mali CSG GPUs. It was included in the RFC in there are interests in this approach. Since the Panthor CSF kernel driver does not own or manage a heap, I needed a way to create a standalone heap. The idea here is for the kernel driver to be an importer. I relied on a patch series to retrieve the heap and allocate a DMA buffer from it: - dma_heap_find() - dma_heap_buffer_alloc() - dma_heap_put() Ref: https://lore.kernel.org/lkml/20230911023038.30649-1-yong.wu@mediatek.com/#t Since the protected/secure memory management is integration specific, I needed a generic way for Panthor to allocate from such heap. In some scenarios it might be a carved-out memory, in others a FW will reside in the system (TEE) and require a secure heap driver to allocate memory (e.g: a similar approach is followd by MTK). Such driver would implement the allocation and free logic. Florent
Hi Rob On 30/01/2025 23:20, Rob Herring wrote: > > Why would panthor need CMA, it has an MMU. > > In any case, I agree with Maxime that this is redundant. > This is correct, the GPU has an MMU. The reason I introduced this custom CMA DTB entry is to allow creation of a standalone DMA heap which can be retrieved by Panthor using the API exposed by: - https://lore.kernel.org/lkml/20230911023038.30649-1-yong.wu@mediatek.com/#t My understanding might be wrong, I am under the impression that current CMA driver only has `dma_contiguous_default_area` as standalone carved-out heap and we cannot have more than one. Please correct me if this is invalid. With the DMA Heap API I based the RFC on, Panthor kernel driver does not manage the protected heap itself, it relies on an exporter to do it. On some system the secure heap will communicate with a secure FW, on others it will be a carved-out memory with restricted access. This is integration specific. Panthor kernel driver will expect to import a DMA buffer obtained from a heap. For the development of the protected mode feature, I decided to modify the CMA driver to create a standalone DMA heap and allocate a DMA buffer from it. It helped me abstract the importing of a heap in Panthor kernel driver. Someone may use a different heap driver to reproduce the setup. * Additional information to help with the context: Mali CSF GPU requires protected memory at the device level which does not belong to a user space process in order to allow the FW to enter protected mode. There is a single FW per GPU instance and the FW is loaded a probe time. Regards, Florent
Hi Florent, Le lundi 03 février 2025 à 13:36 +0000, Florent Tomasin a écrit : > > On 30/01/2025 13:28, Maxime Ripard wrote: > > Hi, > > > > On Thu, Jan 30, 2025 at 01:08:57PM +0000, Florent Tomasin wrote: > > > Introduce a CMA Heap dt-binding allowing custom > > > CMA heap registrations. > > > > > > * Note to the reviewers: > > > The patch was used for the development of the protected mode Just to avoid divergence in nomenclature, and because this is not a new subject, perhaps you should also adhere to the name "restricted". Both Linaro and Mediatek have moved from "secure" to that name in their proposal. As you are the third proposing this (at least for the proposal that are CCed on linux-media), I would have expected in your cover letter a summary of how the other requirement have been blended in your proposal. regards, Nicolas > > > feature in Panthor CSF kernel driver and is not initially thought > > > to land in the Linux kernel. It is mostly relevant if someone > > > wants to reproduce the environment of testing. Please, raise > > > interest if you think the patch has value in the Linux kernel. > > > > > > Signed-off-by: Florent Tomasin <florent.tomasin@arm.com> > > > --- > > > .../devicetree/bindings/dma/linux,cma.yml | 43 +++++++++++++++++++ > > > 1 file changed, 43 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/dma/linux,cma.yml > > > > > > diff --git a/Documentation/devicetree/bindings/dma/linux,cma.yml b/Documentation/devicetree/bindings/dma/linux,cma.yml > > > new file mode 100644 > > > index 000000000000..c532e016bbe5 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/dma/linux,cma.yml > > > @@ -0,0 +1,43 @@ > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/dma/linux-cma.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Custom Linux CMA heap > > > + > > > +description: > > > + The custom Linux CMA heap device tree node allows registering > > > + of multiple CMA heaps. > > > + > > > + The CMA heap name will match the node name of the "memory-region". > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - linux,cma > > > + > > > + memory-region: > > > + maxItems: 1 > > > + description: | > > > + Phandle to the reserved memory node associated with the CMA Heap. > > > + The reserved memory node must follow this binding convention: > > > + - Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt > > > + > > > +examples: > > > + - | > > > + reserved-memory { > > > + #address-cells = <2>; > > > + #size-cells = <2>; > > > + > > > + custom_cma_heap: custom-cma-heap { > > > + compatible = "shared-dma-pool"; > > > + reg = <0x0 0x90600000 0x0 0x1000000>; > > > + reusable; > > > + }; > > > + }; > > > + > > > + device_cma_heap: device-cma-heap { > > > + compatible = "linux,cma"; > > > + memory-region = <&custom_cma_heap>; > > > + }; > > > > Isn't it redundant with the linux,cma-default shared-dma-pool property > > already? > > > > Maxime > > Hi Maxime, > > Please correct me if my understanding is wrong, > > The existing properties: linux,cma-default and shared-dma-pool, do not > allow the creations of multiple standalone CMA heaps, those will create > a single CMA heap: `dma_contiguous_default_area`? Other CMA heaps will > be bound to a driver. > > I introduced the "linux,cma" to allow creating multiple standalone CMA > heaps, with the intention of validating the protected mode support on > Mali CSG GPUs. It was included in the RFC in there are interests in > this approach. > > Since the Panthor CSF kernel driver does not own or manage a heap, > I needed a way to create a standalone heap. The idea here is for the > kernel driver to be an importer. I relied on a patch series to retrieve > the heap and allocate a DMA buffer from it: > - dma_heap_find() > - dma_heap_buffer_alloc() > - dma_heap_put() > > Ref: > https://lore.kernel.org/lkml/20230911023038.30649-1-yong.wu@mediatek.com/#t > > > Since the protected/secure memory management is integration specific, > I needed a generic way for Panthor to allocate from such heap. > > In some scenarios it might be a carved-out memory, in others a FW will > reside in the system (TEE) and require a secure heap driver to allocate > memory (e.g: a similar approach is followd by MTK). Such driver would > implement the allocation and free logic. > > Florent > > >
Hi Nicolas, On 04/02/2025 18:12, Nicolas Dufresne wrote: > Hi Florent, > > Le lundi 03 février 2025 à 13:36 +0000, Florent Tomasin a écrit : >> >> On 30/01/2025 13:28, Maxime Ripard wrote: >>> Hi, >>> >>> On Thu, Jan 30, 2025 at 01:08:57PM +0000, Florent Tomasin wrote: >>>> Introduce a CMA Heap dt-binding allowing custom >>>> CMA heap registrations. >>>> >>>> * Note to the reviewers: >>>> The patch was used for the development of the protected mode > > Just to avoid divergence in nomenclature, and because this is not a new subject, > perhaps you should also adhere to the name "restricted". Both Linaro and > Mediatek have moved from "secure" to that name in their proposal. As you are the > third proposing this (at least for the proposal that are CCed on linux-media), I > would have expected in your cover letter a summary of how the other requirement > have been blended in your proposal. Just to be sure I undertand your suggestion correctly, are you proposing to use "restricted mode" instead of "protected mode"? In the case of Panthor CSF driver, the term: "protected mode" refers to a Mali CSF GPU HW concept: - https://developer.arm.com/documentation/100964/1127/Fast-Models-components/Media-components/Mali-G71 If preferred and to avoid confusion, I can remove the reference to "protected mode" and "Panthor CSF driver" from the commit message to focus only on the CMA heap changes, which are more generic and can apply to any type of CMA memory. Note that the CMA patches were initially shared to help reproduce my environment of development, I can isolate them in a separate patch series and include a reference or "base-commit:" tag to it in the Panthor protected mode RFC, to help progress this review in another thread. It will avoid overlapping these two topics: - Multiple standalone CMA heaps support - Panthor protected mode handling Regards, Florent
On Wed, Feb 12, 2025 at 09:49:56AM +0000, Florent Tomasin wrote: > Note that the CMA patches were initially shared to help reproduce my > environment of development, I can isolate them in a separate patch > series and include a reference or "base-commit:" tag to it in the > Panthor protected mode RFC, to help progress this review in another > thread. It will avoid overlapping these two topics: > > - Multiple standalone CMA heaps support > - Panthor protected mode handling You keep insisting on using CMA here, but it's really not clear to me why you would need CMA in the first place. By CMA, do you mean the CMA allocator, and thus would provide buffers through the usual dma_alloc_* API, or would any allocator providing physically contiguous memory work? In the latter case, would something like this work: https://lore.kernel.org/all/20240515-dma-buf-ecc-heap-v1-1-54cbbd049511@kernel.org/ Maxime
On 12/02/2025 10:01, Maxime Ripard wrote: > On Wed, Feb 12, 2025 at 09:49:56AM +0000, Florent Tomasin wrote: >> Note that the CMA patches were initially shared to help reproduce my >> environment of development, I can isolate them in a separate patch >> series and include a reference or "base-commit:" tag to it in the >> Panthor protected mode RFC, to help progress this review in another >> thread. It will avoid overlapping these two topics: >> >> - Multiple standalone CMA heaps support >> - Panthor protected mode handling > > You keep insisting on using CMA here, but it's really not clear to me > why you would need CMA in the first place. > > By CMA, do you mean the CMA allocator, and thus would provide buffers > through the usual dma_alloc_* API, or would any allocator providing > physically contiguous memory work? You are correct only the CMA allocator is relevant. I needed a way to sub-allocate from a carved-out memory. > In the latter case, would something like this work: > https://lore.kernel.org/all/20240515-dma-buf-ecc-heap-v1-1-54cbbd049511@kernel.org/ Thanks for sharing this link, I was not aware previous work was done on this aspect. The new carveout heap introduced in the series could probably be a good alternative. I will play-around with it and share some updates. Appreciated, Florent
On Wed, 12 Feb 2025 11:01:11 +0100 Maxime Ripard <mripard@kernel.org> wrote: > On Wed, Feb 12, 2025 at 09:49:56AM +0000, Florent Tomasin wrote: > > Note that the CMA patches were initially shared to help reproduce my > > environment of development, I can isolate them in a separate patch > > series and include a reference or "base-commit:" tag to it in the > > Panthor protected mode RFC, to help progress this review in another > > thread. It will avoid overlapping these two topics: > > > > - Multiple standalone CMA heaps support > > - Panthor protected mode handling > > You keep insisting on using CMA here, but it's really not clear to me > why you would need CMA in the first place. CMA is certainly not the solution. As Florent said, it's just here to help people test the panthor protected-mode feature without having to pull Mediatek's protected heap implementation, which currently lives in some vendor tree and is thus quite painful to integrate to a vanilla kernel. I would suggest keeping those patches in a public tree we can point to, and dropping them from v2, to avoid the confusion. > > By CMA, do you mean the CMA allocator, and thus would provide buffers > through the usual dma_alloc_* API, or would any allocator providing > physically contiguous memory work? Panthor can work with the system heap too AFAICT (I've done my testing with the system heap, and it seems to work fine). It gets tricky when you want to allocate protected scanout buffers and import them in the KMS device though. But as said above, we shouldn't really bother exposing custom CMA heaps, because that's not what we want to use ultimately. What we'll want is some ATF implementation for protected memory, that we can rely on to implement a standard protected dma-heap implementation, I guess. > > In the latter case, would something like this work: > https://lore.kernel.org/all/20240515-dma-buf-ecc-heap-v1-1-54cbbd049511@kernel.org/ > > Maxime
On Wed, Feb 12, 2025 at 10:29:32AM +0000, Florent Tomasin wrote: > > > On 12/02/2025 10:01, Maxime Ripard wrote: > > On Wed, Feb 12, 2025 at 09:49:56AM +0000, Florent Tomasin wrote: > >> Note that the CMA patches were initially shared to help reproduce my > >> environment of development, I can isolate them in a separate patch > >> series and include a reference or "base-commit:" tag to it in the > >> Panthor protected mode RFC, to help progress this review in another > >> thread. It will avoid overlapping these two topics: > >> > >> - Multiple standalone CMA heaps support > >> - Panthor protected mode handling > > > > You keep insisting on using CMA here, but it's really not clear to me > > why you would need CMA in the first place. > > > > By CMA, do you mean the CMA allocator, and thus would provide buffers > > through the usual dma_alloc_* API, or would any allocator providing > > physically contiguous memory work? > > You are correct only the CMA allocator is relevant. I needed a way to > sub-allocate from a carved-out memory. I'm still confused, sorry. You're saying that you require CMA but... > > In the latter case, would something like this work: > > https://lore.kernel.org/all/20240515-dma-buf-ecc-heap-v1-1-54cbbd049511@kernel.org/ > > Thanks for sharing this link, I was not aware previous work was done > on this aspect. The new carveout heap introduced in the series could > probably be a good alternative. I will play-around with it and share > some updates. ... you seem to be ok with a driver that doesn't use it? Maxime
On 12/02/2025 10:49, Maxime Ripard wrote: > On Wed, Feb 12, 2025 at 10:29:32AM +0000, Florent Tomasin wrote: >> >> >> On 12/02/2025 10:01, Maxime Ripard wrote: >>> On Wed, Feb 12, 2025 at 09:49:56AM +0000, Florent Tomasin wrote: >>>> Note that the CMA patches were initially shared to help reproduce my >>>> environment of development, I can isolate them in a separate patch >>>> series and include a reference or "base-commit:" tag to it in the >>>> Panthor protected mode RFC, to help progress this review in another >>>> thread. It will avoid overlapping these two topics: >>>> >>>> - Multiple standalone CMA heaps support >>>> - Panthor protected mode handling >>> >>> You keep insisting on using CMA here, but it's really not clear to me >>> why you would need CMA in the first place. >>> >>> By CMA, do you mean the CMA allocator, and thus would provide buffers >>> through the usual dma_alloc_* API, or would any allocator providing >>> physically contiguous memory work? >> >> You are correct only the CMA allocator is relevant. I needed a way to >> sub-allocate from a carved-out memory. > > I'm still confused, sorry. You're saying that you require CMA but... Adding to Boris's comment, the objective here was to enable sub-allocation from a carved-out memory region. The CMA heap was used for convinience. It can be any other heap driver that allows allocating a protected buffer. >>> In the latter case, would something like this work: >>> https://lore.kernel.org/all/20240515-dma-buf-ecc-heap-v1-1-54cbbd049511@kernel.org/ >> >> Thanks for sharing this link, I was not aware previous work was done >> on this aspect. The new carveout heap introduced in the series could >> probably be a good alternative. I will play-around with it and share >> some updates. > > ... you seem to be ok with a driver that doesn't use it? I will confirm it once I have done some validation. From Panthor driver point of view, it does not matter if the we use CMA, or alternative heaps. We just need to be able to allocate from a protected heap. I used the CMA heap to simplify the developpment of the feature, it can be anything. I will extract the CMA changes from the V2 of the RFC to prevent confusion. Regards, Florent
diff --git a/Documentation/devicetree/bindings/dma/linux,cma.yml b/Documentation/devicetree/bindings/dma/linux,cma.yml new file mode 100644 index 000000000000..c532e016bbe5 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/linux,cma.yml @@ -0,0 +1,43 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/dma/linux-cma.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Custom Linux CMA heap + +description: + The custom Linux CMA heap device tree node allows registering + of multiple CMA heaps. + + The CMA heap name will match the node name of the "memory-region". + +properties: + compatible: + enum: + - linux,cma + + memory-region: + maxItems: 1 + description: | + Phandle to the reserved memory node associated with the CMA Heap. + The reserved memory node must follow this binding convention: + - Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt + +examples: + - | + reserved-memory { + #address-cells = <2>; + #size-cells = <2>; + + custom_cma_heap: custom-cma-heap { + compatible = "shared-dma-pool"; + reg = <0x0 0x90600000 0x0 0x1000000>; + reusable; + }; + }; + + device_cma_heap: device-cma-heap { + compatible = "linux,cma"; + memory-region = <&custom_cma_heap>; + };
Introduce a CMA Heap dt-binding allowing custom CMA heap registrations. * Note to the reviewers: The patch was used for the development of the protected mode feature in Panthor CSF kernel driver and is not initially thought to land in the Linux kernel. It is mostly relevant if someone wants to reproduce the environment of testing. Please, raise interest if you think the patch has value in the Linux kernel. Signed-off-by: Florent Tomasin <florent.tomasin@arm.com> --- .../devicetree/bindings/dma/linux,cma.yml | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/linux,cma.yml