Message-ID: <aBo8elFPYgPleK5n@toolbox>
Date: Tue, 6 May 2025 18:44:42 +0200
From: Max Krummenacher <max.oss.09@...il.com>
To: Doug Anderson <dianders@...omium.org>
Cc: max.krummenacher@...adex.com, Jayesh Choudhary <j-choudhary@...com>,
	Andrzej Hajda <andrzej.hajda@...el.com>,
	David Airlie <airlied@...il.com>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Jonas Karlman <jonas@...boo.se>,
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Robert Foss <rfoss@...nel.org>, Simona Vetter <simona@...ll.ch>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] drm/bridge: ti-sn65dsi86: Use HPD in a DP use case

On Thu, May 01, 2025 at 08:38:15PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, May 1, 2025 at 12:48 AM <max.oss.09@...il.com> wrote:
> >
> > From: Max Krummenacher <max.krummenacher@...adex.com>
> >
> > The bridge driver currently disables handling the hot plug input and
> > relies on a always connected eDP panel with fixed delays when the
> > panel is ready.
> 
> Not entirely correct. In some cases we don't have fixed delays and
> instead use a GPIO for HPD. That GPIO gets routed to the eDP panel
> code.

Will reword in a v2

> 
> 
> > If one uses the bridge for a regular display port monitor this
> > assumption is no longer true.
> > If used with a display port monitor change to keep the hot plug
> > detection functionality enabled and change to have the bridge working
> > during runtime suspend to be able to detect the connection state.
> >
> > Note that if HPD_DISABLE is set the HPD bit always returns connected
> > independent of the actual state of the hot plug pin. Thus
> > currently bridge->detect() always returns connected.
> 
> If that's true, it feels like this needs:
> 
> Fixes: c312b0df3b13 ("drm/bridge: ti-sn65dsi86: Implement bridge
> connector operations for DP")
> 
> ...and it would be nice to get Laurent to confirm. Seems weird that he
> wouldn't have noticed that.

I retested by adding a print in ti_sn_bridge_detect().
With the HPD_DISABLE bit set the HPD_DEBOUNCED_STATE is always true
resulting in reporting always connected.

When one does not set the HPD_DISABLE bit and is in runtime suspend
(i.e. detect() enables the bridge with its call to
pm_runtime_get_sync() ) then the HPD_DEBOUNCED_STATE is only set
after the debounce time. As it is immediately read here detect()
always reports disconnected.

> 
> 
> > Signed-off-by: Max Krummenacher <max.krummenacher@...adex.com>
> >
> > ---
> >
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 01d456b955ab..c7496bf142d1 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -333,9 +333,11 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> >          * If HPD somehow makes sense on some future panel we'll have to
> >          * change this to be conditional on someone specifying that HPD should
> >          * be used.
> > +        * Only disable HDP if used for eDP.
> >          */
> > -       regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> > -                          HPD_DISABLE);
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP)
> > +               regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> > +                                  HPD_DISABLE, HPD_DISABLE);
> >
> >         pdata->comms_enabled = true;
> >
> > @@ -357,6 +359,10 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
> >         struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
> >         int ret;
> >
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort &&
> > +           pdata->comms_enabled)
> > +               return 0;
> > +
> 
> I don't understand this part of the patch. You're basically making
> suspend/resume a no-op for the DP case? I don't think that's right...

That is what I wanted to do as nothing else worked ...
Probably there are better solutions.

> 
> First, I don't _think_ you need it, right? ...since "detect" is
> already grabbing the pm_runtime reference this shouldn't be needed
> from a correctness point of view.

Correct, it shouldn't. However if the bridge is coming out of
powerup/reset then we have to wait the debounce time time to get the
current state of HPD. The bridge starts with disconnected and only
sets connected after it seen has the HPD pin at '1' for the debounce
time.

Adding a 400ms sleep would fix that.

> 
> Second, if you're looking to eventually make the interrupt work, I
> don't think this is the right first step. I think in previous
> discussions about this it was agreed that if we wanted the interrupt
> to work then we should just do a "pm_runtime_get_sync()" before
> enabling the interrupt and then a "pm_runtime_put()" after disabling
> it. That'll keep things from suspending.

The HW I use doesn't has the interrupt pin connected. So for me that is
out of scope but should of course work.

> 
> Does that sound correct, or did I goof up on anything?

If I remove disabling suspend/resume and fix detect() to report the
'correct' HPD state in both runtime pm states I now get another issue
after disconnecting and then reconnecting the monitor:

[   50.035964] ti_sn65dsi86 3-002c: [drm:ti_sn_bridge_atomic_enable [ti_sn65dsi86]] *ERROR* Can't read lane count (-110); assuming 4
[   50.212976] ti_sn65dsi86 3-002c: [drm:ti_sn_bridge_atomic_enable [ti_sn65dsi86]] *ERROR* Can't read eDP rev (-110), assuming 1.1
[   50.389802] ti_sn65dsi86 3-002c: [drm:ti_sn_bridge_atomic_enable [ti_sn65dsi86]] *ERROR* Can't read max rate (-110); assuming 5.4 GHz
[   50.686572] ti_sn65dsi86 3-002c: [drm:ti_sn_bridge_atomic_enable [ti_sn65dsi86]] *ERROR* Link training failed, link is off (-5)

monitor stays black without signals.

So it seems the bridges internal state is not completely restored by
the current code. Looking into that now.

> -Doug

Regards
Max

Powered by blists - more mailing lists