Message-ID: <68f0c5ef-ac51-4652-b829-2bc56c5a75c8@ti.com>
Date: Wed, 7 May 2025 15:45:52 +0530
From: Jayesh Choudhary <j-choudhary@...com>
To: Max Krummenacher <max.oss.09@...il.com>,
        Doug Anderson
	<dianders@...omium.org>
CC: <max.krummenacher@...adex.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

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

I have same observations on my end.

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


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@...ps 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@...4s4-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@...4s4-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

Powered by blists - more mailing lists