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 <[email protected]>
> > > > ---
> > > > 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).
>
> 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/[email protected]/
> >
> > 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/[email protected]
> > v4: https://lore.kernel.org/r/[email protected]
> > (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
>
> 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/[email protected]/
>
--
With best wishes
Dmitry
