Message-ID: <7d13fd2a-090e-4a58-b862-050f9ee4ff43@rock-chips.com>
Date: Sat, 11 Oct 2025 10:47:59 +0800
From: Damon Ding <damon.ding@...k-chips.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
 Heiko Stübner <heiko@...ech.de>
Cc: m.szyprowski@...sung.com, andrzej.hajda@...el.com,
 neil.armstrong@...aro.org, rfoss@...nel.org, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linux-arm-kernel@...ts.infradead.org,
 linux-samsung-soc@...r.kernel.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH] drm/bridge: analogix_dp: Fix connector status detection
 for bridges

Hi Dmitry,

On 10/10/2025 8:43 PM, Dmitry Baryshkov wrote:
> On Fri, Oct 10, 2025 at 12:02:43PM +0800, Damon Ding wrote:
>> 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@...ech.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)'.
> 
> I'm not sure, what you are trying to achieve here. The
> drm_bridge_connector should handle the OP_DETECT and use the last bridge
> in the chain that supports detection.
> 
> Note: OP_HPD and OP_DETECT are not that tightly connected, especially
> for DP bridges. It is fine to have a later bridge which generates HPD
> events, while Analogix DP bridge responds to .hpd_notify() events and
> performs its duties.
> 
> For example, it's perfectly fine to have dp-connector with the HPD GPIO
> pin routed to the connector (in which case it will declare OP_HPD). But
> the Analogix bridge should perform actual detection. Normally this is
> handled by reading DPCD and ensuring that there is an actual connected
> device (i.e. either a non-branch device or a branch with at least 1
> sink).
> 

I see. Your kind explanation helped me understand OP_HPD and OP_DETECT 
better.

Back to Heiko's issue, the v3, in which dp-connector declares OP_HPD, 
should be better (refers to arm64/qcom/qcs6490-rb3gen2 and 
arm64/qcom/sa8295p-adp). And the .hpd_notify() of Analogix DP bridge 
will be supported in the future if something needs to be handle with the 
HPD status changes (refers to driver drivers/gpu/drm/msm/dp/dp_display.c).

Additionally, I will keep analogix_dp_bridge_detect() the same as before.

>>
>> 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.
> 
> Well... The drivers should continue to work with old DTs, if they did so
> before.
> 
>> 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.
> 
> dp-connector should set OP_HPD
> analogix-dp should set OP_DETECT
> 

Got it.

>>
>> 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/
>>
> 

Best regards,
Damon

Powered by blists - more mailing lists