Message-ID: <7f1b4a45-04cf-4b6d-878a-5bd0054a9ac0@collabora.com>
Date: Thu, 6 Jun 2024 13:17:07 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: CK Hu (胡俊光) <ck.hu@...iatek.com>,
 "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "tzimmermann@...e.de" <tzimmermann@...e.de>,
 Shawn Sung (宋孝謙) <Shawn.Sung@...iatek.com>,
 "mripard@...nel.org" <mripard@...nel.org>,
 Jitao Shi (石记涛) <jitao.shi@...iatek.com>,
 "daniel@...ll.ch" <daniel@...ll.ch>,
 "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>,
 "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
 "robh@...nel.org" <robh@...nel.org>,
 "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
 "airlied@...il.com" <airlied@...il.com>,
 "krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
 "kernel@...labora.com" <kernel@...labora.com>,
 "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
 Yu-chang Lee (李禹璋) <Yu-chang.Lee@...iatek.com>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "amergnat@...libre.com" <amergnat@...libre.com>
Subject: Re: [PATCH v5 2/3] dt-bindings: arm: mediatek: mmsys: Add OF graph
 support for board path

Il 06/06/24 08:46, Chen-Yu Tsai ha scritto:
> On Wed, Jun 5, 2024 at 7:15 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.com> wrote:
>>
>> Il 05/06/24 03:38, CK Hu (胡俊光) ha scritto:
>>> Hi, Angelo:
>>>
>>> On Tue, 2024-05-21 at 09:57 +0200, AngeloGioacchino Del Regno wrote:
>>>> Document OF graph on MMSYS/VDOSYS: this supports up to three DDP paths
>>>> per HW instance (so potentially up to six displays for multi-vdo SoCs).
>>>>
>>>> The MMSYS or VDOSYS is always the first component in the DDP pipeline,
>>>> so it only supports an output port with multiple endpoints - where each
>>>> endpoint defines the starting point for one of the (currently three)
>>>> possible hardware paths.
>>>>
>>>> Reviewed-by: Rob Herring (Arm) <robh@...nel.org>
>>>> Reviewed-by: Alexandre Mergnat <amergnat@...libre.com>
>>>> Tested-by: Alexandre Mergnat <amergnat@...libre.com>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>>>> ---
>>>>    .../bindings/arm/mediatek/mediatek,mmsys.yaml | 28 +++++++++++++++++++
>>>>    1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>>>> index b3c6888c1457..0ef67ca4122b 100644
>>>> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>>>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>>>> @@ -93,6 +93,34 @@ properties:
>>>>      '#reset-cells':
>>>>        const: 1
>>>>
>>>> +  port:
>>>> +    $ref: /schemas/graph.yaml#/properties/port
>>>> +    description:
>>>> +      Output port node. This port connects the MMSYS/VDOSYS output to
>>>> +      the first component of one display pipeline, for example one of
>>>> +      the available OVL or RDMA blocks.
>>>> +      Some MediaTek SoCs support multiple display outputs per MMSYS.
>>>
>>> This patch looks good to me. Just want to share another information for you.
>>> Here is an example that mmsys/vdosys could point to the display interface node.
>>>
>>> vdosys0: syscon@...1a000 {
>>>             mmsys-display-interface = <&dsi0>, <&dsi1>, <&dp_intf0>;
>>> };
>>>
>>> vdosys1: syscon@...00000 {
>>>             mmsys-display-interface = <&dp_intf1>;
>>> };
>>>
>>> There is no conflict that mmsys/vdosys point to first component of one display pipeline or point to display interface.
>>> Both could co-exist.
>>>
>>
>> Hey CK,
>>
>> yes, this could be an alternative to the OF graphs, and I'm sure that it'd work,
>> even though this kind of solution would still require partial hardcoding of the
>> display paths up until mmsys-display-interface (so, up until DSI0, or DSI1, etc).
> 
> I think you might be misunderstanding CK's proposal? He's simply saying that
> instead of pointing to the start of the pipeline, point to the end instead.
> You can still use the OF graph and work backwards from the output.
> 

Oh, well, if I'm misunderstanding, sorry about that!

Though, OF Graphs are describing a "sequence of stuff" (sorry for the suboptimal
wording) and, well, the data goes from A to C with B in the middle, so the graph
starts at A, goes to B, then goes to C.

Starting from A, going to C, then backwards to B would be actually wrong, as that
appears to describe that the pipeline goes A->C->B instead of A->B->C.

>> The problem with a solution like this is that, well, even though it would work,
>> even if we ignore the suboptimal partial hardcoding, OF graphs are something
>> generic, while the mmsys-display-interface would be a MediaTek specific/custom
>> property.
>>
>> In the end, reusing generic kernel apis/interfaces/etc is always preferred
>> compared to custom solutions, especially in this case, in which the generic
>> stuff is on-par (or actually, depending purely on personal opinions, superior).
> 
> Here you are mixing hardware descriptions and kernel implementation details.
> 

Not really. I'm saying that OF Graph would be preferred compared to a custom
propety, when they both do the same.

But again, I might have misunderstood what CK was trying to say, so just leave it.

> I think this goes back to whether the mmsys/vdosys is actually part of the
> graph or not. It certainly controls the muxes within the graph. But that
> doesn't mean it has to be within the graph itself. It can just have pointers
> to entry points of the graph (for which you would have a couple lines of
> custom code [1]). If the data doesn't flow through the mmsys/vdosys, then

               ^^^ [1] is a link I think? You missed it! :-)

> I would argue that it is not part of the graph.

It's part of the graph, because it is setting up the pipeline - and it's doing that
in hardware, by doing the muxing as you said.
I could even go on arguing that the data does actually pass through that, but I
don't want to start any big deal around that, so I won't.

> 
> I would also argue that the data path should be fully described in the
> device tree, not hardcoding paths based on board usage.

Yes, that's what I also said (perhaps I should've been clearer) since the very
beginning, and it's exactly what made me put effort in making this series, so
we are totally agreeing on this point.


Cheers,
Angelo

> The latter is
> a policy / design decision, not a hardware capability.
> 
> 
> ChenYu
> 
>> As for the two to co-exist, I'm not sure that this is actually needed, as the
>> OF graphs are already (at the end of the graph) pointing to the display interface.
>>
>> In any case, just as a reminder: if there will be any need to add any custom
>> MediaTek specific properties later, it's ok and we can do that at any time.
>>
>> Cheers!
>> Angelo
>>
>>> Regards,
>>> CK
>>>
>>>> +    properties:
>>>> +      endpoint@0:
>>>> +        $ref: /schemas/graph.yaml#/properties/endpoint
>>>> +        description: Output to the primary display pipeline
>>>> +
>>>> +      endpoint@1:
>>>> +        $ref: /schemas/graph.yaml#/properties/endpoint
>>>> +        description: Output to the secondary display pipeline
>>>> +
>>>> +      endpoint@2:
>>>> +        $ref: /schemas/graph.yaml#/properties/endpoint
>>>> +        description: Output to the tertiary display pipeline
>>>> +
>>>> +    anyOf:
>>>> +      - required:
>>>> +          - endpoint@0
>>>> +      - required:
>>>> +          - endpoint@1
>>>> +      - required:
>>>> +          - endpoint@2
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - reg
>>
>>

Powered by blists - more mailing lists