Re: [PATCH v1] ARM: dts: sun8i: h3: orangepi-pc: Add CMA reserved memory node

[Dmitry Osipenko]

On 10/13/22 01:17, Jernej Škrabec wrote:
> Hi,
> 
> Dne četrtek, 15. september 2022 ob 11:01:36 CEST je Dmitry Osipenko 
> napisal(a):
>> Hi,
>>
>> On 9/14/22 21:34, Jernej Škrabec wrote:
>>> Dne sreda, 14. september 2022 ob 17:33:27 CEST je Clément Péron 
> napisal(a):
>>>> Hi Dmitry,
>>>>
>>>> On Wed, 14 Sept 2022 at 17:12, Dmitry Osipenko
>>>>
>>>> <dmitry.osipenko@xxxxxxxxxxxxx> wrote:
>>>>> Add 256MB CMA node to the Orange Pi PC board. This fixes memory
>>>>> allocation
>>>>> failures for Cedrus video decoder on trying to play a 1080p video with
>>>>> gstreamer.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>>  arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts | 14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
>>>>> b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts index
>>>>> b96e015f54ee..e655346a9fb4 100644
>>>>> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
>>>>> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
>>>>> @@ -60,6 +60,20 @@ chosen {
>>>>>
>>>>>                 stdout-path = "serial0:115200n8";
>>>>>         
>>>>>         };
>>>>>
>>>>> +       reserved-memory {
>>>>> +               #address-cells = <1>;
>>>>> +               #size-cells = <1>;
>>>>> +               ranges;
>>>>> +
>>>>> +               linux,cma@40000000 {
>>>>> +                       compatible = "shared-dma-pool";
>>>>> +                       alloc-ranges = <0x40000000 0x40000000>;
>>>>> +                       size = <0x10000000>; /* 256MiB */
>>>>> +                       linux,cma-default;
>>>>> +                       reusable;
>>>>> +               };
>>>>> +       };
>>>>> +
>>>>
>>>> This change seems legit for all H3 boards and could be moved to the H3
>>>> dtsi, no?
>>>
>>> That's true. However, there is a reason why this node doesn't exist. One
>>> or
>>> two H2+ boards (which use H3 dtsi) have only 256 MiB of RAM, so this can't
>>> work with them. A few H3 boards have 512 MiB of RAM, so you eat basically
>>> half of the RAM with that.
>>
>> It's a "reusable" CMA, hence it won't be eaten. System is free to use
>> the reusable CMA. In practice, CMA may get populated with a pinned pages
>> over time, hence system will work fine, but CMA will get fragmented and
>> this may cause problems for a larger CMA allocations.
>>
>> The main problem with 512M boards should be that they may not have a
>> suitable area for 256M CMA because it should be only either a low or
>> high part of the memory that might be busy at a boot time, and then
>> kernel will fail to boot.
>>
>>> Additionally, contrary to A20 and similar SoCs, which
>>> have such node, Cedrus can address whole RAM, so this is not strictly
>>> needed. It's better to leave this decision to distribution. Some don't
>>> care about Cedrus and some do a lot. Default size can be set via kernel
>>> config and it can be overriden by kernel argument if necessary.
>>
>> In my experience generic distributions usually don't care about
>> particular boards/devices. They ship a multiplatform kernel using
>> default config that has 64M for CMA and Cedrus doesn't work well with that.
> 
> It still can be overriden using cma= kernel argument.

The point of the DT change is to make system usable out-of-the-box for
casual (non kernel developer) users.

>> BTW, the sunxi_defconfig doesn't specify CMA size at all, so it defaults
>> to 16M.
> 
> That can be easily changed, someone just need to send patch. Historically, 
> from A20 times, default choice for CMA size was 96 MiB. Good choice, and in my 
> opinion also maximum, is 128 MiB. That's enough for 1080p. 256 MiB is really 
> only needed for 4k content, which only H3 and its variants support, from what 
> I can tell.

128M wasn't enough for playing 1080p h265 big_bug_bunny. Practically it
doesn't matter how big CMA area is, as long as it's "shared".

In my experience 256M CMA is a good choice for devices with 1G RAM.
Bigger CMA may cause problems for initrd, depending on a bootlaoder version.

-- 
Best regards,
Dmitry