| 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
| ||
|
Message-ID: <ec14b01e-7237-4f52-82a6-b8de42fb120b@collabora.com>
Date: Tue, 15 Oct 2024 10:32:22 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Rob Herring <robh@...nel.org>
Cc: chunkuang.hu@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, p.zabel@...gutronix.de, airlied@...il.com,
daniel@...ll.ch, maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
tzimmermann@...e.de, matthias.bgg@...il.com, shawn.sung@...iatek.com,
yu-chang.lee@...iatek.com, ck.hu@...iatek.com, jitao.shi@...iatek.com,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-mediatek@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, wenst@...omium.org,
kernel@...labora.com, sui.jingfeng@...ux.dev, michael@...le.cc,
sjoerd@...labora.com, Alexandre Mergnat <amergnat@...libre.com>,
Michael Walle <mwalle@...nel.org>
Subject: Re: [PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph
support for board path
Il 14/10/24 19:36, Rob Herring ha scritto:
> On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.com> wrote:
>>
>> The display IPs in MediaTek SoCs support being interconnected with
>> different instances of DDP IPs (for example, merge0 or merge1) and/or
>> with different DDP IPs (for example, rdma can be connected with either
>> color, dpi, dsi, merge, etc), forming a full Display Data Path that
>> ends with an actual display.
>>
>> The final display pipeline is effectively board specific, as it does
>> depend on the display that is attached to it, and eventually on the
>> sensors supported by the board (for example, Adaptive Ambient Light
>> would need an Ambient Light Sensor, otherwise it's pointless!), other
>> than the output type.
>>
>> Add support for OF graphs to most of the MediaTek DDP (display) bindings
>> to add flexibility to build custom hardware paths, hence enabling board
>> specific configuration of the display pipeline and allowing to finally
>> migrate away from using hardcoded paths.
>>
>> Reviewed-by: Rob Herring (Arm) <robh@...nel.org>
>> Reviewed-by: Alexandre Mergnat <amergnat@...libre.com>
>> Tested-by: Alexandre Mergnat <amergnat@...libre.com>
>> Reviewed-by: CK Hu <ck.hu@...iatek.com>
>> Tested-by: Michael Walle <mwalle@...nel.org> # on kontron-sbc-i1200
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> ---
>> .../display/mediatek/mediatek,aal.yaml | 40 +++++++++++++++++++
>> .../display/mediatek/mediatek,ccorr.yaml | 21 ++++++++++
>> .../display/mediatek/mediatek,color.yaml | 22 ++++++++++
>> .../display/mediatek/mediatek,dither.yaml | 22 ++++++++++
>> .../display/mediatek/mediatek,dpi.yaml | 25 +++++++++++-
>> .../display/mediatek/mediatek,dsc.yaml | 24 +++++++++++
>> .../display/mediatek/mediatek,dsi.yaml | 27 ++++++++++++-
>> .../display/mediatek/mediatek,ethdr.yaml | 22 ++++++++++
>> .../display/mediatek/mediatek,gamma.yaml | 19 +++++++++
>> .../display/mediatek/mediatek,merge.yaml | 23 +++++++++++
>> .../display/mediatek/mediatek,od.yaml | 22 ++++++++++
>> .../display/mediatek/mediatek,ovl-2l.yaml | 22 ++++++++++
>> .../display/mediatek/mediatek,ovl.yaml | 22 ++++++++++
>> .../display/mediatek/mediatek,postmask.yaml | 21 ++++++++++
>> .../display/mediatek/mediatek,rdma.yaml | 22 ++++++++++
>> .../display/mediatek/mediatek,ufoe.yaml | 21 ++++++++++
>> 16 files changed, 372 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>> index cf24434854ff..47ddba5c41af 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>> @@ -62,6 +62,27 @@ properties:
>> $ref: /schemas/types.yaml#/definitions/phandle-array
>> maxItems: 1
>>
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> + description:
>> + Input and output ports can have multiple endpoints, each of those
>> + connects to either the primary, secondary, etc, display pipeline.
>> +
>> + properties:
>> + port@0:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description: AAL input port
>> +
>> + port@1:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description:
>> + AAL output to the next component's input, for example could be one
>> + of many gamma, overdrive or other blocks.
>> +
>> + required:
>> + - port@0
>> + - port@1
>> +
>> required:
>> - compatible
>> - reg
>> @@ -89,5 +110,24 @@ examples:
>> power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
>> clocks = <&mmsys CLK_MM_DISP_AAL>;
>> mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + port@0 {
>> + reg = <0>;
>> + aal0_in: endpoint {
>> + remote-endpoint = <&ccorr0_out>;
>> + };
>> + };
>> +
>> + port@1 {
>> + reg = <1>;
>> + aal0_out: endpoint {
>> + remote-endpoint = <&gamma0_in>;
>> + };
>> + };
>> + };
>> };
>> };
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>> index 9f8366763831..fca8e7bb0cbc 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>> @@ -57,6 +57,27 @@ properties:
>> $ref: /schemas/types.yaml#/definitions/phandle-array
>> maxItems: 1
>>
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> + description:
>> + Input and output ports can have multiple endpoints, each of those
>> + connects to either the primary, secondary, etc, display pipeline.
>> +
>> + properties:
>> + port@0:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description: CCORR input port
>> +
>> + port@1:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description:
>> + CCORR output to the input of the next desired component in the
>> + display pipeline, usually only one of the available AAL blocks.
>> +
>> + required:
>> + - port@0
>> + - port@1
>> +
>> required:
>> - compatible
>> - reg
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>> index 7df786bbad20..6160439ce4d7 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>> @@ -65,6 +65,28 @@ properties:
>> $ref: /schemas/types.yaml#/definitions/phandle-array
>> maxItems: 1
>>
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> + description:
>> + Input and output ports can have multiple endpoints, each of those
>> + connects to either the primary, secondary, etc, display pipeline.
>> +
>> + properties:
>> + port@0:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description: COLOR input port
>> +
>> + port@1:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description:
>> + COLOR output to the input of the next desired component in the
>> + display pipeline, for example one of the available CCORR or AAL
>> + blocks.
>> +
>> + required:
>> + - port@0
>> + - port@1
>> +
>> required:
>> - compatible
>> - reg
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>> index 6fceb1f95d2a..abaf27916d13 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>> @@ -56,6 +56,28 @@ properties:
>> $ref: /schemas/types.yaml#/definitions/phandle-array
>> maxItems: 1
>>
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> + description:
>> + Input and output ports can have multiple endpoints, each of those
>> + connects to either the primary, secondary, etc, display pipeline.
>> +
>> + properties:
>> + port@0:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description: DITHER input, usually from a POSTMASK or GAMMA block.
>> +
>> + port@1:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description:
>> + DITHER output to the input of the next desired component in the
>> + display pipeline, for example one of the available DSC compressors,
>> + DP_INTF, DSI, LVDS or others.
>> +
>> + required:
>> + - port@0
>> + - port@1
>> +
>> required:
>> - compatible
>> - reg
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>> index 3a82aec9021c..b567e3d58aa1 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>> @@ -71,13 +71,34 @@ properties:
>> Output port node. This port should be connected to the input port of an
>> attached HDMI, LVDS or DisplayPort encoder chip.
>>
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> +
>> + properties:
>> + port@0:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description: DPI input port
>> +
>> + port@1:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description: DPI output to an HDMI, LVDS or DisplayPort encoder input
>
> This is wrong. The existing 'port' is the output. 'port' and 'port@0'
> are treated as the same thing. Since you are adding an input port, the
> new port has to be 'port@1' (or any number but 0).
>
> I haven't looked at the driver code, but it should request port 0 and
> always get the output port. And requesting port 1 will return an error
> or the input port.
Hello Rob,
I want to remind you that in v2 of this series you said that it'd be wrong for
port@0 to be an output, I replied that you misread that as I had modeled it indeed
as an input, and then you gave me your Reviewed-by tag.
Anyway - I get your concern about the previous behavior of `port`, but I chose to
model this that way purely for consistency.
First of all - the driver(s) will check if we're feeding a full graph, as it will
indeed first check if port@1 is present: if it is, then it follows this scheme with
port@0 as INPUT and port@1 as OUTPUT.
If the component in port@0 is an OUTPUT, the bridge attach will fail.
Getting to bindings themselves, then... it would be a mistake to model port@0 as an
output and port@1 as an input, because that would be not only inconsistent with the
DRM Bridge bindings, but would be highly confusing when reading the devicetree.
Please note that the bridge bindings are always declaring port@0 as an INPUT and
other ports as OUTPUT(s).
As an example, you can check display/bridge/analogix,anx7625.yaml or
display/bridge/samsung,mipi-dsim.yaml (and others) for bridges, otherwise
display/st,stm32mp25-lvds.yaml or display/allwinner,sun4i-a10-display-frontend.yaml
(and others) for display controllers, which do all conform to this logic, where
the input is always @0, and the output is @1.
Of course, doing this required me to do extra changes to the MTK DRM drivers to
actually be retro-compatible with the old devicetrees as I explained before.
Just for clarity, if I were to model this with port@0 OUTPUT and @1 INPUT, we would
see in devicetree something like:
dpi-node@...ewhere {
ports {
some_output_1: port@0 {
remote-endpoint = <&some_input_2>;
};
some_input_1: port@1 {
remote-endpoint = <&some_output_0>;
};
};
/* already existing bridge binding, not touched by this commit */
bridge@...ewhere-else {
ports {
some_input_2: port@0 {
remote-endpoint = <&some_output_1>;
};
some_output_2: port@1 {
remote-endpoint = <&to_display_input>;
};
};
...and I think that you agree with me that this would be at least confusing for
whoever reads the DT (and again, IMO, inconsistent and simply wrong).
Instead, with the model proposed in this commit, we will have consistency:
dpi-node@...ewhere {
ports {
some_input_1: port@1 {
remote-endpoint = <&some_output_0>;
};
some_output_1: port@0 {
remote-endpoint = <&some_input_2>;
};
};
/* already existing bridge binding, not touched by this commit */
bridge@...ewhere-else {
ports {
some_input_2: port@0 {
remote-endpoint = <&some_output_1>;
};
some_output_2: port@1 {
remote-endpoint = <&to_display_input>;
};
};
...still, again, all this while still supporting the old device trees (which I plan
to update as soon as possible anyway, so that they're all using the full graph
instead of hardcoding board specific paths in the drivers).
Does this clarify to you the reasons why this was done like that?
If you have any other questions, I will be happy to clarify.
Cheers,
Angelo
Powered by blists - more mailing lists