[PATCH 0/3] Add global CMA reserve area

[PATCH 0/3] Add global CMA reserve area

[Devarsh Thakkar]

Add global CMA reserve area for AM62x, AM62A and AM62P SoCs.
These SoCs do not have MMU and hence require contiguous memory pool to
support various multimedia use-cases.

Brandon Brnich (1):
  arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA

Devarsh Thakkar (2):
  arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA
  arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA

 arch/arm64/boot/dts/ti/k3-am62a7-sk.dts        | 9 +++++++++
 arch/arm64/boot/dts/ti/k3-am62p5-sk.dts        | 7 +++++++
 arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 8 ++++++++
 3 files changed, 24 insertions(+)

-- 
2.39.1

[PATCH 1/3] arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA

[Devarsh Thakkar]

Reserve 128MiB of global CMA which is also marked as re-usable
so that OS can also use the same if peripheral drivers are not using the
same.

AM62x supports multimedia components such as GPU, dual Display and Camera.
Assuming the worst-case scenario where all 3 are run in parallel below
is the calculation :

1) OV5640 camera sensor supports 1920x1080 resolution
-> 1920 width x 1080 height x 2 bytesperpixel x 8 buffers
   (default in yavta) : 32MiB

2) 1920x1200 Microtips LVDS panel supported
-> 1920 width x 1080 height x 4 bytesperpixel x 2 buffers :
   16 MiB

3) 1920x1080 HDMI display supported
-> 1920 width x 1080 height x 4 bytesperpixel x 2 buffers :
   15.82 MiB which is ~16 MiB

4) IMG GPU shares with display allocated buffers while rendering
   but in case some dedicated operation viz color conversion,
   keeping same window of ~16 MiB for GPU too.

Total is 80 MiB and adding 32 MiB for other peripherals and extra
16 MiB to keep as buffer for fragmentation thus rounding total to 128
MiB.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
index f4948b937627..52231bfe60fe 100644
--- a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
@@ -48,6 +48,14 @@ ramoops@9ca00000 {
 			pmsg-size = <0x8000>;
 		};
 
