lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <67594e3abfb0a17c2f7df80003c88a002842491e.camel@mediatek.com>
Date: Tue, 6 Aug 2024 08:29:34 +0000
From: CK Hu (胡俊光) <ck.hu@...iatek.com>
To: "angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>, "chunkuang.hu@...nel.org"
	<chunkuang.hu@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"wenst@...omium.org" <wenst@...omium.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

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@...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).
> > > > > > > 
> > > > > > > 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@...1a000 {
> > > > > >              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@...1a000 {
> > > > > >              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.
According to this thought, this patch is

Reviewed-by: CK Hu <ck.hu@...iatek.com>

> 
> 
> 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
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> > > 
> 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