| Message ID | 20230331091145.737305-5-treapking@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Register Type-C mode-switch in DP bridge endpoints | expand |
On Fri, Mar 31, 2023 at 05:11:39PM +0800, Pin-yen Lin wrote: > Analogix 7625 can be used in systems to switch the DP traffic between > two downstreams, which can be USB Type-C DisplayPort alternate mode > lane or regular DisplayPort output ports. > > Update the binding to accommodate this usage by introducing a > data-lanes and a mode-switch property on endpoints. > > Also include the link to the product brief in the bindings. > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > Tested-by: Chen-Yu Tsai <wenst@chromium.org> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > --- > > (no changes since v12) > > Changes in v12: > - Removed the 4-lane binding in analogix,anx7625.yaml > - Reworded the description for the mode-switch property > > Changes in v11: > - Updated the description of the endpoints > - Referenced video-interfaces.yaml instead for the endpoints > > Changes in v10: > - Collected Reviewed-by and Tested-by tags > > Changes in v9: > - Collected Reviewed-by tag > > Changes in v8: > - Updated anx7625 bindings for data-lane property > - Fixed the subject prefix > > Changes in v7: > - Fixed issues reported by dt_binding_check > - Updated the schema and the example dts for data-lanes. > - Changed to generic naming for the example dts node. > > Changes in v6: > - Remove switches node and use endpoints and data-lanes property to > describe the connections. > > .../display/bridge/analogix,anx7625.yaml | 88 ++++++++++++++++++- > 1 file changed, 85 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > index b42553ac505c..604c7391d74f 100644 > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > @@ -12,7 +12,8 @@ maintainers: > > description: | > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter > - designed for portable devices. > + designed for portable devices. Product brief is available at > + https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf > > properties: > compatible: > @@ -112,9 +113,40 @@ properties: > data-lanes: true > > port@1: > - $ref: /schemas/graph.yaml#/properties/port > + $ref: /schemas/graph.yaml#/$defs/port-base > description: > - Video port for panel or connector. > + Video port for panel or connector. Each endpoint connects to a video > + output downstream, and the "data-lanes" property is used to describe > + the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, SSTX1, > + SSRX2, SSTX2, respectively. > + > + patternProperties: > + "^endpoint@[01]$": > + $ref: /schemas/media/video-interfaces.yaml# unevaluatedProperties: false > + properties: > + reg: true > + > + remote-endpoint: true Don't need to list these 2. > + > + data-lanes: > + oneOf: > + - items: > + - enum: [0, 1, 2, 3] > + > + - items: > + - const: 0 > + - const: 1 > + > + - items: > + - const: 2 > + - const: 3 > + > + mode-switch: > + type: boolean > + description: Serves as Type-C mode switch if present. > + > + required: > + - remote-endpoint Don't need required. > > required: > - port@0 > @@ -186,3 +218,53 @@ examples: > }; > }; > }; > + - | > + i2c3 { i2c-3 But better yet, just expand the existing example rather than a whole new one. > + #address-cells = <1>; > + #size-cells = <0>; > + > + encoder@58 { > + compatible = "analogix,anx7625"; > + reg = <0x58>; > + pinctrl-names = "default"; > + pinctrl-0 = <&anx7625_dp_pins>; > + enable-gpios = <&pio 176 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&pio 177 GPIO_ACTIVE_HIGH>; > + vdd10-supply = <&pp1100_dpbrdg>; > + vdd18-supply = <&pp1800_dpbrdg_dx>; > + vdd33-supply = <&pp3300_dpbrdg_dx>; > + analogix,audio-enable; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + anx7625_dp_in: endpoint { > + bus-type = <7>; > + remote-endpoint = <&dpi_out>; > + }; > + }; > + > + port@1 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + reg = <1>; > + anx_typec0: endpoint@0 { > + reg = <0>; > + mode-switch; > + data-lanes = <0 1>; > + remote-endpoint = <&typec_port0>; > + }; > + anx_typec1: endpoint@1 { > + reg = <1>; > + mode-switch; > + data-lanes = <2 3>; > + remote-endpoint = <&typec_port1>; > + }; > + }; > + }; > + }; > + }; > -- > 2.40.0.348.gf938b09366-goog >
Quoting Pin-yen Lin (2023-03-31 02:11:39) > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > index b42553ac505c..604c7391d74f 100644 > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > @@ -12,7 +12,8 @@ maintainers: > > description: | > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter > - designed for portable devices. > + designed for portable devices. Product brief is available at > + https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf > > properties: > compatible: > @@ -112,9 +113,40 @@ properties: > data-lanes: true > > port@1: > - $ref: /schemas/graph.yaml#/properties/port > + $ref: /schemas/graph.yaml#/$defs/port-base > description: > - Video port for panel or connector. > + Video port for panel or connector. Each endpoint connects to a video > + output downstream, and the "data-lanes" property is used to describe > + the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, SSTX1, > + SSRX2, SSTX2, respectively. > + > + patternProperties: > + "^endpoint@[01]$": > + $ref: /schemas/media/video-interfaces.yaml# > + properties: > + reg: true > + > + remote-endpoint: true > + > + data-lanes: > + oneOf: > + - items: > + - enum: [0, 1, 2, 3] > + > + - items: > + - const: 0 > + - const: 1 > + > + - items: > + - const: 2 > + - const: 3 > + > + mode-switch: Is it possible to not have this property? Can we have the driver for this anx device look at the remote-endpoint and if it sees that it is not a drm_bridge or panel on the other end, or a DP connector, that it should register a typec mode switch (or two depending on the number of endpoints in port@1)? Is there any case where that doesn't hold true? I see these possible scenarios: 1. DPI to DP bridge steering DP to one of two usb-c-connectors In this case, endpoint@0 is connected to one usb-c-connector and endpoint@1 is connected to another usb-c-connector. The input endpoint is only connected to DPI. The USB endpoint is not present (although I don't see this described in the binding either, so we would need a port@2, entirely optional to describe USB3 input). The driver will register two mode switches. 2. DPI to DP bridge with USB3 to one usb-c-connector In this case, endpoint@1 doesn't exist. The SSTX1/2 and SSRX1/2 pins are all connected to a usb-c-connector node. The input ports (0 and 2) are connected to both DPI and USB. The device acts as both a mode-switch and an orientation-switch. It registers both switches. I wonder if there is any benefit to describing SBU connections or CC connections? Maybe we don't register the orientation-switch if the SBU or CC connection isn't described? 3. DPI to DP bridge connected to eDP panel In this case, endpoint@1 doesn't exist. The USB endpoint is not present (port@2). Depending on how the crosspoint should be configured, we'll need to use data-lanes in the port@1 endpoint to describe which SSTRX pair to use (1 or 2). Or we'll have to use the endpoint's reg property to describe which pair to drive DP on. Presumably the default configuration is SSRX2/SSTX2 providing 2 lanes of DP to an eDP panel. The endpoint@0 in port@1 will be connected to a drm_panel, and the driver will be able to detect this properly by checking for the existence of an aux-bus node or the return value of of_dp_aux_populate_bus(). 4. DPI to DP bridge connected to DP connector This is similar to the eDP panel scenario #3. In this case, endpoint@1 doesn't exist. The USB endpoint is not present (port@2). Same story about port@1 and lane configuration, but we don't have an aux-bus node. In this case, the drivers/gpu/drm/bridge/display-connector.c driver will probe for the dp-connector node and add a drm_bridge. This anx driver will similarly add a drm_bridge, but it needs to look at the node connected on port@1:endpoint@0 with drm_of_get_bridge() and check if it is a drm_bridge (DP connector) or if it is some type-c thing (connector or orientation-switch). I think having this mode-switch property here lets us avoid calling drm_of_get_bridge() unconditionally in anx7625_parse_dt(). drm_of_get_bridge() will always return -EPROBE_DEFER when this is the last drm_bridge in the chain and the other side of the endpoint is a type-c thing (scenarios #1 and #2). Maybe we should teach drm_of_get_bridge() that a drm_bridge might be connected to a type-c device and have it not return -EPROBE_DEFER in that case. Or make some new API like drm_of_get_bridge_typec() that checks if the typec framework knows about the endpoint in question (as either a typec switch or a connector) and returns a NULL bridge pointer. If we had that then I think this property is not necessary. Hopefully the usb-c-connector can always be registered with the typec framework? I'm worried that the driver that registers the usb-c-connector node may want to form a struct typec_port with typec_register_port() and that will get stuck in a similar -EPROBE_DEFER loop waiting for this mode-switch to appear. So having this property also avoids that problem by telling typec framework to wait until this driver can register a mode-switch. TL;DR: Is this mode-switch property a workaround for probe defer? Can we figure out where the mode switch is in software and not have the property in DT? If we can it would certainly improve things because forgetting to add the property can lead to broken behavior, and we don't do anything like this for chains of drm_bridge devices. We just describe the display chain and let the kernel figure out which bridge should handle hpd, edid reading, or mode detection, etc.
Hi Stephen, On Wed, Apr 12, 2023 at 10:38 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Pin-yen Lin (2023-03-31 02:11:39) > > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > index b42553ac505c..604c7391d74f 100644 > > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > @@ -12,7 +12,8 @@ maintainers: > > > > description: | > > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter > > - designed for portable devices. > > + designed for portable devices. Product brief is available at > > + https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf > > > > properties: > > compatible: > > @@ -112,9 +113,40 @@ properties: > > data-lanes: true > > > > port@1: > > - $ref: /schemas/graph.yaml#/properties/port > > + $ref: /schemas/graph.yaml#/$defs/port-base > > description: > > - Video port for panel or connector. > > + Video port for panel or connector. Each endpoint connects to a video > > + output downstream, and the "data-lanes" property is used to describe > > + the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, SSTX1, > > + SSRX2, SSTX2, respectively. > > + > > + patternProperties: > > + "^endpoint@[01]$": > > + $ref: /schemas/media/video-interfaces.yaml# > > + properties: > > + reg: true > > + > > + remote-endpoint: true > > + > > + data-lanes: > > + oneOf: > > + - items: > > + - enum: [0, 1, 2, 3] > > + > > + - items: > > + - const: 0 > > + - const: 1 > > + > > + - items: > > + - const: 2 > > + - const: 3 > > + > > + mode-switch: > > Is it possible to not have this property? Can we have the driver for > this anx device look at the remote-endpoint and if it sees that it is > not a drm_bridge or panel on the other end, or a DP connector, that it > should register a typec mode switch (or two depending on the number of > endpoints in port@1)? Is there any case where that doesn't hold true? > > I see these possible scenarios: > > 1. DPI to DP bridge steering DP to one of two usb-c-connectors > > In this case, endpoint@0 is connected to one usb-c-connector and > endpoint@1 is connected to another usb-c-connector. The input endpoint > is only connected to DPI. The USB endpoint is not present (although I > don't see this described in the binding either, so we would need a > port@2, entirely optional to describe USB3 input). The driver will > register two mode switches. > > 2. DPI to DP bridge with USB3 to one usb-c-connector > > In this case, endpoint@1 doesn't exist. The SSTX1/2 and SSRX1/2 pins are > all connected to a usb-c-connector node. The input ports (0 and 2) are > connected to both DPI and USB. The device acts as both a mode-switch and > an orientation-switch. It registers both switches. I wonder if there is > any benefit to describing SBU connections or CC connections? Maybe we > don't register the orientation-switch if the SBU or CC connection isn't > described? > > 3. DPI to DP bridge connected to eDP panel > > In this case, endpoint@1 doesn't exist. The USB endpoint is not present > (port@2). Depending on how the crosspoint should be configured, we'll > need to use data-lanes in the port@1 endpoint to describe which SSTRX > pair to use (1 or 2). Or we'll have to use the endpoint's reg property > to describe which pair to drive DP on. Presumably the default > configuration is SSRX2/SSTX2 providing 2 lanes of DP to an eDP panel. > The endpoint@0 in port@1 will be connected to a drm_panel, and the > driver will be able to detect this properly by checking for the > existence of an aux-bus node or the return value of > of_dp_aux_populate_bus(). Can we assume that the eDP panel always stays behind an `aux-bus` node? Can't the panel be connected to the bridge directly in the graph? Though this might not matter if we only register mode switches when there are usb-c-connectors connected. > > 4. DPI to DP bridge connected to DP connector > > This is similar to the eDP panel scenario #3. In this case, endpoint@1 > doesn't exist. The USB endpoint is not present (port@2). Same story > about port@1 and lane configuration, but we don't have an aux-bus node. > In this case, the drivers/gpu/drm/bridge/display-connector.c driver will > probe for the dp-connector node and add a drm_bridge. This anx driver > will similarly add a drm_bridge, but it needs to look at the node > connected on port@1:endpoint@0 with drm_of_get_bridge() and check if it > is a drm_bridge (DP connector) or if it is some type-c thing (connector > or orientation-switch). > > I think having this mode-switch property here lets us avoid calling > drm_of_get_bridge() unconditionally in anx7625_parse_dt(). > drm_of_get_bridge() will always return -EPROBE_DEFER when this is the > last drm_bridge in the chain and the other side of the endpoint is a > type-c thing (scenarios #1 and #2). Maybe we should teach > drm_of_get_bridge() that a drm_bridge might be connected to a type-c > device and have it not return -EPROBE_DEFER in that case. Or make some > new API like drm_of_get_bridge_typec() that checks if the typec > framework knows about the endpoint in question (as either a typec switch > or a connector) and returns a NULL bridge pointer. If we had that then I > think this property is not necessary. > > Hopefully the usb-c-connector can always be registered with the typec > framework? I'm worried that the driver that registers the > usb-c-connector node may want to form a struct typec_port with > typec_register_port() and that will get stuck in a similar -EPROBE_DEFER > loop waiting for this mode-switch to appear. So having this property > also avoids that problem by telling typec framework to wait until this > driver can register a mode-switch. > > TL;DR: Is this mode-switch property a workaround for probe defer? Can we > figure out where the mode switch is in software and not have the > property in DT? If we can it would certainly improve things because > forgetting to add the property can lead to broken behavior, and we don't > do anything like this for chains of drm_bridge devices. We just describe > the display chain and let the kernel figure out which bridge should > handle hpd, edid reading, or mode detection, etc. Actually the `mode-switch` property here is mainly because `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches when the property is present. I am not sure what side effects would be if I remove the ID-matching condition in `typec_mux_match`, so I added the property here. Is it feasible to remove the `mode-switch` property here given the existing implementation of the Type-C framework? [1]: https://elixir.bootlin.com/linux/latest/source/drivers/usb/typec/mux.c#L351 [2]: https://elixir.bootlin.com/linux/latest/source/drivers/usb/typec/mux.c#L290 Best regards, Pin-yen
Quoting Pin-yen Lin (2023-04-13 02:50:44) > Hi Stephen, > > On Wed, Apr 12, 2023 at 10:38 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Pin-yen Lin (2023-03-31 02:11:39) > > > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > index b42553ac505c..604c7391d74f 100644 > > > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > @@ -12,7 +12,8 @@ maintainers: > > > > > > description: | > > > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter > > > - designed for portable devices. > > > + designed for portable devices. Product brief is available at > > > + https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf > > > > > > properties: > > > compatible: > > > @@ -112,9 +113,40 @@ properties: > > > data-lanes: true > > > > > > port@1: > > > - $ref: /schemas/graph.yaml#/properties/port > > > + $ref: /schemas/graph.yaml#/$defs/port-base > > > description: > > > - Video port for panel or connector. > > > + Video port for panel or connector. Each endpoint connects to a video > > > + output downstream, and the "data-lanes" property is used to describe > > > + the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, SSTX1, > > > + SSRX2, SSTX2, respectively. > > > + > > > + patternProperties: > > > + "^endpoint@[01]$": > > > + $ref: /schemas/media/video-interfaces.yaml# > > > + properties: > > > + reg: true > > > + > > > + remote-endpoint: true > > > + > > > + data-lanes: > > > + oneOf: > > > + - items: > > > + - enum: [0, 1, 2, 3] > > > + > > > + - items: > > > + - const: 0 > > > + - const: 1 > > > + > > > + - items: > > > + - const: 2 > > > + - const: 3 > > > + > > > + mode-switch: > > > > Is it possible to not have this property? Can we have the driver for > > this anx device look at the remote-endpoint and if it sees that it is > > not a drm_bridge or panel on the other end, or a DP connector, that it > > should register a typec mode switch (or two depending on the number of > > endpoints in port@1)? Is there any case where that doesn't hold true? > > > > I see these possible scenarios: > > > > 1. DPI to DP bridge steering DP to one of two usb-c-connectors > > > > In this case, endpoint@0 is connected to one usb-c-connector and > > endpoint@1 is connected to another usb-c-connector. The input endpoint > > is only connected to DPI. The USB endpoint is not present (although I > > don't see this described in the binding either, so we would need a > > port@2, entirely optional to describe USB3 input). The driver will > > register two mode switches. > > > > 2. DPI to DP bridge with USB3 to one usb-c-connector > > > > In this case, endpoint@1 doesn't exist. The SSTX1/2 and SSRX1/2 pins are > > all connected to a usb-c-connector node. The input ports (0 and 2) are > > connected to both DPI and USB. The device acts as both a mode-switch and > > an orientation-switch. It registers both switches. I wonder if there is > > any benefit to describing SBU connections or CC connections? Maybe we > > don't register the orientation-switch if the SBU or CC connection isn't > > described? > > > > 3. DPI to DP bridge connected to eDP panel > > > > In this case, endpoint@1 doesn't exist. The USB endpoint is not present > > (port@2). Depending on how the crosspoint should be configured, we'll > > need to use data-lanes in the port@1 endpoint to describe which SSTRX > > pair to use (1 or 2). Or we'll have to use the endpoint's reg property > > to describe which pair to drive DP on. Presumably the default > > configuration is SSRX2/SSTX2 providing 2 lanes of DP to an eDP panel. > > The endpoint@0 in port@1 will be connected to a drm_panel, and the > > driver will be able to detect this properly by checking for the > > existence of an aux-bus node or the return value of > > of_dp_aux_populate_bus(). > > Can we assume that the eDP panel always stays behind an `aux-bus` > node? Can't the panel be connected to the bridge directly in the > graph? Though this might not matter if we only register mode switches > when there are usb-c-connectors connected. The panel is connected to the bridge in the graph. I think we should assume the eDP panel is on an aux-bus. Maybe another scenario is a design that has a DP to HDMI bridge wired down on the device? In which case the output port would be connected to the HDMI bridge. > > > > 4. DPI to DP bridge connected to DP connector > > > > This is similar to the eDP panel scenario #3. In this case, endpoint@1 > > doesn't exist. The USB endpoint is not present (port@2). Same story > > about port@1 and lane configuration, but we don't have an aux-bus node. > > In this case, the drivers/gpu/drm/bridge/display-connector.c driver will > > probe for the dp-connector node and add a drm_bridge. This anx driver > > will similarly add a drm_bridge, but it needs to look at the node > > connected on port@1:endpoint@0 with drm_of_get_bridge() and check if it > > is a drm_bridge (DP connector) or if it is some type-c thing (connector > > or orientation-switch). > > > > I think having this mode-switch property here lets us avoid calling > > drm_of_get_bridge() unconditionally in anx7625_parse_dt(). > > drm_of_get_bridge() will always return -EPROBE_DEFER when this is the > > last drm_bridge in the chain and the other side of the endpoint is a > > type-c thing (scenarios #1 and #2). Maybe we should teach > > drm_of_get_bridge() that a drm_bridge might be connected to a type-c > > device and have it not return -EPROBE_DEFER in that case. Or make some > > new API like drm_of_get_bridge_typec() that checks if the typec > > framework knows about the endpoint in question (as either a typec switch > > or a connector) and returns a NULL bridge pointer. If we had that then I > > think this property is not necessary. > > > > Hopefully the usb-c-connector can always be registered with the typec > > framework? I'm worried that the driver that registers the > > usb-c-connector node may want to form a struct typec_port with > > typec_register_port() and that will get stuck in a similar -EPROBE_DEFER > > loop waiting for this mode-switch to appear. So having this property > > also avoids that problem by telling typec framework to wait until this > > driver can register a mode-switch. > > > > TL;DR: Is this mode-switch property a workaround for probe defer? Can we > > figure out where the mode switch is in software and not have the > > property in DT? If we can it would certainly improve things because > > forgetting to add the property can lead to broken behavior, and we don't > > do anything like this for chains of drm_bridge devices. We just describe > > the display chain and let the kernel figure out which bridge should > > handle hpd, edid reading, or mode detection, etc. > > Actually the `mode-switch` property here is mainly because > `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches > when the property is present. I am not sure what side effects would be > if I remove the ID-matching condition in `typec_mux_match`, so I added > the property here. > > Is it feasible to remove the `mode-switch` property here given the > existing implementation of the Type-C framework? Omitting the mode-switch property would require changes to the type-c framework. I'm wondering if we can have this anx driver register mode switches for however many endpoints exist in the output port all the time when the aux-bus node doesn't exist. Then the type-c framework can walk from the usb-c-connector to each connected node looking for a device that is both a drm_bridge and a mode-switch. When it finds that combination, it knows that the mode-switch has been found. This hinges on the idea that a device that would have the mode-switch property is a drm_bridge and would register a mode-switch with the type-c framework. It may be a little complicated though, because we would only register one drm_bridge for the input to this anx device. The type-c walking code would need to look at the graph endpoint, and find the parent device to see if it is a drm_bridge.
Quoting Stephen Boyd (2023-04-13 17:22:46) > Quoting Pin-yen Lin (2023-04-13 02:50:44) > > > > Actually the `mode-switch` property here is mainly because > > `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches > > when the property is present. I am not sure what side effects would be > > if I remove the ID-matching condition in `typec_mux_match`, so I added > > the property here. > > > > Is it feasible to remove the `mode-switch` property here given the > > existing implementation of the Type-C framework? > > Omitting the mode-switch property would require changes to the type-c > framework. > > I'm wondering if we can have this anx driver register mode switches for > however many endpoints exist in the output port all the time when the > aux-bus node doesn't exist. Then the type-c framework can walk from the > usb-c-connector to each connected node looking for a device that is both > a drm_bridge and a mode-switch. When it finds that combination, it knows > that the mode-switch has been found. This hinges on the idea that a > device that would have the mode-switch property is a drm_bridge and > would register a mode-switch with the type-c framework. > > It may be a little complicated though, because we would only register > one drm_bridge for the input to this anx device. The type-c walking code > would need to look at the graph endpoint, and find the parent device to > see if it is a drm_bridge. I've been thinking more about this. I think we should only have the 'mode-switch' property possible when the USB input pins (port@2) are connected and the DPI input pins are connected (port@0). Probably you don't have that case though? In your case, this device should register either one or two drm_bridges that connect to whatever downstream is actually muxing the 2 DP lanes with the USB SS lanes onto the usb-c-connector. If that is the EC for ChromeOS, then the EC should have a binding that accepts some number of input ports for DP. The EC would act as a drm_bridge, or in this case probably two bridges, and also as two type-c switches for each drm_bridge corresponding to the usb-c-connector nodes. When DP is on the cable, the type-c switch/mux would signal to the drm_bridge that the display is 'connected' via DRM_BRIDGE_OP_DETECT and struct drm_bridge_funcs::detect(). Then the drm_bridge in this anx part would implement struct drm_bridge_funcs::atomic_enable() and configure the crosspoint switch the right way depending on the reg property of the output node in port@1. Because you don't have the part that implements the orientation-switch, you don't need to implement the code for it. I think simply adding support in the binding for mode-switch and orientation-switch if this is directly wired to a usb-c-connector should be sufficient. Those properties would be at the top-level and not part of the graph binding.
On Thu, Apr 20, 2023 at 2:10 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Stephen Boyd (2023-04-13 17:22:46) > > Quoting Pin-yen Lin (2023-04-13 02:50:44) > > > > > > Actually the `mode-switch` property here is mainly because > > > `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches > > > when the property is present. I am not sure what side effects would be > > > if I remove the ID-matching condition in `typec_mux_match`, so I added > > > the property here. > > > > > > Is it feasible to remove the `mode-switch` property here given the > > > existing implementation of the Type-C framework? > > > > Omitting the mode-switch property would require changes to the type-c > > framework. > > > > I'm wondering if we can have this anx driver register mode switches for > > however many endpoints exist in the output port all the time when the > > aux-bus node doesn't exist. Then the type-c framework can walk from the > > usb-c-connector to each connected node looking for a device that is both > > a drm_bridge and a mode-switch. When it finds that combination, it knows > > that the mode-switch has been found. This hinges on the idea that a > > device that would have the mode-switch property is a drm_bridge and > > would register a mode-switch with the type-c framework. > > > > It may be a little complicated though, because we would only register > > one drm_bridge for the input to this anx device. The type-c walking code > > would need to look at the graph endpoint, and find the parent device to > > see if it is a drm_bridge. > > I've been thinking more about this. I think we should only have the > 'mode-switch' property possible when the USB input pins (port@2) are > connected and the DPI input pins are connected (port@0). Probably you > don't have that case though? No we don't have the use case that uses the USB input pins on anx7625. > > In your case, this device should register either one or two drm_bridges > that connect to whatever downstream is actually muxing the 2 DP lanes > with the USB SS lanes onto the usb-c-connector. What do you mean by "muxing the 2 DP lanes with the USB SS lanes''? In our use case, the USB data lanes from both ports are connected to a USB hub, but the DP lanes are muxed by the crosspoint switch on anx7625. HPD and AUX for the external display are muxed by the EC. You can find the diagram at https://lore.kernel.org/linux-usb/YxGzk6DNAt0aCvIY@chromium.org/ > If that is the EC for > ChromeOS, then the EC should have a binding that accepts some number of > input ports for DP. The EC would act as a drm_bridge, or in this case > probably two bridges, and also as two type-c switches for each > drm_bridge corresponding to the usb-c-connector nodes. When DP is on the > cable, the type-c switch/mux would signal to the drm_bridge that the > display is 'connected' via DRM_BRIDGE_OP_DETECT and struct > drm_bridge_funcs::detect(). Then the drm_bridge in this anx part would > implement struct drm_bridge_funcs::atomic_enable() and configure the > crosspoint switch the right way depending on the reg property of the > output node in port@1. So there will be two drm bridges that act as the downstreams for anx7625, and we find the downstream with connector_status_connected to configure the crosspoint switch? How do we support that kind of topology given that the drm bridge chain is currently a list? Are you suggesting making the bridge topology to a tree, or maintaining the two downstreams inside the anx7625 driver and not attaching them to the bridge chain? Also, if we still register mode switches on the two downstream bridges, why do you prefer that over the original approach that register switches in the anx7625 driver? > > Because you don't have the part that implements the orientation-switch, > you don't need to implement the code for it. I think simply adding > support in the binding for mode-switch and orientation-switch if this is > directly wired to a usb-c-connector should be sufficient. Those > properties would be at the top-level and not part of the graph binding.
On Thu, Apr 20, 2023 at 5:10 PM Pin-yen Lin <treapking@chromium.org> wrote: > > On Thu, Apr 20, 2023 at 2:10 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Stephen Boyd (2023-04-13 17:22:46) > > > Quoting Pin-yen Lin (2023-04-13 02:50:44) > > > > > > > > Actually the `mode-switch` property here is mainly because > > > > `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches > > > > when the property is present. I am not sure what side effects would be > > > > if I remove the ID-matching condition in `typec_mux_match`, so I added > > > > the property here. > > > > > > > > Is it feasible to remove the `mode-switch` property here given the > > > > existing implementation of the Type-C framework? > > > > > > Omitting the mode-switch property would require changes to the type-c > > > framework. > > > > > > I'm wondering if we can have this anx driver register mode switches for > > > however many endpoints exist in the output port all the time when the > > > aux-bus node doesn't exist. Then the type-c framework can walk from the > > > usb-c-connector to each connected node looking for a device that is both > > > a drm_bridge and a mode-switch. When it finds that combination, it knows > > > that the mode-switch has been found. This hinges on the idea that a > > > device that would have the mode-switch property is a drm_bridge and > > > would register a mode-switch with the type-c framework. I spent some time working on this approach on the Type-C side. The issue I met is that the driver doesn't know whether a node is a drm_bridge before the anx7625 driver probes. When there is a "mode-switch" property in the node, the Type-C framework knows that "here is a mode switch, but the corresponding driver hasn't registered the typec_mux". So it returns -EPROBE_DEFER and retries later. However, if we remove the property, the Type-C framework won't know whether a node will be registered as a drm_bridge and register a typec_mux. Do you have other suggestions on this if we want to choose this approach? Best regards, Pin-yen > > > > > > It may be a little complicated though, because we would only register > > > one drm_bridge for the input to this anx device. The type-c walking code > > > would need to look at the graph endpoint, and find the parent device to > > > see if it is a drm_bridge. > > > > I've been thinking more about this. I think we should only have the > > 'mode-switch' property possible when the USB input pins (port@2) are > > connected and the DPI input pins are connected (port@0). Probably you > > don't have that case though? > > No we don't have the use case that uses the USB input pins on anx7625. > > > > In your case, this device should register either one or two drm_bridges > > that connect to whatever downstream is actually muxing the 2 DP lanes > > with the USB SS lanes onto the usb-c-connector. > > What do you mean by "muxing the 2 DP lanes with the USB SS lanes''? In > our use case, the USB data lanes from both ports are connected to a > USB hub, but the DP lanes are muxed by the crosspoint switch on > anx7625. HPD and AUX for the external display are muxed by the EC. You > can find the diagram at > https://lore.kernel.org/linux-usb/YxGzk6DNAt0aCvIY@chromium.org/ > > > If that is the EC for > > ChromeOS, then the EC should have a binding that accepts some number of > > input ports for DP. The EC would act as a drm_bridge, or in this case > > probably two bridges, and also as two type-c switches for each > > drm_bridge corresponding to the usb-c-connector nodes. When DP is on the > > cable, the type-c switch/mux would signal to the drm_bridge that the > > display is 'connected' via DRM_BRIDGE_OP_DETECT and struct > > drm_bridge_funcs::detect(). Then the drm_bridge in this anx part would > > implement struct drm_bridge_funcs::atomic_enable() and configure the > > crosspoint switch the right way depending on the reg property of the > > output node in port@1. > > So there will be two drm bridges that act as the downstreams for > anx7625, and we find the downstream with connector_status_connected to > configure the crosspoint switch? How do we support that kind of > topology given that the drm bridge chain is currently a list? Are you > suggesting making the bridge topology to a tree, or maintaining the > two downstreams inside the anx7625 driver and not attaching them to > the bridge chain? > > Also, if we still register mode switches on the two downstream > bridges, why do you prefer that over the original approach that > register switches in the anx7625 driver? > > > > > Because you don't have the part that implements the orientation-switch, > > you don't need to implement the code for it. I think simply adding > > support in the binding for mode-switch and orientation-switch if this is > > directly wired to a usb-c-connector should be sufficient. Those > > properties would be at the top-level and not part of the graph binding.
Quoting Pin-yen Lin (2023-04-20 02:10:46) > On Thu, Apr 20, 2023 at 2:10 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Stephen Boyd (2023-04-13 17:22:46) > > > Quoting Pin-yen Lin (2023-04-13 02:50:44) > > > > > > > > Actually the `mode-switch` property here is mainly because > > > > `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches > > > > when the property is present. I am not sure what side effects would be > > > > if I remove the ID-matching condition in `typec_mux_match`, so I added > > > > the property here. > > > > > > > > Is it feasible to remove the `mode-switch` property here given the > > > > existing implementation of the Type-C framework? > > > > > > Omitting the mode-switch property would require changes to the type-c > > > framework. > > > > > > I'm wondering if we can have this anx driver register mode switches for > > > however many endpoints exist in the output port all the time when the > > > aux-bus node doesn't exist. Then the type-c framework can walk from the > > > usb-c-connector to each connected node looking for a device that is both > > > a drm_bridge and a mode-switch. When it finds that combination, it knows > > > that the mode-switch has been found. This hinges on the idea that a > > > device that would have the mode-switch property is a drm_bridge and > > > would register a mode-switch with the type-c framework. > > > > > > It may be a little complicated though, because we would only register > > > one drm_bridge for the input to this anx device. The type-c walking code > > > would need to look at the graph endpoint, and find the parent device to > > > see if it is a drm_bridge. > > > > I've been thinking more about this. I think we should only have the > > 'mode-switch' property possible when the USB input pins (port@2) are > > connected and the DPI input pins are connected (port@0). Probably you > > don't have that case though? > > No we don't have the use case that uses the USB input pins on anx7625. > > > > In your case, this device should register either one or two drm_bridges > > that connect to whatever downstream is actually muxing the 2 DP lanes > > with the USB SS lanes onto the usb-c-connector. > > What do you mean by "muxing the 2 DP lanes with the USB SS lanes''? In > our use case, the USB data lanes from both ports are connected to a > USB hub, but the DP lanes are muxed by the crosspoint switch on > anx7625. HPD and AUX for the external display are muxed by the EC. You > can find the diagram at > https://lore.kernel.org/linux-usb/YxGzk6DNAt0aCvIY@chromium.org/ I mean that you must have some sort of orientation switch hardware that takes the 2 DP lanes and the 2 USB SuperSpeed "lanes" or "pairs" and puts them all onto a usb-c-connector. The usb-c-connector node can't be connected directly to the anx7625 in your diagram because there must be some sort of "flipper" that does the orientation control. Otherwise the usb-c-connector wouldn't work if the user flipped the cable. Probably this is some TCPC or redriver controlled by the EC. > > > If that is the EC for > > ChromeOS, then the EC should have a binding that accepts some number of > > input ports for DP. The EC would act as a drm_bridge, or in this case > > probably two bridges, and also as two type-c switches for each > > drm_bridge corresponding to the usb-c-connector nodes. When DP is on the > > cable, the type-c switch/mux would signal to the drm_bridge that the > > display is 'connected' via DRM_BRIDGE_OP_DETECT and struct > > drm_bridge_funcs::detect(). Then the drm_bridge in this anx part would > > implement struct drm_bridge_funcs::atomic_enable() and configure the > > crosspoint switch the right way depending on the reg property of the > > output node in port@1. > > So there will be two drm bridges that act as the downstreams for > anx7625, and we find the downstream with connector_status_connected to > configure the crosspoint switch? How do we support that kind of > topology given that the drm bridge chain is currently a list? Are you > suggesting making the bridge topology to a tree, or maintaining the > two downstreams inside the anx7625 driver and not attaching them to > the bridge chain? Good point. I'm suggesting to make the drm bridge chain into a tree. We need to teach drm_bridge core about a mux, and have some logic to navigate atomically switching from one output to another. I was talking with dianders@ and he was suggesting to use bridge attach/detach for this. I'm not sure that will work though because that hook is only called when the encoder is attached to the bridge. It may also turn out that this helps with DP multi-stream transport (MST). As far as I can recall DP MST doesn't mesh well with drm_bridge designs because it wants to operate on a drm_connector and drm_bridge_connector_init() wants to make only one drm_connector for a chain of bridges. If you squint, the anx7625 could be an MST "branch" that only supports one drm_connector being enabled at a time. Maybe that is what we should do here, make drm_bridge support creating more than one drm_connector and when there is a mux in the tree it walks both sides and runs a callback similar to struct drm_dp_mst_topology_cbs::add_connector() to tell the encoder that there's another possible drm_connector here. > > Also, if we still register mode switches on the two downstream > bridges, why do you prefer that over the original approach that > register switches in the anx7625 driver? I prefer to not have a mode-switch property here for a couple reasons: 1. The binding is usb type-c specific, and in the case of the IT6505 part there is nothing that indicates this is a usb type-c piece of hardware. The IT6505 is simply a display bridge. The anx7625 part actually does accept usb signals though, but that isn't being used or described here. That's where my disclaimer about mode-switch making sense applies when the usb input is used. 2. Putting mode-switch into the graph endpoint nodes is awkward. It is a device property, and graph nodes are not devices. Some patches in this series have to work around this fact and special case the graph walking logic to treat the graph itself as a place to look for the property. 3. The mode-switch property probably isn't necessary at all. The DT reviewers have been asking why it is needed. The EC driver that registers the usb-c-connectors can be the mode-switch and the orientation-switch. And in reality, it _is_ both. The DP signals and the USB signals go to the TCPC/redriver that is controlled by the EC and the EC is the device that's doing the mode switching to push DP and USB through the TCPC/redriver out on the right pins of the usb-c-connector. I guess another way to think about it is that the DP signal coming out of the anx7625 part is not "usb type-c" at all, unless the USB signal is coming out on the other side of the crosspoint switch and all four lanes are wired to some usb-c-connector or redriver. Similarly, the situation could look like trogdor, where DP is produced by the DP PHY in the SoC and goes through an analog mux to steer DP to one or the other TCPC that's wired to the usb-c-connector. There isn't any driver to control that mux, but if there was it would be a gpio controlled mux that would be a drm_bridge, because there isn't anything type-c about this hardware. And finally, I can see a possibility where the IT6505 is actually wired to two different dp-connector ports. In that situation, there is no type-c involvement, but we would still want to expose that to userspace as two drm_connectors where only one encoder can be attached to them. If we did that with drm_bridge, then anyone could make these sorts of chains with muxes and it would present a sane userspace interface.
+Jagan who worked on a similar design and initiated the thread. Hi Stephen, On Sat, Apr 29, 2023 at 12:47 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Pin-yen Lin (2023-04-20 02:10:46) > > On Thu, Apr 20, 2023 at 2:10 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Stephen Boyd (2023-04-13 17:22:46) > > > > Quoting Pin-yen Lin (2023-04-13 02:50:44) > > > > > > > > > > Actually the `mode-switch` property here is mainly because > > > > > `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches > > > > > when the property is present. I am not sure what side effects would be > > > > > if I remove the ID-matching condition in `typec_mux_match`, so I added > > > > > the property here. > > > > > > > > > > Is it feasible to remove the `mode-switch` property here given the > > > > > existing implementation of the Type-C framework? > > > > > > > > Omitting the mode-switch property would require changes to the type-c > > > > framework. > > > > > > > > I'm wondering if we can have this anx driver register mode switches for > > > > however many endpoints exist in the output port all the time when the > > > > aux-bus node doesn't exist. Then the type-c framework can walk from the > > > > usb-c-connector to each connected node looking for a device that is both > > > > a drm_bridge and a mode-switch. When it finds that combination, it knows > > > > that the mode-switch has been found. This hinges on the idea that a > > > > device that would have the mode-switch property is a drm_bridge and > > > > would register a mode-switch with the type-c framework. > > > > > > > > It may be a little complicated though, because we would only register > > > > one drm_bridge for the input to this anx device. The type-c walking code > > > > would need to look at the graph endpoint, and find the parent device to > > > > see if it is a drm_bridge. > > > > > > I've been thinking more about this. I think we should only have the > > > 'mode-switch' property possible when the USB input pins (port@2) are > > > connected and the DPI input pins are connected (port@0). Probably you > > > don't have that case though? > > > > No we don't have the use case that uses the USB input pins on anx7625. > > > > > > In your case, this device should register either one or two drm_bridges > > > that connect to whatever downstream is actually muxing the 2 DP lanes > > > with the USB SS lanes onto the usb-c-connector. > > > > What do you mean by "muxing the 2 DP lanes with the USB SS lanes''? In > > our use case, the USB data lanes from both ports are connected to a > > USB hub, but the DP lanes are muxed by the crosspoint switch on > > anx7625. HPD and AUX for the external display are muxed by the EC. You > > can find the diagram at > > https://lore.kernel.org/linux-usb/YxGzk6DNAt0aCvIY@chromium.org/ > > I mean that you must have some sort of orientation switch hardware that > takes the 2 DP lanes and the 2 USB SuperSpeed "lanes" or "pairs" and > puts them all onto a usb-c-connector. The usb-c-connector node can't be > connected directly to the anx7625 in your diagram because there must be > some sort of "flipper" that does the orientation control. Otherwise the > usb-c-connector wouldn't work if the user flipped the cable. Probably > this is some TCPC or redriver controlled by the EC. > > > > > > If that is the EC for > > > ChromeOS, then the EC should have a binding that accepts some number of > > > input ports for DP. The EC would act as a drm_bridge, or in this case > > > probably two bridges, and also as two type-c switches for each > > > drm_bridge corresponding to the usb-c-connector nodes. When DP is on the > > > cable, the type-c switch/mux would signal to the drm_bridge that the > > > display is 'connected' via DRM_BRIDGE_OP_DETECT and struct > > > drm_bridge_funcs::detect(). Then the drm_bridge in this anx part would > > > implement struct drm_bridge_funcs::atomic_enable() and configure the > > > crosspoint switch the right way depending on the reg property of the > > > output node in port@1. > > > > So there will be two drm bridges that act as the downstreams for > > anx7625, and we find the downstream with connector_status_connected to > > configure the crosspoint switch? How do we support that kind of > > topology given that the drm bridge chain is currently a list? Are you > > suggesting making the bridge topology to a tree, or maintaining the > > two downstreams inside the anx7625 driver and not attaching them to > > the bridge chain? > > Good point. I'm suggesting to make the drm bridge chain into a tree. We > need to teach drm_bridge core about a mux, and have some logic to > navigate atomically switching from one output to another. I was talking > with dianders@ and he was suggesting to use bridge attach/detach for > this. I'm not sure that will work though because that hook is only > called when the encoder is attached to the bridge. > > It may also turn out that this helps with DP multi-stream transport > (MST). As far as I can recall DP MST doesn't mesh well with drm_bridge > designs because it wants to operate on a drm_connector and > drm_bridge_connector_init() wants to make only one drm_connector for a > chain of bridges. If you squint, the anx7625 could be an MST "branch" > that only supports one drm_connector being enabled at a time. Maybe that > is what we should do here, make drm_bridge support creating more than > one drm_connector and when there is a mux in the tree it walks both > sides and runs a callback similar to struct > drm_dp_mst_topology_cbs::add_connector() to tell the encoder that > there's another possible drm_connector here. I have been surveying the approaches to change the bridge chain in runtime, and I found this thread[1]. Quoting from Daniel: "... exchanging the bridge chain isn't supported, there's no locking for that since it's assumed to be invariant over the lifetime of the drm_device instance. The simplest way to make that happen right now is to have 2 drm_encoder instances, one with the lvds bridge chain, the other with the hdmi bridge chain, and select the right encoder/bridge chain depending upon which output userspace picks. ... I wouldn't try to make bridge chains exchangeable instead, that's headaches - e.g. with dp mst we've also opted for a bunch of fake drm_encoders to model that kind of switching." I'm not sure how we register two encoders properly, though. Do we make the encoder driver check all the downstream bridges and register two drm_encoder when a bridge is acting as a mux? [1]: https://www.spinics.net/lists/dri-devel/msg340511.html > > > > > Also, if we still register mode switches on the two downstream > > bridges, why do you prefer that over the original approach that > > register switches in the anx7625 driver? > > I prefer to not have a mode-switch property here for a couple reasons: > > 1. The binding is usb type-c specific, and in the case of the IT6505 > part there is nothing that indicates this is a usb type-c piece of > hardware. The IT6505 is simply a display bridge. The anx7625 part > actually does accept usb signals though, but that isn't being used or > described here. That's where my disclaimer about mode-switch making > sense applies when the usb input is used. > > 2. Putting mode-switch into the graph endpoint nodes is awkward. It is > a device property, and graph nodes are not devices. Some patches in > this series have to work around this fact and special case the graph > walking logic to treat the graph itself as a place to look for the > property. > > 3. The mode-switch property probably isn't necessary at all. The DT > reviewers have been asking why it is needed. The EC driver that > registers the usb-c-connectors can be the mode-switch and the > orientation-switch. And in reality, it _is_ both. The DP signals and > the USB signals go to the TCPC/redriver that is controlled by the EC > and the EC is the device that's doing the mode switching to push DP and > USB through the TCPC/redriver out on the right pins of the > usb-c-connector. > > I guess another way to think about it is that the DP signal coming out > of the anx7625 part is not "usb type-c" at all, unless the USB signal is > coming out on the other side of the crosspoint switch and all four lanes > are wired to some usb-c-connector or redriver. Similarly, the situation > could look like trogdor, where DP is produced by the DP PHY in the SoC > and goes through an analog mux to steer DP to one or the other TCPC > that's wired to the usb-c-connector. There isn't any driver to control > that mux, but if there was it would be a gpio controlled mux that would > be a drm_bridge, because there isn't anything type-c about this > hardware. > > And finally, I can see a possibility where the IT6505 is actually wired > to two different dp-connector ports. In that situation, there is no > type-c involvement, but we would still want to expose that to userspace > as two drm_connectors where only one encoder can be attached to them. If > we did that with drm_bridge, then anyone could make these sorts of > chains with muxes and it would present a sane userspace interface. Thanks for the detailed explanation. Yes, our use case in this series is not related to the USB-C port from the bridge's perspective. The bridge only cares about the output changes, and it doesn't need to know whether the downstream is a USB-C port or a DP connector. Best regards, Pin-yen
Quoting Pin-yen Lin (2023-05-09 20:41:54) > On Sat, Apr 29, 2023 at 12:47 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Good point. I'm suggesting to make the drm bridge chain into a tree. We > > need to teach drm_bridge core about a mux, and have some logic to > > navigate atomically switching from one output to another. I was talking > > with dianders@ and he was suggesting to use bridge attach/detach for > > this. I'm not sure that will work though because that hook is only > > called when the encoder is attached to the bridge. > > > > It may also turn out that this helps with DP multi-stream transport > > (MST). As far as I can recall DP MST doesn't mesh well with drm_bridge > > designs because it wants to operate on a drm_connector and > > drm_bridge_connector_init() wants to make only one drm_connector for a > > chain of bridges. If you squint, the anx7625 could be an MST "branch" > > that only supports one drm_connector being enabled at a time. Maybe that > > is what we should do here, make drm_bridge support creating more than > > one drm_connector and when there is a mux in the tree it walks both > > sides and runs a callback similar to struct > > drm_dp_mst_topology_cbs::add_connector() to tell the encoder that > > there's another possible drm_connector here. > > I have been surveying the approaches to change the bridge chain in > runtime, and I found this thread[1]. Quoting from Daniel: I find the lore links easier to read. > "... exchanging the bridge chain isn't supported, there's no locking > for that since it's assumed to be invariant over the lifetime of the > drm_device instance. The simplest way to make that happen right now is to > have 2 drm_encoder instances, one with the lvds bridge chain, the other > with the hdmi bridge chain, and select the right encoder/bridge chain > depending upon which output userspace picks. > ... > I wouldn't try to make bridge chains exchangeable instead, that's > headaches - e.g. with dp mst we've also opted for a bunch of fake > drm_encoders to model that kind of switching." > > I'm not sure how we register two encoders properly, though. Do we make > the encoder driver check all the downstream bridges and register two > drm_encoder when a bridge is acting as a mux? I honestly don't know because I'm not a DRM expert. Maybe Jagan or DRM bridge maintainers can add to the thread here. I kept reading the thread[2] and I think they settled on 2 drm_encoders because they're different display formats (LVDS vs. HDMI) and 2 drm_connector structures. But then I watched the youtube video from this thread[3] and it seems like 1 drm_encoder is actually what should be done because there is really only DSI at the root. There's at least three people working on this topic of muxing displays though. Good news? The analogy between their gpio controlled mux and this anx part with a crosspoint switch is that the gpio is like the crosspoint switch, but the gpio is like a virtual mux? If the gpio is asserted for them, one display bridge is enabled (HDMI) and the other one is not (LVDS). In this case, the type-c cables may be connected to both usb-c-connectors and HPD may be asserted on both, but only one drm_connector will be able to attach to the 1 drm_encoder at a time. If we had 2 drm_encoders it would be possible to attach both encoders to both drm_connectors at the same time, which is impossible because it's a mux. Indicating that each connector is connected is OK when HPD is high on both usb-c-connectors. Userspace will have to attach an encoder to the drm_connector it wants to use, and the drm_connector will indicate which drm_encoders are possible for it, so all the information will be provided to userspace in this design. I think it really comes down to implementing the tree walking logic in the drm bridge APIs. The problem is things like drm_bridge_get_next_bridge(), drm_bridge_get_prev_bridge(), and drm_for_each_bridge_in_chain() which will have to operate on a tree instead of a list. There's also drm_bridge_chain_get_first_bridge() and drm_bridge_attach(). The good news is most of these APIs are used sparingly. Maybe the simplest way to handle this is to maintain a tree of bridges and generate bridge chains when an encoder is attached to a connector in the tree. Presumably there is only one connector possible for a leaf of the bridge tree, and one encoder at the root of the bridge chain. From the drm_bridge structure, you'll only be able to iterate over the bridge chain that is currently configured. Same for the encoder, you'll only be able to walk the currently configured bridge chain from struct drm_encoder::bridge_chain. This hinges on the idea that the bridge_chain is a list, not a tree, and that it only needs to exist when an encoder is attached to a connector. When an encoder isn't attached to a connector the bridges will be in the tree, and probably that tree structure will be maintained in the bridge driver itself knowing that there is one input side bridge and two output side bridges. When the input bridge is attached, it should be able to query the output side bridges for the connector that the encoder is attaching to and configure the mux and hook the input bridge to the correct output bridge. [2] https://lore.kernel.org/all/CAPj87rO7a=NbarOyZp1pE=19Lf2aGjWu7sru-eHwGjX0fpN+-A@mail.gmail.com/ [3] https://lore.kernel.org/all/CAMty3ZAQyADGLVcB13qJdEy_BiZEKpyDfCr9QrM-ucFLFPZLcw@mail.gmail.com/
On Fri, May 12, 2023 at 1:30 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Pin-yen Lin (2023-05-09 20:41:54) > > On Sat, Apr 29, 2023 at 12:47 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Good point. I'm suggesting to make the drm bridge chain into a tree. We > > > need to teach drm_bridge core about a mux, and have some logic to > > > navigate atomically switching from one output to another. I was talking > > > with dianders@ and he was suggesting to use bridge attach/detach for > > > this. I'm not sure that will work though because that hook is only > > > called when the encoder is attached to the bridge. > > > > > > It may also turn out that this helps with DP multi-stream transport > > > (MST). As far as I can recall DP MST doesn't mesh well with drm_bridge > > > designs because it wants to operate on a drm_connector and > > > drm_bridge_connector_init() wants to make only one drm_connector for a > > > chain of bridges. If you squint, the anx7625 could be an MST "branch" > > > that only supports one drm_connector being enabled at a time. Maybe that > > > is what we should do here, make drm_bridge support creating more than > > > one drm_connector and when there is a mux in the tree it walks both > > > sides and runs a callback similar to struct > > > drm_dp_mst_topology_cbs::add_connector() to tell the encoder that > > > there's another possible drm_connector here. > > > > I have been surveying the approaches to change the bridge chain in > > runtime, and I found this thread[1]. Quoting from Daniel: > > I find the lore links easier to read. > > > "... exchanging the bridge chain isn't supported, there's no locking > > for that since it's assumed to be invariant over the lifetime of the > > drm_device instance. The simplest way to make that happen right now is to > > have 2 drm_encoder instances, one with the lvds bridge chain, the other > > with the hdmi bridge chain, and select the right encoder/bridge chain > > depending upon which output userspace picks. > > ... > > I wouldn't try to make bridge chains exchangeable instead, that's > > headaches - e.g. with dp mst we've also opted for a bunch of fake > > drm_encoders to model that kind of switching." > > > > I'm not sure how we register two encoders properly, though. Do we make > > the encoder driver check all the downstream bridges and register two > > drm_encoder when a bridge is acting as a mux? > > I honestly don't know because I'm not a DRM expert. Maybe Jagan or DRM > bridge maintainers can add to the thread here. > > I kept reading the thread[2] and I think they settled on 2 drm_encoders > because they're different display formats (LVDS vs. HDMI) and 2 > drm_connector structures. But then I watched the youtube video from this > thread[3] and it seems like 1 drm_encoder is actually what should be > done because there is really only DSI at the root. There's at least > three people working on this topic of muxing displays though. Good news? > > The analogy between their gpio controlled mux and this anx part with a > crosspoint switch is that the gpio is like the crosspoint switch, but > the gpio is like a virtual mux? If the gpio is asserted for them, one > display bridge is enabled (HDMI) and the other one is not (LVDS). > > In this case, the type-c cables may be connected to both > usb-c-connectors and HPD may be asserted on both, but only one > drm_connector will be able to attach to the 1 drm_encoder at a time. If > we had 2 drm_encoders it would be possible to attach both encoders to > both drm_connectors at the same time, which is impossible because it's a > mux. Indicating that each connector is connected is OK when HPD is high > on both usb-c-connectors. Userspace will have to attach an encoder to > the drm_connector it wants to use, and the drm_connector will indicate > which drm_encoders are possible for it, so all the information will be > provided to userspace in this design. > > I think it really comes down to implementing the tree walking logic in > the drm bridge APIs. The problem is things like > drm_bridge_get_next_bridge(), drm_bridge_get_prev_bridge(), and > drm_for_each_bridge_in_chain() which will have to operate on a tree > instead of a list. There's also drm_bridge_chain_get_first_bridge() and > drm_bridge_attach(). The good news is most of these APIs are used > sparingly. > > Maybe the simplest way to handle this is to maintain a tree of bridges > and generate bridge chains when an encoder is attached to a connector in > the tree. Presumably there is only one connector possible for a leaf of > the bridge tree, and one encoder at the root of the bridge chain. From > the drm_bridge structure, you'll only be able to iterate over the bridge > chain that is currently configured. Same for the encoder, you'll only be > able to walk the currently configured bridge chain from struct > drm_encoder::bridge_chain. If I understand correctly, encoders are attached to connectors when the driver initializes itself. Do you mean that the chain should be generated when a connector is connected? If so, "attach" becomes adding a bridge to the bridge "tree", and the chain is only determined after a connector is plugged. When HPD fires, we can use the .detect callback to find the right connector and form the bridge chain. The following changes would need to be made to the existing APIs: (1) Some of the chain-traversing helpers (e.g., drm_for_each_bridge_in_chain()) need to be changed to traversing the bridge tree. (2) Drivers that hold a pointer to the connector need to either hold a list of possible connectors or use a helper to get the active connector at runtime. I don't really know if this would work out. e.g., Do we really not need the chain when the connector is disconnected? I will appreciate any suggestions on this. Regards, Pin-yen > This hinges on the idea that the bridge_chain is a list, not a tree, and > that it only needs to exist when an encoder is attached to a connector. > When an encoder isn't attached to a connector the bridges will be in the > tree, and probably that tree structure will be maintained in the bridge > driver itself knowing that there is one input side bridge and two output > side bridges. When the input bridge is attached, it should be able to > query the output side bridges for the connector that the encoder is > attaching to and configure the mux and hook the input bridge to the > correct output bridge. > > [2] https://lore.kernel.org/all/CAPj87rO7a=NbarOyZp1pE=19Lf2aGjWu7sru-eHwGjX0fpN+-A@mail.gmail.com/ > [3] https://lore.kernel.org/all/CAMty3ZAQyADGLVcB13qJdEy_BiZEKpyDfCr9QrM-ucFLFPZLcw@mail.gmail.com/
On Wed, May 10, 2023 at 9:12 AM Pin-yen Lin <treapking@chromium.org> wrote: > > +Jagan who worked on a similar design and initiated the thread. > > Hi Stephen, > > On Sat, Apr 29, 2023 at 12:47 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Pin-yen Lin (2023-04-20 02:10:46) > > > On Thu, Apr 20, 2023 at 2:10 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > Quoting Stephen Boyd (2023-04-13 17:22:46) > > > > > Quoting Pin-yen Lin (2023-04-13 02:50:44) > > > > > > > > > > > > Actually the `mode-switch` property here is mainly because > > > > > > `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches > > > > > > when the property is present. I am not sure what side effects would be > > > > > > if I remove the ID-matching condition in `typec_mux_match`, so I added > > > > > > the property here. > > > > > > > > > > > > Is it feasible to remove the `mode-switch` property here given the > > > > > > existing implementation of the Type-C framework? > > > > > > > > > > Omitting the mode-switch property would require changes to the type-c > > > > > framework. > > > > > > > > > > I'm wondering if we can have this anx driver register mode switches for > > > > > however many endpoints exist in the output port all the time when the > > > > > aux-bus node doesn't exist. Then the type-c framework can walk from the > > > > > usb-c-connector to each connected node looking for a device that is both > > > > > a drm_bridge and a mode-switch. When it finds that combination, it knows > > > > > that the mode-switch has been found. This hinges on the idea that a > > > > > device that would have the mode-switch property is a drm_bridge and > > > > > would register a mode-switch with the type-c framework. > > > > > > > > > > It may be a little complicated though, because we would only register > > > > > one drm_bridge for the input to this anx device. The type-c walking code > > > > > would need to look at the graph endpoint, and find the parent device to > > > > > see if it is a drm_bridge. > > > > > > > > I've been thinking more about this. I think we should only have the > > > > 'mode-switch' property possible when the USB input pins (port@2) are > > > > connected and the DPI input pins are connected (port@0). Probably you > > > > don't have that case though? > > > > > > No we don't have the use case that uses the USB input pins on anx7625. > > > > > > > > In your case, this device should register either one or two drm_bridges > > > > that connect to whatever downstream is actually muxing the 2 DP lanes > > > > with the USB SS lanes onto the usb-c-connector. > > > > > > What do you mean by "muxing the 2 DP lanes with the USB SS lanes''? In > > > our use case, the USB data lanes from both ports are connected to a > > > USB hub, but the DP lanes are muxed by the crosspoint switch on > > > anx7625. HPD and AUX for the external display are muxed by the EC. You > > > can find the diagram at > > > https://lore.kernel.org/linux-usb/YxGzk6DNAt0aCvIY@chromium.org/ > > > > I mean that you must have some sort of orientation switch hardware that > > takes the 2 DP lanes and the 2 USB SuperSpeed "lanes" or "pairs" and > > puts them all onto a usb-c-connector. The usb-c-connector node can't be > > connected directly to the anx7625 in your diagram because there must be > > some sort of "flipper" that does the orientation control. Otherwise the > > usb-c-connector wouldn't work if the user flipped the cable. Probably > > this is some TCPC or redriver controlled by the EC. > > > > > > > > > If that is the EC for > > > > ChromeOS, then the EC should have a binding that accepts some number of > > > > input ports for DP. The EC would act as a drm_bridge, or in this case > > > > probably two bridges, and also as two type-c switches for each > > > > drm_bridge corresponding to the usb-c-connector nodes. When DP is on the > > > > cable, the type-c switch/mux would signal to the drm_bridge that the > > > > display is 'connected' via DRM_BRIDGE_OP_DETECT and struct > > > > drm_bridge_funcs::detect(). Then the drm_bridge in this anx part would > > > > implement struct drm_bridge_funcs::atomic_enable() and configure the > > > > crosspoint switch the right way depending on the reg property of the > > > > output node in port@1. > > > > > > So there will be two drm bridges that act as the downstreams for > > > anx7625, and we find the downstream with connector_status_connected to > > > configure the crosspoint switch? How do we support that kind of > > > topology given that the drm bridge chain is currently a list? Are you > > > suggesting making the bridge topology to a tree, or maintaining the > > > two downstreams inside the anx7625 driver and not attaching them to > > > the bridge chain? > > > > Good point. I'm suggesting to make the drm bridge chain into a tree. We > > need to teach drm_bridge core about a mux, and have some logic to > > navigate atomically switching from one output to another. I was talking > > with dianders@ and he was suggesting to use bridge attach/detach for > > this. I'm not sure that will work though because that hook is only > > called when the encoder is attached to the bridge. > > > > It may also turn out that this helps with DP multi-stream transport > > (MST). As far as I can recall DP MST doesn't mesh well with drm_bridge > > designs because it wants to operate on a drm_connector and > > drm_bridge_connector_init() wants to make only one drm_connector for a > > chain of bridges. If you squint, the anx7625 could be an MST "branch" > > that only supports one drm_connector being enabled at a time. Maybe that > > is what we should do here, make drm_bridge support creating more than > > one drm_connector and when there is a mux in the tree it walks both > > sides and runs a callback similar to struct > > drm_dp_mst_topology_cbs::add_connector() to tell the encoder that > > there's another possible drm_connector here. > > I have been surveying the approaches to change the bridge chain in > runtime, and I found this thread[1]. Quoting from Daniel: > "... exchanging the bridge chain isn't supported, there's no locking > for that since it's assumed to be invariant over the lifetime of the > drm_device instance. The simplest way to make that happen right now is to > have 2 drm_encoder instances, one with the lvds bridge chain, the other > with the hdmi bridge chain, and select the right encoder/bridge chain > depending upon which output userspace picks. > ... > I wouldn't try to make bridge chains exchangeable instead, that's > headaches - e.g. with dp mst we've also opted for a bunch of fake > drm_encoders to model that kind of switching." > > I'm not sure how we register two encoders properly, though. Do we make > the encoder driver check all the downstream bridges and register two > drm_encoder when a bridge is acting as a mux? > > [1]: https://www.spinics.net/lists/dri-devel/msg340511.html > > > > > > > > > Also, if we still register mode switches on the two downstream > > > bridges, why do you prefer that over the original approach that > > > register switches in the anx7625 driver? > > > > I prefer to not have a mode-switch property here for a couple reasons: > > > > 1. The binding is usb type-c specific, and in the case of the IT6505 > > part there is nothing that indicates this is a usb type-c piece of > > hardware. The IT6505 is simply a display bridge. The anx7625 part > > actually does accept usb signals though, but that isn't being used or > > described here. That's where my disclaimer about mode-switch making > > sense applies when the usb input is used. > > > > 2. Putting mode-switch into the graph endpoint nodes is awkward. It is > > a device property, and graph nodes are not devices. Some patches in > > this series have to work around this fact and special case the graph > > walking logic to treat the graph itself as a place to look for the > > property. > > > > 3. The mode-switch property probably isn't necessary at all. The DT > > reviewers have been asking why it is needed. The EC driver that > > registers the usb-c-connectors can be the mode-switch and the > > orientation-switch. And in reality, it _is_ both. The DP signals and > > the USB signals go to the TCPC/redriver that is controlled by the EC > > and the EC is the device that's doing the mode switching to push DP and > > USB through the TCPC/redriver out on the right pins of the > > usb-c-connector. I'm hoping that I'm replying to the right in-line comments. The design I have tried was able to switch the runtime display, where I initially booted the system with LVDS and once Linux comes up the console then I can connect HDMI Out cable so-that switch bridge will detect the HPD and Turn-Off LVDS and Turn-ON HDMI Display. If I unplug HDMI Out cable then the switch bridge detects the change of HPD and Turn-Off HDMI and Turn ON LVDS. The design uses a switching bridge that acts as another gpio-interrupt bridge that sits between the DSI controller and SN65DSI84/ADV7535 bridges. From the switch bridge, the final bridge pipeline will attach during bridge ops attach call which is indeed uncommon with respect to some of the mainline drivers. If the switch bridge detects the changes in HPD then the handler will update kms and which indeed updates the bridge chain at run-time. So may be unconventional but I don't find any other solutions other than altering the bridge chain at run time. Incidentally, I see more switch bridges coming on this design in the market in order to produce more than one output. May be it is a worthy topic to discuss further. Thanks, Jagan.
diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml index b42553ac505c..604c7391d74f 100644 --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml @@ -12,7 +12,8 @@ maintainers: description: | The ANX7625 is an ultra-low power 4K Mobile HD Transmitter - designed for portable devices. + designed for portable devices. Product brief is available at + https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf properties: compatible: @@ -112,9 +113,40 @@ properties: data-lanes: true port@1: - $ref: /schemas/graph.yaml#/properties/port + $ref: /schemas/graph.yaml#/$defs/port-base description: - Video port for panel or connector. + Video port for panel or connector. Each endpoint connects to a video + output downstream, and the "data-lanes" property is used to describe + the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, SSTX1, + SSRX2, SSTX2, respectively. + + patternProperties: + "^endpoint@[01]$": + $ref: /schemas/media/video-interfaces.yaml# + properties: + reg: true + + remote-endpoint: true + + data-lanes: + oneOf: + - items: + - enum: [0, 1, 2, 3] + + - items: + - const: 0 + - const: 1 + + - items: + - const: 2 + - const: 3 + + mode-switch: + type: boolean + description: Serves as Type-C mode switch if present. + + required: + - remote-endpoint required: - port@0 @@ -186,3 +218,53 @@ examples: }; }; }; + - | + i2c3 { + #address-cells = <1>; + #size-cells = <0>; + + encoder@58 { + compatible = "analogix,anx7625"; + reg = <0x58>; + pinctrl-names = "default"; + pinctrl-0 = <&anx7625_dp_pins>; + enable-gpios = <&pio 176 GPIO_ACTIVE_HIGH>; + reset-gpios = <&pio 177 GPIO_ACTIVE_HIGH>; + vdd10-supply = <&pp1100_dpbrdg>; + vdd18-supply = <&pp1800_dpbrdg_dx>; + vdd33-supply = <&pp3300_dpbrdg_dx>; + analogix,audio-enable; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + anx7625_dp_in: endpoint { + bus-type = <7>; + remote-endpoint = <&dpi_out>; + }; + }; + + port@1 { + #address-cells = <1>; + #size-cells = <0>; + + reg = <1>; + anx_typec0: endpoint@0 { + reg = <0>; + mode-switch; + data-lanes = <0 1>; + remote-endpoint = <&typec_port0>; + }; + anx_typec1: endpoint@1 { + reg = <1>; + mode-switch; + data-lanes = <2 3>; + remote-endpoint = <&typec_port1>; + }; + }; + }; + }; + };