[PATCH v1] drm/bridge: ti-sn65dsi86: Use HPD in a DP use case

[PATCH v1] drm/bridge: ti-sn65dsi86: Use HPD in a DP use case

[max.oss.09]

From: Max Krummenacher <max.krummenacher@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.

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.

Signed-off-by: Max Krummenacher <max.krummenacher@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;
+
 	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
 	if (ret) {
 		DRM_ERROR("failed to enable supplies %d\n", ret);
@@ -386,6 +392,9 @@ static int __maybe_unused ti_sn65dsi86_suspend(struct device *dev)
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
 	int ret;
 
+	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		return 0;
+
 	if (pdata->refclk)
 		ti_sn65dsi86_disable_comms(pdata);
 
-- 
2.42.0

Re: [PATCH v1] drm/bridge: ti-sn65dsi86: Use HPD in a DP use case

[Doug Anderson]

Hi,

On Thu, May 1, 2025 at 12:48 AM <max.oss.09@gmail.com> wrote:
>
> From: Max Krummenacher <max.krummenacher@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.


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


> Signed-off-by: Max Krummenacher <max.krummenacher@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...

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.

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.

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

-Doug

Re: [PATCH v1] drm/bridge: ti-sn65dsi86: Use HPD in a DP use case

[Max Krummenacher]

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@gmail.com> wrote:
> >
> > From: Max Krummenacher <max.krummenacher@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.

> 
> 
> > Signed-off-by: Max Krummenacher <max.krummenacher@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.

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

Re: [PATCH v1] drm/bridge: ti-sn65dsi86: Use HPD in a DP use case

[Jayesh Choudhary]

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.09@gmail.com> wrote:
>>>
>>> From: Max Krummenacher <max.krummenacher@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.krummenacher@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?


>> -Doug
> 
> Regards
> Max


Warm Regards,
Jayesh

Re: [PATCH v1] drm/bridge: ti-sn65dsi86: Use HPD in a DP use case

[Max Krummenacher]

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.09@gmail.com> wrote:
> > > > 
> > > > From: Max Krummenacher <max.krummenacher@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.krummenacher@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-choudhary@ti.com/