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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 06/08/24 10:29, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Thu, 2024-07-25 at 11:46 +0200, AngeloGioacchino Del Regno wrote:
>> Il 05/07/24 11:28, CK Hu (胡俊光) ha scritto:
>>> On Tue, 2024-06-11 at 08:54 +0200, AngeloGioacchino Del Regno wrote:
>>>> Il 11/06/24 08:48, CK Hu (胡俊光) ha scritto:
>>>>> On Mon, 2024-06-10 at 10:28 +0200, AngeloGioacchino Del Regno wrote:
>>>>>> Il 06/06/24 07:29, CK Hu (胡俊光) ha scritto:
>>>>>>> Hi, Angelo:
>>>>>>>
>>>>>>> On Wed, 2024-06-05 at 13:15 +0200, AngeloGioacchino Del Regno 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@kernel.org>
>>>>>>>>>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>>>>>>> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@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@1c01a000 {
>>>>>>>>>                mmsys-display-interface = <&dsi0>, <&dsi1>, <&dp_intf0>;
>>>>>>>>> };
>>>>>>>>>       
>>>>>>>>> vdosys1: syscon@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).
>>>>>>>>
>>>>>>>> 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).
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> The alternative solution is using OF graphs to point display interface and use MediaTek specific property to first component:
>>>>>>>
>>>>>>> vdosys0: syscon@1c01a000 {
>>>>>>>               ports {
>>>>>>>                        port@0 {
>>>>>>>                                  endpoint {
>>>>>>>                                           remote-endpoint = <&dsi0_endpoint>;
>>>>>>>                                  };
>>>>>>>                        };
>>>>>>>      
>>>>>>>                        port@1 {
>>>>>>>                                  endpoint {
>>>>>>>                                           remote-endpoint = <&dsi1_endpoint>;
>>>>>>>                                  };
>>>>>>>                        };
>>>>>>>      
>>>>>>>                        port@2 {
>>>>>>>                                  endpoint {
>>>>>>>                                           remote-endpoint = <&dp_intf0_endpoint>;
>>>>>>>                                  };
>>>>>>>                        };
>>>>>>>               };
>>>>>>>      
>>>>>>>               display-first-component = <&ovl0>;
>>>>>>> };
>>>>>>>
>>>>>>> And I agree to it's better to keep only OF graphs property, so it would be
>>>>>>>
>>>>>>> vdosys0: syscon@1c01a000 {
>>>>>>>               ports {
>>>>>>>                        port@0 {
>>>>>>>                                  endpoint {
>>>>>>>                                           remote-endpoint = <&dsi0_endpoint>;
>>>>>>>                       
>>>>>>>                };
>>>>>>>                        };
>>>>>>>      
>>>>>>>                        port@1 {
>>>>>>>                                  endpoint {
>>>>>>>                                           remote-endpoint = <&dsi1_endpoint>;
>>>>>>>                               
>>>>>>>        };
>>>>>>>                        };
>>>>>>>      
>>>>>>>                        port@2 {
>>>>>>>                                  endpoint {
>>>>>>>                                           remote-endpoint = <&dp_intf0_endpoint>;
>>>>>>>                                  }
>>>>>>> ;
>>>>>>>                        };
>>>>>>>               };
>>>>>>> };
>>>>>>>
>>>>>>> Maybe we could use OF graphs for both first component and display interface and drop using MediaTek specific property.
>>>>>>>
>>>>>>
>>>>>> We could, or we can simply walk through the OF Graph in the driver and get the
>>>>>> display interface like that, as it's board-specific ;-)
>>>>>>
>>>>>> ...but anyway, let's see that later: after getting this series upstreamed, I will
>>>>>> convert all MediaTek boards (including Chromebooks) to use the graphs instead, and
>>>>>> you'll see that, at least for the currently supported boards, there's no need for
>>>>>> any custom property.
>>>>>>
>>>>>> Also, setting the DSI0/1/dpintf endpoint to VDO0 is technically wrong, as that is
>>>>>> supposed to be the last one, and a graph is conceptually supposed to go from the
>>>>>> first to the last in sequence.
>>>>>>
>>>>>> *if* we will ever need (probably not) to get the VDO0 node to point directly to
>>>>>> the last node for whatever reason, the right way would be the first one you said,
>>>>>> so, mediatek,mmsys-display-interface = <&dsi0>, <&dsi1>, etc etc
>>>>>>
>>>>>> ...or mediatek,mmsys-possible-displays = < ... phandles >
>>>>>>
>>>>>> ...or anyway, many other solutions are possible - but again, I think this is not
>>>>>> the right time to think about that. Knowing that there are eventual solutions for
>>>>>> any need that might arise in the future is enough, IMO :-)
>>>>>
>>>>> This is one routing of display pipeline and the relation of VDOSYS0 with display pipeline.
>>>>>
>>>>>                   +-- VDOSYS0 ---------------------------------------------+
>>>>>                   |                                                        |
>>>>>                   |                                                        |
>>>>> DRAM -> IOMMU ---> OVL0 -> RDMA0 -> ... -> DSC0 -> MERGE0 -> DP_INTF0 ---->
>>>>>                   |                                                        |
>>>>>                   |                                                        |
>>>>>                   +--------------------------------------------------------+
>>>>>
>>>>> Video data is read by IOMMU from DRAM and send to display pipeline. Then video data travel through first component to display interface.
>>>>> VDOSYS0 manage each component in the pipeline include first component and display interface.
>>>>> The management include clock gating, reset, video data input/output routing.
>>>>> The relationship of VDOSYTS0 with first component is the same as the relationship of VDOSYS0 with display interface.
>>>>> If VDOSYS0 is not suitable using OF graph point to display interface, VDOSYS0 is also not suitable using OF graph point to first component.
>>>>
>>>> In the cases in which VDO goes directly to the display, it *is* possible to make it
>>>> point directly to the display.
>>>>
>>>> In the cases in which the pipeline is larger, VDO still points to the display, but
>>>> only later in the pipeline.
>>
>> Sorry I have just noticed your reply while looking for the status of this series.
>>
>>> I mean VDOSYS0 is not suitable 'using OF graph' to point to both display interface and first component.
>>
>> I seriously don't get why you're saying that VDOSYS0 is not suitable for OF Graphs
>> and I'm sorry but I suspect that the reason is that you don't understand the
>> concept of what a graph defines, other than how can it be walked through by design.
>>
>>> So VDOSYS0 should use specific property to point to both display interface and first component.
>>> Maybe
>>>
>>> vdosys0 {
>>> 	dma-device = <ovl0>;
>>> 	display-interface = <&dsi0>, <&dsi1>, <&dp_intf0>;
>>> };
>>
>> What you just wrote here adds custom properties for no reason - as in, there is
>> no reason for vdosys0 to have two properties pointing one to the first component
>> and one to all of the possible display interfaces for vdosys0.
> 
> vdosys has management relationship with these display device.
> The hardware relationship always exist even though I have no software reason.
> 
>> Provided a graph, that graph does express the OVL0 relationship with VDO0, and
>> it does express the relationship between OVL0 and the final display interface
>> -> through expressing the relationship between OVL0 and all of the middle
>>      components until reaching the actual display interface. <-
>>
>> Anyway, the proposed snippet either:
>>    a. Invalidates the point of this series entirely, as in, graphs in this case are
>>       implemented in order to stop hardcoding display paths for each board into the
>>       driver; or
>>    b. It is exactly the same as a graph, except with different properties and without
>>       ports and endpoints.
>>
>> Moreover, there is no advantage in setting all of the possible display interfaces
>> that are connectable to VDOSYS0 in a display-interface property:
>> from a board-specific perspective, the board cares only about the interfaces that
>> are *available to that board*, and not about any other.
> 
> I think in the board dts, the property could be overwritten.
> So the display interface list could be changed.
> 
>>
>> If interfaces X and Y are available to a board, that board will have a graph for X
>> and a graph for Y, so they are both perfectly described with ... graphs!
>>
>> ...and even though the VDO0 (or the SoC, whatever) supports interface Z, if said
>> display interface is *not* present on the board, that interface will not be
>> described by any graph, because it does not pertain to that board, it's unused and
>> it's useless to describe (even though it would be possible to add it regardless of
>> whether it's usable or not on that board).
>>
>> Last thing - I don't know if you have this doubt or not, but for the sake of making
>> the stream of information complete: even dual-dsi displays can be described with a
>> graph without any issue.
> 
> Here is my thought to make me accept this patch (I'm not sure you agree or not)
> dma-device and display-interface is used to point some specific device related to vdosys.
> OF graph is used to point to the display pipeline.
> Each has different meaning so each could co-exist.

Yes, that is correct, and I agree.

> According to this thought, this patch is
> 
> Reviewed-by: CK Hu <ck.hu@mediatek.com>

Thanks for that.

I hope we can get this picked for v6.12 (very likely to be the next LTS) :-)

Cheers,
Angelo.

> 
>>
>>
>> Regards,
>> Angelo
>>
>>>
>>> Regards,
>>> CK
>>>
>>>>
>>>>> The job of the component in display pipeline is to process the video data,
>>>>> but the job of VDOSYS0 is to manage (clock gating, reset, routing) the pipeline.
>>>>> If the OF graph is to show the video data travel path, VDOSYS0 should not exist in the OF graph.
>>>>>
>>>>> Regards,
>>>>> CK
>>>>>
>>>>>>
>>>>>> Cheers,
>>>>>> Angelo
>>>>>>
>>>>>>> Regards,
>>>>>>> CK
>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>
>>