+		/* global cma region */
+		linux,cma {
+			compatible = "shared-dma-pool";
+			reusable;
+			size = <0x00 0x8000000>;
+			linux,cma-default;
+		};
+
 		secure_tfa_ddr: tfa@9e780000 {
 			reg = <0x00 0x9e780000 0x00 0x80000>;
 			alignment = <0x1000>;
-- 
2.39.1

[PATCH 2/3] arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA

[Devarsh Thakkar]

Reserve 576MiB of CMA as global CMA pool starting after initial 1GiB of
DDR.

AM62ax has different multimedia components such as Camera, Display, H.264
VPU and JPEG Encoder which use CMA for buffer allocations.

The 12x 720x480 realtime VPU decode use-case requires 544MiB of CMA,
additional 32MiB is kept as buffer in case some other peripheral also
require it while VPU is running.

The reason to choose latter 1GiB is to not overlap with existing memory map
which is utilizing initial 1GiB for remoteproc firmwares as shared here
[1].

Also some drivers such as JPEG require 32bit addressing so not allocating
from higher DDR address.

Link: https://lore.kernel.org/all/20240605124859.3034-5-hnagalla@ti.com [1]
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 arch/arm64/boot/dts/ti/k3-am62a7-sk.dts | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts b/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts
index e026f65738b3..67faf46d7a35 100644
--- a/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts
+++ b/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts
@@ -40,6 +40,15 @@ reserved-memory {
 		#size-cells = <2>;
 		ranges;
 
+		/* global cma region */
+		linux,cma {
+			compatible = "shared-dma-pool";
+			reusable;
+			size = <0x00 0x24000000>;
+			alloc-ranges = <0x00 0xc0000000 0x00 0x24000000>;
+			linux,cma-default;
+		};
+
 		secure_tfa_ddr: tfa@9e780000 {
 			reg = <0x00 0x9e780000 0x00 0x80000>;
 			alignment = <0x1000>;
-- 
2.39.1

Re: [PATCH 2/3] arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA

[Brandon Brnich]

Hi Devarsh,

On 20:39-20240613, Devarsh Thakkar wrote:
> Reserve 576MiB of CMA as global CMA pool starting after initial 1GiB of
> DDR.
> 
> AM62ax has different multimedia components such as Camera, Display, H.264
> VPU and JPEG Encoder which use CMA for buffer allocations.
> 
> The 12x 720x480 realtime VPU decode use-case requires 544MiB of CMA,
> additional 32MiB is kept as buffer in case some other peripheral also
> require it while VPU is running.
> 
> The reason to choose latter 1GiB is to not overlap with existing memory map
> which is utilizing initial 1GiB for remoteproc firmwares as shared here
> [1].
> 
> Also some drivers such as JPEG require 32bit addressing so not allocating
> from higher DDR address.
> 
> Link: https://lore.kernel.org/all/20240605124859.3034-5-hnagalla@ti.com [1]
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>

I have validated that this patch works with VPU. 

Tested-by: Brandon Brnich <b-brnich@ti.com>

Best,
Brandon

> ---
>  arch/arm64/boot/dts/ti/k3-am62a7-sk.dts | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts b/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts
> index e026f65738b3..67faf46d7a35 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts
> @@ -40,6 +40,15 @@ reserved-memory {
>  		#size-cells = <2>;
>  		ranges;
>  
> +		/* global cma region */
> +		linux,cma {
> +			compatible = "shared-dma-pool";
> +			reusable;
> +			size = <0x00 0x24000000>;
> +			alloc-ranges = <0x00 0xc0000000 0x00 0x24000000>;
> +			linux,cma-default;
> +		};
> +
>  		secure_tfa_ddr: tfa@9e780000 {
>  			reg = <0x00 0x9e780000 0x00 0x80000>;
>  			alignment = <0x1000>;
> -- 
> 2.39.1
> 

[PATCH 3/3] arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA

[Devarsh Thakkar]

From: Brandon Brnich <b-brnich@ti.com>

AM62p has different multimedia components such as Camera, Display, H264
Video Codec which uses CMA for buffer allocations. We require 576MiB for 12
channel decode-to-encode 720x480@30FPS use case.

Signed-off-by: Brandon Brnich <b-brnich@ti.com>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 arch/arm64/boot/dts/ti/k3-am62p5-sk.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
index fb980d46e304..5ef74d9f8eea 100644
--- a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
+++ b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
@@ -48,6 +48,13 @@ reserved-memory {
 		#size-cells = <2>;
 		ranges;
 
+		linux,cma {
+			compatible = "shared-dma-pool";
+			reusable;
+			size = <0x00 0x24000000>;
+			linux,cma-default;
+		};
+
 		secure_tfa_ddr: tfa@9e780000 {
 			reg = <0x00 0x9e780000 0x00 0x80000>;
 			no-map;
-- 
2.39.1

Re: [PATCH 3/3] arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA

[Brandon Brnich]

Hi Devarsh, 


On 20:39-20240613, Devarsh Thakkar wrote:
> From: Brandon Brnich <b-brnich@ti.com>
> 
> AM62p has different multimedia components such as Camera, Display, H264
> Video Codec which uses CMA for buffer allocations. We require 576MiB for 12
> channel decode-to-encode 720x480@30FPS use case.
> 
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>  arch/arm64/boot/dts/ti/k3-am62p5-sk.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
> index fb980d46e304..5ef74d9f8eea 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
> @@ -48,6 +48,13 @@ reserved-memory {
>  		#size-cells = <2>;
>  		ranges;
>  
> +		linux,cma {
> +			compatible = "shared-dma-pool";
> +			reusable;
> +			size = <0x00 0x24000000>;
> +			linux,cma-default;
> +		};

Since AM62p has 8gb memory, this allocation can come from upper portion.
Doing so breaks Wave5 encoding/decoding as the driver can not yet handle
48 bit addressing. 48bit support is scheduled to be upstreamed, but unsure of
when this will actually make it in.  

Could we force this into lower 32bits using same
alloc-ranges as done in your AM62a patch[0]?


[0]: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240613150902.2173582-3-devarsht@ti.com/

Best,
Brandon

> +
>  		secure_tfa_ddr: tfa@9e780000 {
>  			reg = <0x00 0x9e780000 0x00 0x80000>;
>  			no-map;
> -- 
> 2.39.1
> 

Re: [PATCH 3/3] arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA

[Devarsh Thakkar]

Hi Vignesh,

On 14/06/24 22:34, Brandon Brnich wrote:
[..]

>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>> index fb980d46e304..5ef74d9f8eea 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>> +++ b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>> @@ -48,6 +48,13 @@ reserved-memory {
>>  		#size-cells = <2>;
>>  		ranges;
>>  
>> +		linux,cma {
>> +			compatible = "shared-dma-pool";
>> +			reusable;
>> +			size = <0x00 0x24000000>;
>> +			linux,cma-default;
>> +		};
> 
> Since AM62p has 8gb memory, this allocation can come from upper portion.
> Doing so breaks Wave5 encoding/decoding as the driver can not yet handle
> 48 bit addressing. 48bit support is scheduled to be upstreamed, but unsure of
> when this will actually make it in.  
> 
> Could we force this into lower 32bits using same
> alloc-ranges as done in your AM62a patch[0]?
> 


I would like to take a look at this separately, is it possible for you
to drop this patch [3/3] from series due to above comments and take the
rest two from the series ?

Regards
Devarsh

Re: [PATCH 0/3] Add global CMA reserve area

[Randolph Sapp]

On Thu Jun 13, 2024 at 10:08 AM CDT, Devarsh Thakkar wrote:
> Add global CMA reserve area for AM62x, AM62A and AM62P SoCs.
> These SoCs do not have MMU and hence require contiguous memory pool to
> support various multimedia use-cases.
>
> Brandon Brnich (1):
>   arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA
>
> Devarsh Thakkar (2):
>   arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA
>   arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA
>
>  arch/arm64/boot/dts/ti/k3-am62a7-sk.dts        | 9 +++++++++
>  arch/arm64/boot/dts/ti/k3-am62p5-sk.dts        | 7 +++++++
>  arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 8 ++++++++
>  3 files changed, 24 insertions(+)

I'm still a little torn about putting this allocation into the device tree
directly as the actual required allocation size depends on the task.

If it's allowed though, this series is fine for introducing those changes. This
uses the long-tested values we've been using on our tree for a bit now. The only
thing that's a little worrying is the missing range definitions for devices with
more than 32bits of addressable memory as Brandon has pointed out. Once that's
addressed:

Reviewed-by: Randolph Sapp <rs@ti.com>

Specifying these regions using the kernel cmdline parameter via u-boot was
brought up as a potential workaround. This is fine until you get into distro
boot methods which will almost certainly attempt to override those. I don't
know. Still a little odd. Curious how the community feels about it.

Technically the user or distro can still override it with the cmdline parameter
if necessary, so this may be the best way to have a useful default.

Re: [PATCH 0/3] Add global CMA reserve area

[Andrew Davis]

On 6/14/24 12:58 PM, Randolph Sapp wrote:
> On Thu Jun 13, 2024 at 10:08 AM CDT, Devarsh Thakkar wrote:
>> Add global CMA reserve area for AM62x, AM62A and AM62P SoCs.
>> These SoCs do not have MMU and hence require contiguous memory pool to
>> support various multimedia use-cases.
>>
>> Brandon Brnich (1):
>>    arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA
>>
>> Devarsh Thakkar (2):
>>    arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA
>>    arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA
>>
>>   arch/arm64/boot/dts/ti/k3-am62a7-sk.dts        | 9 +++++++++
>>   arch/arm64/boot/dts/ti/k3-am62p5-sk.dts        | 7 +++++++
>>   arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 8 ++++++++
>>   3 files changed, 24 insertions(+)
> 
> I'm still a little torn about putting this allocation into the device tree
> directly as the actual required allocation size depends on the task.
> 

That is the exact reason this does not belong in DT. For everyone *not*
using the most extreme case (12x decodes at the same time), this is
all wasted space. If one is running out of CMA, they can add more on
the kernel cmdline.

> If it's allowed though, this series is fine for introducing those changes. This
> uses the long-tested values we've been using on our tree for a bit now. The only
> thing that's a little worrying is the missing range definitions for devices with
> more than 32bits of addressable memory as Brandon has pointed out. Once that's
> addressed:
> 
> Reviewed-by: Randolph Sapp <rs@ti.com>
> 
> Specifying these regions using the kernel cmdline parameter via u-boot was
> brought up as a potential workaround. This is fine until you get into distro
> boot methods which will almost certainly attempt to override those. I don't
> know. Still a little odd. Curious how the community feels about it.
> 
> Technically the user or distro can still override it with the cmdline parameter
> if necessary, so this may be the best way to have a useful default.
> 

The most useful default is one that doesn't eat 576 MiB of memory "just in case".
Needing that much CMA is the exception case and should be the one that requires
adding something to the kernel cmdline.

If the kernel cmdline option does not work in some cases then we should
fix that instead of hard-coding a workaround here in DT. We are robbing
ourselves of a better solution here.

Andrew

Re: [PATCH 0/3] Add global CMA reserve area

[Devarsh Thakkar]

Hi Andrew, Vignesh,

On 24/06/24 22:03, Andrew Davis wrote:
> On 6/14/24 12:58 PM, Randolph Sapp wrote:
>> On Thu Jun 13, 2024 at 10:08 AM CDT, Devarsh Thakkar wrote:
>>> Add global CMA reserve area for AM62x, AM62A and AM62P SoCs.
>>> These SoCs do not have MMU and hence require contiguous memory pool to
>>> support various multimedia use-cases.
>>>
>>> Brandon Brnich (1):
>>>    arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA
>>>
>>> Devarsh Thakkar (2):
>>>    arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA
>>>    arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA
>>>
>>>   arch/arm64/boot/dts/ti/k3-am62a7-sk.dts        | 9 +++++++++
>>>   arch/arm64/boot/dts/ti/k3-am62p5-sk.dts        | 7 +++++++
>>>   arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 8 ++++++++
>>>   3 files changed, 24 insertions(+)
>>
>> I'm still a little torn about putting this allocation into the device tree
>> directly as the actual required allocation size depends on the task.
>>
> 
> That is the exact reason this does not belong in DT. For everyone *not*
> using the most extreme case (12x decodes at the same time), this is
> all wasted space. If one is running out of CMA, they can add more on
> the kernel cmdline.
> 

I disagree with this. The 12x decode for 480p is not an extreme use-case this
is something VPU is capable to run at optimum frame-rate (12x 1080p it can't)
and as the AM62A7 is meant to be AI + multimedia centric device, per the
device definition we were given the requirements to support a list of
multimedia use-cases which should work out of box and 12x decode for 480p was
one of them as device is very much capable of doing that with optimum
performance and I don't think it is right to change these requirements on the fly.

The AM62A7 board has 4 GiB of DDR and we have been using this CMA value since
more than a year, I have never heard anyone complain about out of memory or
CMA starvation and it suffices to requirements of *most use-cases*, but if for
some specific use-case it doesn't suffice, user can change it via kernel cmdline.

The kernelcmdline suggestion doesn't suffice out of box experience required,
we don't want to ask the user to reboot the board everytime they run out of CMA.


>> If it's allowed though, this series is fine for introducing those changes. This
>> uses the long-tested values we've been using on our tree for a bit now. The
>> only
>> thing that's a little worrying is the missing range definitions for devices
>> with
>> more than 32bits of addressable memory as Brandon has pointed out. Once that's
>> addressed:
>>
>> Reviewed-by: Randolph Sapp <rs@ti.com>
>>
>> Specifying these regions using the kernel cmdline parameter via u-boot was
>> brought up as a potential workaround. This is fine until you get into distro
>> boot methods which will almost certainly attempt to override those. I don't
>> know. Still a little odd. Curious how the community feels about it.
>>
>> Technically the user or distro can still override it with the cmdline parameter
>> if necessary, so this may be the best way to have a useful default.
>>
> 

Unlike above, this solution is independent of distro as it should be as we
want that all the supported multimedia use-cases should work out of box. This
solution is nothing illegal as CMA region carveouts are not a kernel
deprecated feature.

> The most useful default is one that doesn't eat 576 MiB of memory "just in case".
> Needing that much CMA is the exception case and should be the one that requires
> adding something to the kernel cmdline.
> 

I disagree with this, I would have agreed if this point was made in context of
generic device, but we are forgetting here that AM62A7 is a AI+multimedia
centric device and customers expect multimedia use-cases to work out of box.

We have 4 GiB RAM and if carving out 576 MiB is helping achieve all major
multimedia use-cases then what's the problem ? What exactly is failing for you
? If some specific scenarios are getting hurt then in that case overlays or
kernel cmdline option can be used to override the cma.

I feel we are over-complicating things here and going back-and-forth each
cycle even though there are no competing alternatives present today.
And this blocks out of box experience, as today even the basic HDMI and GPU
use-cases don't work out of box.

I had also discussed around this with community on last OSS summit as
discussed here [1] were it was suggested too to use this solution as adopted
by other vendors.

[1]: https://lore.kernel.org/all/0eee0424-f177-808f-3a86-499443155ddb@ti.com/

Regards
Devarsh

Re: [PATCH 0/3] Add global CMA reserve area

[Randolph Sapp]

On Fri Jun 28, 2024 at 10:57 AM CDT, Devarsh Thakkar wrote:
> Hi Andrew, Vignesh,
>
> On 24/06/24 22:03, Andrew Davis wrote:
> > On 6/14/24 12:58 PM, Randolph Sapp wrote:
> >> On Thu Jun 13, 2024 at 10:08 AM CDT, Devarsh Thakkar wrote:
> >>> Add global CMA reserve area for AM62x, AM62A and AM62P SoCs.
> >>> These SoCs do not have MMU and hence require contiguous memory pool to
> >>> support various multimedia use-cases.
> >>>
> >>> Brandon Brnich (1):
> >>>    arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA
> >>>
> >>> Devarsh Thakkar (2):
> >>>    arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA
> >>>    arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA
> >>>
> >>>   arch/arm64/boot/dts/ti/k3-am62a7-sk.dts        | 9 +++++++++
> >>>   arch/arm64/boot/dts/ti/k3-am62p5-sk.dts        | 7 +++++++
> >>>   arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 8 ++++++++
> >>>   3 files changed, 24 insertions(+)
> >>
> >> I'm still a little torn about putting this allocation into the device tree
> >> directly as the actual required allocation size depends on the task.
> >>
> > 
> > That is the exact reason this does not belong in DT. For everyone *not*
> > using the most extreme case (12x decodes at the same time), this is
> > all wasted space. If one is running out of CMA, they can add more on
> > the kernel cmdline.
> > 
>
> I disagree with this. The 12x decode for 480p is not an extreme use-case this
> is something VPU is capable to run at optimum frame-rate (12x 1080p it can't)
> and as the AM62A7 is meant to be AI + multimedia centric device, per the
> device definition we were given the requirements to support a list of
> multimedia use-cases which should work out of box and 12x decode for 480p was
> one of them as device is very much capable of doing that with optimum
> performance and I don't think it is right to change these requirements on the fly.
>
> The AM62A7 board has 4 GiB of DDR and we have been using this CMA value since
> more than a year, I have never heard anyone complain about out of memory or
> CMA starvation and it suffices to requirements of *most use-cases*, but if for
> some specific use-case it doesn't suffice, user can change it via kernel cmdline.
>
> The kernelcmdline suggestion doesn't suffice out of box experience required,
> we don't want to ask the user to reboot the board everytime they run out of CMA.
>
>
> >> If it's allowed though, this series is fine for introducing those changes. This
> >> uses the long-tested values we've been using on our tree for a bit now. The
> >> only
> >> thing that's a little worrying is the missing range definitions for devices
> >> with
> >> more than 32bits of addressable memory as Brandon has pointed out. Once that's
> >> addressed:
> >>
> >> Reviewed-by: Randolph Sapp <rs@ti.com>
> >>
> >> Specifying these regions using the kernel cmdline parameter via u-boot was
> >> brought up as a potential workaround. This is fine until you get into distro
> >> boot methods which will almost certainly attempt to override those. I don't
> >> know. Still a little odd. Curious how the community feels about it.
> >>
> >> Technically the user or distro can still override it with the cmdline parameter
> >> if necessary, so this may be the best way to have a useful default.
> >>
> > 
>
> Unlike above, this solution is independent of distro as it should be as we
> want that all the supported multimedia use-cases should work out of box. This
> solution is nothing illegal as CMA region carveouts are not a kernel
> deprecated feature.

Right. I support this change for at least introducing a usable default. 32M of
CMA is barely enough to run glmark2 under Weston once everything's up and
running.

As I said before, the user or distro can still override the dt CMA block with
the cma kernel parameter if they aren't happy with the default block.
Unfortunately this is about the only way to have a usable default value to fall
back on.

> > The most useful default is one that doesn't eat 576 MiB of memory "just in case".
> > Needing that much CMA is the exception case and should be the one that requires
> > adding something to the kernel cmdline.
> > 
>
> I disagree with this, I would have agreed if this point was made in context of
> generic device, but we are forgetting here that AM62A7 is a AI+multimedia
> centric device and customers expect multimedia use-cases to work out of box.
>
> We have 4 GiB RAM and if carving out 576 MiB is helping achieve all major
> multimedia use-cases then what's the problem ? What exactly is failing for you
> ? If some specific scenarios are getting hurt then in that case overlays or
> kernel cmdline option can be used to override the cma.
>
> I feel we are over-complicating things here and going back-and-forth each
> cycle even though there are no competing alternatives present today.
> And this blocks out of box experience, as today even the basic HDMI and GPU
> use-cases don't work out of box.
>
> I had also discussed around this with community on last OSS summit as
> discussed here [1] were it was suggested too to use this solution as adopted
> by other vendors.
>
> [1]: https://lore.kernel.org/all/0eee0424-f177-808f-3a86-499443155ddb@ti.com/
>
> Regards
> Devarsh

If the community accepts it, it's fine by me.

- Randolph

Re: [PATCH 0/3] Add global CMA reserve area

[Vignesh Raghavendra]

On 28/06/24 22:05, Randolph Sapp wrote:
> On Fri Jun 28, 2024 at 10:57 AM CDT, Devarsh Thakkar wrote:
>> Hi Andrew, Vignesh,
>>
>> On 24/06/24 22:03, Andrew Davis wrote:
>>> On 6/14/24 12:58 PM, Randolph Sapp wrote:
>>>> On Thu Jun 13, 2024 at 10:08 AM CDT, Devarsh Thakkar wrote:
>>>>> Add global CMA reserve area for AM62x, AM62A and AM62P SoCs.
>>>>> These SoCs do not have MMU and hence require contiguous memory pool to
>>>>> support various multimedia use-cases.
>>>>>
>>>>> Brandon Brnich (1):
>>>>>    arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA
>>>>>
>>>>> Devarsh Thakkar (2):
>>>>>    arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA
>>>>>    arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA
>>>>>
>>>>>   arch/arm64/boot/dts/ti/k3-am62a7-sk.dts        | 9 +++++++++
>>>>>   arch/arm64/boot/dts/ti/k3-am62p5-sk.dts        | 7 +++++++
>>>>>   arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 8 ++++++++
>>>>>   3 files changed, 24 insertions(+)
>>>>
>>>> I'm still a little torn about putting this allocation into the device tree
>>>> directly as the actual required allocation size depends on the task.
>>>>
>>>
>>> That is the exact reason this does not belong in DT. For everyone *not*
>>> using the most extreme case (12x decodes at the same time), this is
>>> all wasted space. If one is running out of CMA, they can add more on
>>> the kernel cmdline.
>>>
>>
>> I disagree with this. The 12x decode for 480p is not an extreme use-case this
>> is something VPU is capable to run at optimum frame-rate (12x 1080p it can't)
>> and as the AM62A7 is meant to be AI + multimedia centric device, per the
>> device definition we were given the requirements to support a list of
>> multimedia use-cases which should work out of box and 12x decode for 480p was
>> one of them as device is very much capable of doing that with optimum
>> performance and I don't think it is right to change these requirements on the fly.
>>
>> The AM62A7 board has 4 GiB of DDR and we have been using this CMA value since
>> more than a year, I have never heard anyone complain about out of memory or
>> CMA starvation and it suffices to requirements of *most use-cases*, but if for
>> some specific use-case it doesn't suffice, user can change it via kernel cmdline.
>>
>> The kernelcmdline suggestion doesn't suffice out of box experience required,
>> we don't want to ask the user to reboot the board everytime they run out of CMA.
>>
>>
>>>> If it's allowed though, this series is fine for introducing those changes. This
>>>> uses the long-tested values we've been using on our tree for a bit now. The
>>>> only
>>>> thing that's a little worrying is the missing range definitions for devices
>>>> with
>>>> more than 32bits of addressable memory as Brandon has pointed out. Once that's
>>>> addressed:
>>>>
>>>> Reviewed-by: Randolph Sapp <rs@ti.com>
>>>>
>>>> Specifying these regions using the kernel cmdline parameter via u-boot was
>>>> brought up as a potential workaround. This is fine until you get into distro
>>>> boot methods which will almost certainly attempt to override those. I don't
>>>> know. Still a little odd. Curious how the community feels about it.
>>>>
>>>> Technically the user or distro can still override it with the cmdline parameter
>>>> if necessary, so this may be the best way to have a useful default.
>>>>
>>>
>>
>> Unlike above, this solution is independent of distro as it should be as we
>> want that all the supported multimedia use-cases should work out of box. This
>> solution is nothing illegal as CMA region carveouts are not a kernel
>> deprecated feature.
> 
> Right. I support this change for at least introducing a usable default. 32M of
> CMA is barely enough to run glmark2 under Weston once everything's up and
> running.
> 
> As I said before, the user or distro can still override the dt CMA block with
> the cma kernel parameter if they aren't happy with the default block.
> Unfortunately this is about the only way to have a usable default value to fall
> back on.
> 


Given the number of SoMs and non TI EVMs that are about to come out with
AM62A/P and AM67s, we need to provide a consistent way of being able to
support multimedia IPs out of the box. Modifying cmdline may not always
be feasible given distro defaults don't always provide a way to do so.

So I am inclined to queue first 2 patches unless there is another way t
achieve this.

[...]

-- 
Regards
Vignesh

Re: [PATCH 0/3] Add global CMA reserve area

[Andrew Davis]

On 6/30/24 2:09 AM, Vignesh Raghavendra wrote:
> 
> 
> On 28/06/24 22:05, Randolph Sapp wrote:
>> On Fri Jun 28, 2024 at 10:57 AM CDT, Devarsh Thakkar wrote:
>>> Hi Andrew, Vignesh,
>>>
>>> On 24/06/24 22:03, Andrew Davis wrote:
>>>> On 6/14/24 12:58 PM, Randolph Sapp wrote:
>>>>> On Thu Jun 13, 2024 at 10:08 AM CDT, Devarsh Thakkar wrote:
>>>>>> Add global CMA reserve area for AM62x, AM62A and AM62P SoCs.
>>>>>> These SoCs do not have MMU and hence require contiguous memory pool to
>>>>>> support various multimedia use-cases.
>>>>>>
>>>>>> Brandon Brnich (1):
>>>>>>     arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA
>>>>>>
>>>>>> Devarsh Thakkar (2):
>>>>>>     arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA
>>>>>>     arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA
>>>>>>
>>>>>>    arch/arm64/boot/dts/ti/k3-am62a7-sk.dts        | 9 +++++++++
>>>>>>    arch/arm64/boot/dts/ti/k3-am62p5-sk.dts        | 7 +++++++
>>>>>>    arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 8 ++++++++
>>>>>>    3 files changed, 24 insertions(+)
>>>>>
>>>>> I'm still a little torn about putting this allocation into the device tree
>>>>> directly as the actual required allocation size depends on the task.
>>>>>
>>>>
>>>> That is the exact reason this does not belong in DT. For everyone *not*
>>>> using the most extreme case (12x decodes at the same time), this is
>>>> all wasted space. If one is running out of CMA, they can add more on
>>>> the kernel cmdline.
>>>>
>>>
>>> I disagree with this. The 12x decode for 480p is not an extreme use-case this
>>> is something VPU is capable to run at optimum frame-rate (12x 1080p it can't)
>>> and as the AM62A7 is meant to be AI + multimedia centric device, per the
>>> device definition we were given the requirements to support a list of
>>> multimedia use-cases which should work out of box and 12x decode for 480p was
>>> one of them as device is very much capable of doing that with optimum
>>> performance and I don't think it is right to change these requirements on the fly.
>>>
>>> The AM62A7 board has 4 GiB of DDR and we have been using this CMA value since
>>> more than a year, I have never heard anyone complain about out of memory or
>>> CMA starvation and it suffices to requirements of *most use-cases*, but if for
>>> some specific use-case it doesn't suffice, user can change it via kernel cmdline.
>>>
>>> The kernelcmdline suggestion doesn't suffice out of box experience required,
>>> we don't want to ask the user to reboot the board everytime they run out of CMA.
>>>
>>>
>>>>> If it's allowed though, this series is fine for introducing those changes. This
>>>>> uses the long-tested values we've been using on our tree for a bit now. The
>>>>> only
>>>>> thing that's a little worrying is the missing range definitions for devices
>>>>> with
>>>>> more than 32bits of addressable memory as Brandon has pointed out. Once that's
>>>>> addressed:
>>>>>
>>>>> Reviewed-by: Randolph Sapp <rs@ti.com>
>>>>>
>>>>> Specifying these regions using the kernel cmdline parameter via u-boot was
>>>>> brought up as a potential workaround. This is fine until you get into distro
>>>>> boot methods which will almost certainly attempt to override those. I don't
>>>>> know. Still a little odd. Curious how the community feels about it.
>>>>>
>>>>> Technically the user or distro can still override it with the cmdline parameter
>>>>> if necessary, so this may be the best way to have a useful default.
>>>>>
>>>>
>>>
>>> Unlike above, this solution is independent of distro as it should be as we
>>> want that all the supported multimedia use-cases should work out of box. This
>>> solution is nothing illegal as CMA region carveouts are not a kernel
>>> deprecated feature.
>>
>> Right. I support this change for at least introducing a usable default. 32M of
>> CMA is barely enough to run glmark2 under Weston once everything's up and
>> running.
>>
>> As I said before, the user or distro can still override the dt CMA block with
>> the cma kernel parameter if they aren't happy with the default block.
>> Unfortunately this is about the only way to have a usable default value to fall
>> back on.
>>
> 
> 
> Given the number of SoMs and non TI EVMs that are about to come out with
> AM62A/P and AM67s, we need to provide a consistent way of being able to
> support multimedia IPs out of the box. Modifying cmdline may not always
> be feasible given distro defaults don't always provide a way to do so.
> 

We need to keep thinking then. I empathize with desire to put
configuration in Device Tree. DT feels like a great spot for it,
it is ubiquitous on these boards and has a good bit of tooling around
it. We are already describing the hardware, why not configure it here
too? But the reason we do not want to go down that road is simple:
DT takes away use-case flexibility. A lack of flexibility is fine for
hardware which is unchanging, but not for configuration.

Device policy and configuration must be left to userspace.

It is not for us to decide how folks should use our hardware, and
that is what we are doing when we configure it in DT.

For configuration that must happen in early boot before userspace is
available (such as kernel stdout and memory carveouts) we have the
kernel cmdline. If we find something that cannot be done today though
the cmdline, then we should add that support to the cmdline, not
give up and just hide the configuration in DT.

What this series does is already available on the kernel cmdline.
Our bootloader can provide sane defaults on the cmdline today.
If the worry is that distros will override this default then
go fix those distros.

> So I am inclined to queue first 2 patches unless there is another way t
> achieve this.
> 

Our lack of creativity in finding better solutions to this issue is
not an excuse to add more junk to DT..

Andrew

> [...]
> 

Re: [PATCH 0/3] Add global CMA reserve area

[Randolph Sapp]

On Mon Jul 1, 2024 at 9:33 AM CDT, Andrew Davis wrote:
> On 6/30/24 2:09 AM, Vignesh Raghavendra wrote:
> > 
> > 
> > On 28/06/24 22:05, Randolph Sapp wrote:
> >> On Fri Jun 28, 2024 at 10:57 AM CDT, Devarsh Thakkar wrote:
> >>> Hi Andrew, Vignesh,
> >>>
> >>> On 24/06/24 22:03, Andrew Davis wrote:
> >>>> On 6/14/24 12:58 PM, Randolph Sapp wrote:
> >>>>> On Thu Jun 13, 2024 at 10:08 AM CDT, Devarsh Thakkar wrote:
> >>>>>> Add global CMA reserve area for AM62x, AM62A and AM62P SoCs.
> >>>>>> These SoCs do not have MMU and hence require contiguous memory pool to
> >>>>>> support various multimedia use-cases.
> >>>>>>
> >>>>>> Brandon Brnich (1):
> >>>>>>     arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA
> >>>>>>
> >>>>>> Devarsh Thakkar (2):
> >>>>>>     arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA
> >>>>>>     arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA
> >>>>>>
> >>>>>>    arch/arm64/boot/dts/ti/k3-am62a7-sk.dts        | 9 +++++++++
> >>>>>>    arch/arm64/boot/dts/ti/k3-am62p5-sk.dts        | 7 +++++++
> >>>>>>    arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 8 ++++++++
> >>>>>>    3 files changed, 24 insertions(+)
> >>>>>
> >>>>> I'm still a little torn about putting this allocation into the device tree
> >>>>> directly as the actual required allocation size depends on the task.
> >>>>>
> >>>>
> >>>> That is the exact reason this does not belong in DT. For everyone *not*
> >>>> using the most extreme case (12x decodes at the same time), this is
> >>>> all wasted space. If one is running out of CMA, they can add more on
> >>>> the kernel cmdline.
> >>>>
> >>>
> >>> I disagree with this. The 12x decode for 480p is not an extreme use-case this
> >>> is something VPU is capable to run at optimum frame-rate (12x 1080p it can't)
> >>> and as the AM62A7 is meant to be AI + multimedia centric device, per the
> >>> device definition we were given the requirements to support a list of
> >>> multimedia use-cases which should work out of box and 12x decode for 480p was
> >>> one of them as device is very much capable of doing that with optimum
> >>> performance and I don't think it is right to change these requirements on the fly.
> >>>
> >>> The AM62A7 board has 4 GiB of DDR and we have been using this CMA value since
> >>> more than a year, I have never heard anyone complain about out of memory or
> >>> CMA starvation and it suffices to requirements of *most use-cases*, but if for
> >>> some specific use-case it doesn't suffice, user can change it via kernel cmdline.
> >>>
> >>> The kernelcmdline suggestion doesn't suffice out of box experience required,
> >>> we don't want to ask the user to reboot the board everytime they run out of CMA.
> >>>
> >>>
> >>>>> If it's allowed though, this series is fine for introducing those changes. This
> >>>>> uses the long-tested values we've been using on our tree for a bit now. The
> >>>>> only
> >>>>> thing that's a little worrying is the missing range definitions for devices
> >>>>> with
> >>>>> more than 32bits of addressable memory as Brandon has pointed out. Once that's
> >>>>> addressed:
> >>>>>
> >>>>> Reviewed-by: Randolph Sapp <rs@ti.com>
> >>>>>
> >>>>> Specifying these regions using the kernel cmdline parameter via u-boot was
> >>>>> brought up as a potential workaround. This is fine until you get into distro
> >>>>> boot methods which will almost certainly attempt to override those. I don't
> >>>>> know. Still a little odd. Curious how the community feels about it.
> >>>>>
> >>>>> Technically the user or distro can still override it with the cmdline parameter
> >>>>> if necessary, so this may be the best way to have a useful default.
> >>>>>
> >>>>
> >>>
> >>> Unlike above, this solution is independent of distro as it should be as we
> >>> want that all the supported multimedia use-cases should work out of box. This
> >>> solution is nothing illegal as CMA region carveouts are not a kernel
> >>> deprecated feature.
> >>
> >> Right. I support this change for at least introducing a usable default. 32M of
> >> CMA is barely enough to run glmark2 under Weston once everything's up and
> >> running.
> >>
> >> As I said before, the user or distro can still override the dt CMA block with
> >> the cma kernel parameter if they aren't happy with the default block.
> >> Unfortunately this is about the only way to have a usable default value to fall
> >> back on.
> >>
> > 
> > 
> > Given the number of SoMs and non TI EVMs that are about to come out with
> > AM62A/P and AM67s, we need to provide a consistent way of being able to
> > support multimedia IPs out of the box. Modifying cmdline may not always
> > be feasible given distro defaults don't always provide a way to do so.
> > 
>
> We need to keep thinking then. I empathize with desire to put
> configuration in Device Tree. DT feels like a great spot for it,
> it is ubiquitous on these boards and has a good bit of tooling around
> it. We are already describing the hardware, why not configure it here
> too? But the reason we do not want to go down that road is simple:
> DT takes away use-case flexibility. A lack of flexibility is fine for
> hardware which is unchanging, but not for configuration.

I agree with the sentiment here, but this explicit case is an exception. This is
no more than a default value. Userspace can still change this allocation with
the kernel cma parameter. That still takes precedence.

> Device policy and configuration must be left to userspace.
>
> It is not for us to decide how folks should use our hardware, and
> that is what we are doing when we configure it in DT.
>
> For configuration that must happen in early boot before userspace is
> available (such as kernel stdout and memory carveouts) we have the
> kernel cmdline. If we find something that cannot be done today though
> the cmdline, then we should add that support to the cmdline, not
> give up and just hide the configuration in DT.
>
> What this series does is already available on the kernel cmdline.
> Our bootloader can provide sane defaults on the cmdline today.
> If the worry is that distros will override this default then
> go fix those distros.

Our bootloaders doing anything to kernel cmdline parameters is inherently
non-standard. You just brought up userspace control. They clobber cmdline
parameters all the time. Unsetting this bootloader value will be the first thing
they do. This will be done passively, since they normally inject a new
intermediary boot stage that they can actually control between u-boot and their
kernel.

- Randolph

> > So I am inclined to queue first 2 patches unless there is another way t
> > achieve this.
> > 
>
> Our lack of creativity in finding better solutions to this issue is
> not an excuse to add more junk to DT..
>
> Andrew
>
> > [...]
> > 

Re: (subset)[PATCH 0/3] Add global CMA reserve area

[Vignesh Raghavendra]

Hi Devarsh Thakkar,

On Thu, 13 Jun 2024 20:38:59 +0530, Devarsh Thakkar wrote:
> Add global CMA reserve area for AM62x, AM62A and AM62P SoCs.
> These SoCs do not have MMU and hence require contiguous memory pool to
> support various multimedia use-cases.
> 
> Brandon Brnich (1):
>   arm64: dts: ti: k3-am62p5-sk: Reserve 576 MiB of global CMA
> 
> [...]

I have applied the following to branch ti-k3-dts-next on [1].
Thank you!

[1/3] arm64: dts: ti: k3-am62x-sk-common: Reserve 128MiB of global CMA
      commit: 9e8560556f9c41da28118af3385b4e9dc832ae2b
[2/3] arm64: dts: ti: k3-am62a7-sk: Reserve 576MiB of global CMA
      commit: 6406c5d5512c0814b8c155df7f833a98d9069a72

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent up the chain during
the next merge window (or sooner if it is a relevant bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/ti/linux.git
--
Vignesh