Message-ID: <2ebace6f-d3c4-4516-b6cb-4951de06b6c8@rock-chips.com>
Date: Mon, 17 Nov 2025 09:33:55 +0800
From: Chaoyi Chen <chaoyi.chen@...k-chips.com>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>, Chaoyi Chen
 <kernel@...kyi.com>, Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
 Peter Chen <hzpeterchen@...il.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Vinod Koul <vkoul@...nel.org>,
 Kishon Vijay Abraham I <kishon@...nel.org>, Heiko Stuebner
 <heiko@...ech.de>, Sandy Huang <hjc@...k-chips.com>,
 Andy Yan <andy.yan@...k-chips.com>,
 Yubing Zhang <yubing.zhang@...k-chips.com>,
 Frank Wang <frank.wang@...k-chips.com>,
 Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Amit Sunil Dhamne <amitsd@...gle.com>, Dragan Simic <dsimic@...jaro.org>,
 Johan Jonker <jbx6244@...il.com>, Diederik de Haas <didi.debian@...ow.org>,
 Peter Robinson <pbrobinson@...il.com>
Cc: linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v9 08/10] drm/rockchip: cdn-dp: Add multiple bridges to
 support PHY port selection

Hi Luca,

On 11/12/2025 9:37 AM, Chaoyi Chen wrote:
> Hello Luca,
>
> On 11/11/2025 11:14 PM, Luca Ceresoli wrote:
>> Hello Chaoyi,
>>
>> On Tue Nov 11, 2025 at 11:50 AM CET, Chaoyi Chen wrote:
>>> From: Chaoyi Chen <chaoyi.chen@...k-chips.com>
>>>
>>> The RK3399 has two USB/DP combo PHY and one CDN-DP controller. And
>>> the CDN-DP can be switched to output to one of the PHYs. If both ports
>>> are plugged into DP, DP will select the first port for output.
>>>
>>> This patch adds support for multiple bridges, enabling users to flexibly
>>> select the output port. For each PHY port, a separate encoder and bridge
>>> are registered.
>>>
>>> The change is based on the DRM AUX HPD bridge, rather than the
>>> extcon approach. This requires the DT to correctly describe the
>>> connections between the first bridge in bridge chain and DP
>>> controller. For example, the bridge chain may be like this:
>>>
>>> PHY aux birdge -> fsa4480 analog audio switch bridge ->
>>> onnn,nb7vpq904m USB reminder bridge -> USB-C controller AUX HPD bridge
>>>
>>> In this case, the connection relationships among the PHY aux bridge
>>> and the DP contorller need to be described in DT.
>>>
>>> In addition, the cdn_dp_parse_next_bridge_dt() will parses it and
>>> determines whether to register one or two bridges.
>>>
>>> Since there is only one DP controller, only one of the PHY ports can
>>> output at a time. The key is how to switch between different PHYs,
>>> which is handled by cdn_dp_switch_port() and cdn_dp_enable().
>>>
>>> There are two cases:
>>>
>>> 1. Neither bridge is enabled. In this case, both bridges can
>>> independently read the EDID, and the PHY port may switch before
>>> reading the EDID.
>>>
>>> 2. One bridge is already enabled. In this case, other bridges are not
>>> allowed to read the EDID. So we will try to return the cached EDID.
>>>
>>> Since the scenario of two ports plug in at the same time is rare,
>>> I don't have a board which support two TypeC connector to test this.
>>> Therefore, I tested forced switching on a single PHY port, as well as
>>> output using a fake PHY port alongside a real PHY port.
>>>
>>> Signed-off-by: Chaoyi Chen <chaoyi.chen@...k-chips.com>
>> [...]
>>
>>> @@ -966,28 +1084,16 @@ static int cdn_dp_pd_event(struct notifier_block *nb,
>>>       return NOTIFY_DONE;
>>>   }
>>>
>>> -static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>>> +static int cdn_bridge_add(struct device *dev,
>>> +              struct drm_bridge *bridge,
>>> +              struct drm_bridge *next_bridge,
>>> +              struct drm_encoder *encoder)
>>>   {
>>>       struct cdn_dp_device *dp = dev_get_drvdata(dev);
>>> -    struct drm_encoder *encoder;
>>> +    struct drm_device *drm_dev = dp->drm_dev;
>>> +    struct drm_bridge *last_bridge = NULL;
>>>       struct drm_connector *connector;
>>> -    struct cdn_dp_port *port;
>>> -    struct drm_device *drm_dev = data;
>>> -    int ret, i;
>> [...]
>>
>>> +    if (next_bridge) {
>>> +        ret = drm_bridge_attach(encoder, next_bridge, bridge,
>>> +                    DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        last_bridge = next_bridge;
>>> +        while (drm_bridge_get_next_bridge(last_bridge))
>>> +            last_bridge = drm_bridge_get_next_bridge(last_bridge);
>> DRM bridges are now refcounted, and you are not calling drm_bridge_get()
>> and drm_bridge_put() here. But here you can use
>> drm_bridge_chain_get_last_bridge() which will simplify your job.
>>
>> Don't forget to call drm_bridge_put() on the returned bridge when the
>> bridge is not referenced anymore. This should be as easy as adding a
>> cleanup action on the variable declaration above:
>>
>> -    struct drm_bridge *last_bridge = NULL;
>> +    struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL;
>
> Ah, I have seen your patch about this. Thank you for the reminder, I will fix this in v10.
>
>>
>>> @@ -1029,8 +1147,102 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>>>           return ret;
>>>       }
>>>
>>> +    if (last_bridge)
>>> +        connector->fwnode = fwnode_handle_get(of_fwnode_handle(last_bridge->of_node));
>>> +
>>>       drm_connector_attach_encoder(connector, encoder);
>>>
>>> +    return 0;
>>> +}
>>> +
>>> +static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp)
>>> +{
>>> +    struct device_node *np = dp->dev->of_node;
>>> +    struct device_node *port __free(device_node) = of_graph_get_port_by_id(np, 1);
>>> +    struct drm_bridge *bridge;
>>> +    int count = 0;
>>> +    int ret = 0;
>>> +    int i;
>>> +
>>> +    /* If device use extcon, do not use hpd bridge */
>>> +    for (i = 0; i < dp->ports; i++) {
>>> +        if (dp->port[i]->extcon) {
>>> +            dp->bridge_count = 1;
>>> +            return 0;
>>> +        }
>>> +    }
>>> +
>>> +
>>> +    /* One endpoint may correspond to one next bridge. */
>>> +    for_each_of_graph_port_endpoint(port, dp_ep) {
>>> +        struct device_node *next_bridge_node __free(device_node) =
>>> +            of_graph_get_remote_port_parent(dp_ep);
>>> +
>>> +        bridge = of_drm_find_bridge(next_bridge_node);
>>> +        if (!bridge) {
>>> +            ret = -EPROBE_DEFER;
>>> +            goto out;
>>> +        }
>>> +
>>> +        dp->next_bridge_valid = true;
>>> +        dp->next_bridge_list[count].bridge = bridge;
>> You are storing a reference to a drm_bridge, so have to increment the
>> refcount:
>>
>>         dp->next_bridge_list[count].bridge = drm_bridge_get(bridge);
>>                                              ^^^^^^^^^^^^^^
>>
>> FYI there is a plan to replace of_drm_find_bridge() with a function that
>> increases the bridge refcount before returning the bridge, but it's not
>> there yet. When that will happen, the explicit drm_bridge_get() won't be
>> needed anymore and this code can be updated accordingly.

Out of curiosity, I checked the callers of of_drm_find_bridge(), and it seems that the vast majority of them do not pay attention to the increase or decrease of reference counts. Does this mean that even if we add reference counting in of_drm_find_bridge(), we still need to modify the corresponding functions of their callers and decrease the reference count at the appropriate time? Thank you.


>>
>> Also you have to call drm_bridge_put() to release that reference when the
>> pointer goes away. I guess that should happen in cdn_dp_unbind().
>
> You're right, this is indeed a pitfall. I will fix it in v10.
>
>
-- 
Best,
Chaoyi

Powered by blists - more mailing lists