Re: [PATCH] drm/bridge: analogix_dp: Fix connector status detection for bridges

[Damon Ding <damon.ding@rock-chips.com>]

Hi,

On 10/10/2025 7:42 AM, Heiko Stübner wrote:
> Hi Dmitry,
> 
> Am Freitag, 10. Oktober 2025, 00:30:11 Mitteleuropäische Sommerzeit schrieb Dmitry Baryshkov:
>> On Thu, Oct 09, 2025 at 09:30:28PM +0200, Heiko Stuebner wrote:
>>> Right now if there is a next bridge attached to the analogix-dp controller
>>> the driver always assumes this bridge is connected to something, but this
>>> is of course not always true, as that bridge could also be a hotpluggable
>>> dp port for example.
>>>
>>> On the other hand, as stated in commit cb640b2ca546 ("drm/bridge: display-
>>> connector: don't set OP_DETECT for DisplayPorts"), "Detecting the monitor
>>> for DisplayPort targets is more complicated than just reading the HPD pin
>>> level" and we should be "letting the actual DP driver perform detection."
>>>
>>> So use drm_bridge_detect() to detect the next bridge's state but ignore
>>> that bridge if the analogix-dp is handling the hpd-gpio.
>>>
>>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>>> ---
>>> As this patch stands, it would go on top of v6 of Damon's bridge-connector
>>> work, but could very well be also integrated into one of the changes there.
>>>
>>> I don't know yet if my ordering and/or reasoning is the correct one or if
>>> a better handling could be done, but with that change I do get a nice
>>> hotplug behaviour on my rk3588-tiger-dp-carrier board, where the
>>> Analogix-DP ends in a full size DP port.
>>>
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index c04b5829712b..cdc56e83b576 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -983,8 +983,12 @@ analogix_dp_bridge_detect(struct drm_bridge *bridge, struct drm_connector *conne
>>>   	struct analogix_dp_device *dp = to_dp(bridge);
>>>   	enum drm_connector_status status = connector_status_disconnected;
>>>   
>>> -	if (dp->plat_data->next_bridge)
>>> -		return connector_status_connected;
>>> +	/*
>>> +	 * An optional next bridge should be in charge of detection the
>>> +	 * connection status, except if we manage a actual hpd gpio.
>>> +	 */
>>> +	if (dp->plat_data->next_bridge && !dp->hpd_gpiod)
>>> +		return drm_bridge_detect(dp->plat_data->next_bridge, connector);

I have tried to use the drm_bridge_detect() API to do this as 
simple-bridge driver, but it does not work well for bridges that do not 
declare OP_DETECT.

Take nxp-ptn3460 as an example, the connected status will be treated as 
connector_status_unknown via the following call stack:

drm_helper_probe_single_connector_modes()
   -> drm_helper_probe_detect()
      -> drm_bridge_connector_detect()
         -> analogix_dp_bridge_detect()
            -> drm_bridge_detect()
               -> return connector_status_unknown due to !OP_DETECT

However, the connected status will be connector_status_connected as 
expected if Analogix DP does not declare OP_DETECT[0]:

drm_helper_probe_single_connector_modes()
   -> drm_helper_probe_detect()
      -> drm_bridge_connector_detect()
         -> return connector_status_connected due to CONNECTOR_LVDS

Base on Andy's commit 5d156a9c3d5e ("drm/bridge: Pass down connector to 
drm bridge detect hook"), the drm_connector becomes available in 
drm_bridge_detect().

It may be better to unify the logic of drm_bridge_detect() and 
drm_bridge_connector_detect(), which sets the connected status according 
to the connector_type. Then the codes will be more reasonable and become 
similar to the simple-bridge demo via 
'drm_bridge_detect(dp->plat_data->next_bridge, connector)'.

But we still need a specific check for DP-connector to resolve this 
issue. The '!dp->hpd_gpiod' may not be well-considered. Perhaps we could 
introduce a new API, similar to drm_bridge_is_panel(), called 
drm_bridge_is_display_connector()?

>>
>> And it's also not correct because the next bridge might be a retimer
>> with the bridge next to it being a one with the actual detection
>> capabilities. drm_bridge_connector solves that in a much better way. See
>> the series at [1]
>>
>> [1] https://lore.kernel.org/dri-devel/41c2a141-a72e-4780-ab32-f22f3a2e0179@samsung.com/
> 
> Hence my comment above about that possibly not being the right variant.
> Sort of asking for direction :-) .
> 
> I am working on top of Damon's drm-bridge-connector series as noted above,
> but it looks like the detect function still is called at does then stuff.
> 
> My board is the rk3588-tiger-displayport-carrier [0], with a dp-connector
> which is the next bridge, so _without_ any changes, the analogix-dp
> always assumes "something" is connected and I end up with
> 
> [    9.869198] [drm:analogix_dp_bridge_atomic_enable] *ERROR* failed to get hpd single ret = -110
> [    9.980422] [drm:analogix_dp_bridge_atomic_enable] *ERROR* failed to get hpd single ret = -110
> [   10.091522] [drm:analogix_dp_bridge_atomic_enable] *ERROR* failed to get hpd single ret = -110
> [   10.202419] [drm:analogix_dp_bridge_atomic_enable] *ERROR* failed to get hpd single ret = -110
> [   10.313651] [drm:analogix_dp_bridge_atomic_enable] *ERROR* failed to get hpd single ret = -110
> 
> when no display is connected.
> 
> With this change I do get the expected hotplug behaviour, so something is
> missing still even with the bridge-connector series.
> 
> 
> Heiko
> 
> 
> [0] v3: https://lore.kernel.org/r/20250812083217.1064185-3-heiko@sntech.de
>      v4: https://lore.kernel.org/r/20251009225050.88192-3-heiko@sntech.de
>      (moved hpd-gpios from dp-connector back to analogix-dp per dp-connector
>      being not able to detect dp-monitors)
>>
>>>   
>>>   	if (!analogix_dp_detect_hpd(dp))
>>>   		status = connector_status_connected;
>>
>>
> 
> 

I see... There is also a way to leave the connected status check in 
Analogix DP bridge:

1.If the later bridge does not support HPD function, the 'force-hpd' 
property must be set under the DP DT node. The DT modifications may be
large by this way.
2.If the later bridge do support HPD function like the DP-connector, the
connected status detection method is GPIO HPD or functional pin HPD.

With the DT modifications for above 1, the analogix_dp_bridge_detect() 
can be simplified to:

static enum drm_connector_status
analogix_dp_bridge_detect(struct drm_bridge *bridge, struct 
drm_connector *connector)
{
	struct analogix_dp_device *dp = to_dp(bridge);
	enum drm_connector_status status = connector_status_disconnected;

	if (!analogix_dp_detect_hpd(dp))
		status = connector_status_connected;

	return status;
}

Best regards,
Damon

[0]https://lore.kernel.org/all/22a5119c-01da-4a7a-bafb-f657b1552a56@rock-chips.com/