[PATCH v5 2/3] dt-bindings: arm: mediatek: mmsys: Add OF graph support for board path

Thu Jun 6 11:17:07 UTC 2024

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 at collabora.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 at kernel.org>
>>>> Reviewed-by: Alexandre Mergnat <amergnat at baylibre.com>
>>>> Tested-by: Alexandre Mergnat <amergnat at baylibre.com>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno at collabora.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 at 1c01a000 {
>>>             mmsys-display-interface = <&dsi0>, <&dsi1>, <&dp_intf0>;
>>> };
>>>
>>> vdosys1: syscon at 1c100000 {
>>>             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 at 0:
>>>> +        $ref: /schemas/graph.yaml#/properties/endpoint
>>>> +        description: Output to the primary display pipeline
>>>> +
>>>> +      endpoint at 1:
>>>> +        $ref: /schemas/graph.yaml#/properties/endpoint
>>>> +        description: Output to the secondary display pipeline
>>>> +
>>>> +      endpoint at 2:
>>>> +        $ref: /schemas/graph.yaml#/properties/endpoint
>>>> +        description: Output to the tertiary display pipeline
>>>> +
>>>> +    anyOf:
>>>> +      - required:
>>>> +          - endpoint at 0
>>>> +      - required:
>>>> +          - endpoint at 1
>>>> +      - required:
>>>> +          - endpoint at 2
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - reg
>>
>>