[PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path
AngeloGioacchino Del Regno
angelogioacchino.delregno at collabora.com
Wed Oct 16 09:23:18 UTC 2024
Il 15/10/24 15:48, Rob Herring ha scritto:
> On Tue, Oct 15, 2024 at 10:32:22AM +0200, AngeloGioacchino Del Regno wrote:
>> Il 14/10/24 19:36, Rob Herring ha scritto:
>>> On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno at collabora.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 at kernel.org>
>>>> Reviewed-by: Alexandre Mergnat <amergnat at baylibre.com>
>>>> Tested-by: Alexandre Mergnat <amergnat at baylibre.com>
>>>> Reviewed-by: CK Hu <ck.hu at mediatek.com>
>>>> Tested-by: Michael Walle <mwalle at kernel.org> # on kontron-sbc-i1200
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno at collabora.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 at 0:
>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>> + description: AAL input port
>>>> +
>>>> + port at 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 at 0
>>>> + - port at 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 at 0 {
>>>> + reg = <0>;
>>>> + aal0_in: endpoint {
>>>> + remote-endpoint = <&ccorr0_out>;
>>>> + };
>>>> + };
>>>> +
>>>> + port at 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 at 0:
>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>> + description: CCORR input port
>>>> +
>>>> + port at 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 at 0
>>>> + - port at 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 at 0:
>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>> + description: COLOR input port
>>>> +
>>>> + port at 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 at 0
>>>> + - port at 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 at 0:
>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>> + description: DITHER input, usually from a POSTMASK or GAMMA block.
>>>> +
>>>> + port at 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 at 0
>>>> + - port at 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 at 0:
>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>> + description: DPI input port
>>>> +
>>>> + port at 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 at 0'
>>> are treated as the same thing. Since you are adding an input port, the
>>> new port has to be 'port at 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 at 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.
>
> I have not misread it. Then I guess I forgot about it and missed it the
> next time on v3.
>
Okay, that was some misunderstanding then - it's fine, no problem.
>> 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 at 1 is present: if it is, then it follows this scheme with
>> port at 0 as INPUT and port at 1 as OUTPUT.
>> If the component in port at 0 is an OUTPUT, the bridge attach will fail.
>>
>> Getting to bindings themselves, then... it would be a mistake to model port at 0 as an
>> output and port at 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.
>
> Somewhat confusing, yes. Highly, no. Put a comment in the DT.
>
"Somewhat or highly" boils down to personal opinion, so I still go for "highly"
but won't argue about that as wouldn't be productive and would bring us nowhere
anyway, so, whatever :-)
Putting a comment in DT is an option, yes, but that comment would need to be put
on all of the MediaTek SoC device trees - current and future - and IMO would scream
"inconsistency warning" (in different words, of course) all over, which honestly
doesn't really look clean... at least to my eyes...
>> Please note that the bridge bindings are always declaring port at 0 as an INPUT and
>> other ports as OUTPUT(s).
>
> There is no guarantee on that. Port numbering is local to the bridge and
> opaque to anything outside that bridge. That is why you have to document
> what each port represents.
>
I know and I agree that there's no guarantee on the numbering. I can see that in
other non-drm-bridge bindings, and that's fine.
> Would we have followed that convention if all the ports were defined
> from the start? Certainly. But that didn't happen and you are stuck with
> the existing binding and ABI.
>
I thought about adding a new compatible for the new port scheme, but that looked
even worse to me as, after doing that (yeah, I actually went for it in the first
version that I have never sent upstream) during my own proof-read I started
screaming "HACK! HACK! NOOO!" all over, and rewritten the entire thing.
>> 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.
>
> You can't fix existing software...
>
That's true, but I don't see that as an "excuse" (grant me this term please) to
"carelessly" keep it in a suboptimal state.
This driver has been growing almost uncontrollably with (wrong, anyway!)
board-specific component vectors - and writing a new one would just add up
more code duplication to the mix and/or worsen the maintainability of older
MediaTek SoCs (as the "old" driver will get forgotten and never updated anymore).
> If you want to break the ABI, then that's ultimately up to you and
> Mediatek maintainers to decide0. This case is easy to avoid, why would
> you knowingly break the ABI here.
Because if we don't do this, we condemn (forever) this driver, or part of it
to have an inverted port scheme compared to either:
A. The other drm/mediatek components; or
B. The other bridge drivers (of which, some are used with MTK as well)
...and we also condemn (forever, again) all MediaTek device trees to scream
"port inconsistency warning: A for drm/mediatek components, B for every other
thing", which would scream "drm/mediatek is somewhat broken", which can be
avoided.
> OTOH, this seems like a big enough
> change I would imagine it is a matter of time before supporting a
> missing OF graph for the components will be a problem.
>
Sorry I didn't understand this part, can you please-please-please reword?
Cheers,
Angelo
More information about the dri-devel
mailing list