On Wed, May 07, 2025 at 03:45:52PM +0530, Jayesh Choudhary wrote:
> Hello Max,
>
> On 06/05/25 22:14, Max Krummenacher wrote:
> > 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...@gmail.com> wrote:
> > > >
> > > > From: Max Krummenacher <max.krummenac...@toradex.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.
> >
>
> I have same observations on my end.
>
> > >
> > >
> > > > Signed-off-by: Max Krummenacher <max.krummenac...@toradex.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.
> >
>
>
> While adding this delay fixes the detect issue, it could lead to other
> problems.
> Detect hook is called every 10 sec and considering that, 400ms is a
> considerable amount of time. And it could cause performance issues like
> frame drops and glitches in display.
>
> For 1920x1080@60fps resolution, when I run weston application, I see
> around ~24 frames being dropped every 10 sec with visual glitch in
> display. This seems consistent with 400ms delay for 60fps or 16.67ms per
> frame (24*16.67ms).
>
> root@j784s4-evm:~# weston-simple-egl
> libEGL warning: MESA-LOADER: failed to open tidss:
> /usr/lib/dri/tidss_dri.so: cannot open shared object file: No such file or
> directory (search paths /usr/lib/dri, suffix _dri)
>
> 276 frames in 5 seconds: 55.200001 fps
> 301 frames in 5 seconds: 60.200001 fps
> 277 frames in 5 seconds: 55.400002 fps
> 301 frames in 5 seconds: 60.200001 fps
> 277 frames in 5 seconds: 55.400002 fps
> 301 frames in 5 seconds: 60.200001 fps
> 277 frames in 5 seconds: 55.400002 fps
> 301 frames in 5 seconds: 60.200001 fps
> 277 frames in 5 seconds: 55.400002 fps
> 301 frames in 5 seconds: 60.200001 fps
> 277 frames in 5 seconds: 55.400002 fps
> 301 frames in 5 seconds: 60.200001 fps
> 278 frames in 5 seconds: 55.599998 fps
> ^Csimple-egl exiting
> root@j784s4-evm:~#
>
> > >
> > > 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.
> >
>
> I have seen such link training failures occasionally when the
> display connector is not connected but the state is not reflected
> correctly.
> Maybe it could be attributed to long polling duration???
> Are you observing it even on re-runs?
I think it is caused by the used hardware allowing to control the enable
signal but not controlling the power supplies.
If that is really true I have yet to find out.
>
>
> > > -Doug
> >
> > Regards
> > Max
>
>
> Warm Regards,
> Jayesh
In my opinion we should drop this patch in favour of Jayesh's V2 [1].
I will comment there.
Regards
Max
[1] https://lore.kernel.org/all/20250508115433.449102-1-j-choudh...@ti.com/
