Re: [PATCH v4 6/7] ARM: dts: exynos: convert MFC device to generic reserved memory bindings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hello Marek,

On 05/27/2016 07:32 AM, Marek Szyprowski wrote:
> Hello,
> 
> 
> On 2016-05-25 19:11, Javier Martinez Canillas wrote:
>> Hello Marek,
>>
>> On 05/24/2016 09:31 AM, Marek Szyprowski wrote:
>>> This patch replaces custom properties for defining reserved memory
>>> regions with generic reserved memory bindings for MFC video codec
>>> device.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>> ---
>> [snip]
>>
>>> +
>>> +/ {
>>> +    reserved-memory {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges;
>>> +
>>> +        mfc_left: region@51000000 {
>>> +            compatible = "shared-dma-pool";
>>> +            no-map;
>>> +            reg = <0x51000000 0x800000>;
>>> +        };
>>> +
>>> +        mfc_right: region@43000000 {
>>> +            compatible = "shared-dma-pool";
>>> +            no-map;
>>> +            reg = <0x43000000 0x800000>;
>>> +        };
>>> +    };
>> I've a question probably for a follow up patch, but do you know what's a
>> sane default size for these? I needed to bump the mfc_left size from 8 MiB
>> to 16 MiB in order to decode a 480p H264 video using GStramer. So clearly
>> the default sizes are not that useful.
> 
> Right, the default size for the 'left' region can be increased. Frankly, those
> values (8MiB/0x43000000+ 8MiB/0x51000000) comes from my initial patches
> prepared for some demo and don't have much with any real requirements. They
> were copied (blindly...) by various developers without any deeper understanding.

Yes, I've to admit that I was one of those when added the MFC regions to the
Peach Chromebooks but worked because I tested with small videos. When trying
to decode bigger videos, then had to increase the 'left' region as mentioned.

> Probably the most sane would be to use something like this:
> 
> mfc_left: region_mfc_left {
>          compatible = "shared-dma-pool";
>          no-map;
>          size = <0x1000000>;
>          alignment = <0x100000>;
> };
> 
> mfc_right: region_mfc_right {
>          compatible = "shared-dma-pool";
>          no-map;
>          size = <0x800000>;
>          alignment = <0x100000>;
> };
> 
> So the region will be allocated automatically from the available memory. This way
> another nice feature of the generic reserved memory regions can be used.
>

That sounds better indeed. Not requiring a certain memory offset will also have the
nice side effect to prevent conflicts like the one Pankaj had with his initramfs [0].
 
> The only platform which really requires MFC regions to be placed at certain memory
> offsets is Samsung S5PV210/S5PC110 (sometimes called exynos3), where there is no
> memory address interleaving and MFC device has limited memory interface, which cannot
> do 2 transactions to the same physical memory bank. However S5PV210/S5PC110 machine
> code lost support for MFC during conversion to device tree, so it is not a problem
> here.
>
> All newer platforms (Exynos4, Exynos3250, Exynos5+) use memory interleaving, so the
> actual offset of memory bank has no influence on the physical memory bank.
>

I see, thanks a lot for the explanation.
 
>>> +};
>>> diff --git a/arch/arm/boot/dts/exynos4210-origen.dts b/arch/arm/boot/dts/exynos4210-origen.dts
>>> index ad7394c..f5e4eb2 100644
>>> --- a/arch/arm/boot/dts/exynos4210-origen.dts
>>> +++ b/arch/arm/boot/dts/exynos4210-origen.dts
>>> @@ -18,6 +18,7 @@
>>>   #include "exynos4210.dtsi"
>>>   #include <dt-bindings/gpio/gpio.h>
>>>   #include <dt-bindings/input/input.h>
>>> +#include "exynos-mfc-reserved-memory.dtsi"
>>>     / {
>>>       model = "Insignal Origen evaluation board based on Exynos4210";
>>> @@ -288,8 +289,7 @@
>>>   };
>>>     &mfc {
>>> -    samsung,mfc-r = <0x43000000 0x800000>;
>>> -    samsung,mfc-l = <0x51000000 0x800000>;
>>> +    memory-region = <&mfc_left>, <&mfc_right>;
>>>       status = "okay";
>> I wonder if shouldn't be better to include the exynos-mfc-reserved-memory.dtsi
>> on each SoC dtsi and set the memory-regions in the MFC node instead of doing
>> it on each DTS, and let DTS to just replace with its own memory regions if the
>> default sizes are not suitable for them.
> 
> I don't have strong opinion on this. Maybe it would make more sense to move the
> following entry:
> 
> &mfc {
>         memory-region = <&mfc_left>, <&mfc_right>;
> };
> 
> also to the exynos-mfc-reserved-memory.dtsi ? So board will just include it if
> it want to use MFC device with reserved memory regions.
>

Ok, that also sounds like a good option for me.
 
>> Reviewed-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>> Tested-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> 
> Best regards

[0]: https://lkml.org/lkml/2016/5/26/98

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux