Re: [PATCH v2] drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort connector type

[Jayesh Choudhary <j-choudhary@ti.com>]

Hello Dmitry, Doug,

Thanks a lot for the review.

On 22/05/25 18:44, Dmitry Baryshkov wrote:
> On Wed, May 21, 2025 at 06:10:59PM -0700, Doug Anderson wrote:
>> Hi,
>>
>> On Thu, May 8, 2025 at 4:54 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>>
>>> By default, HPD was disabled on SN65DSI86 bridge. When the driver was
>>> added (commit "a095f15c00e27"), the HPD_DISABLE bit was set in pre-enable
>>> call which was moved to other function calls subsequently.
>>> Later on, commit "c312b0df3b13" added detect utility for DP mode. But with
>>> HPD_DISABLE bit set, all the HPD events are disabled[0] and the debounced
>>> state always return 1 (always connected state)
>>>
>>> Also, with the suspend and resume calls before every register access, the
>>> bridge starts with disconnected state and the HPD state is reflected
>>> correctly only after debounce time (400ms). However, adding this delay
>>> in the detect function causes frame drop and visible glitch in display.
>>>
>>> So to get the detect utility working properly for DP mode without any
>>> performance issues in display, instead of reading HPD state from the
>>> register, rely on aux communication. Use 'drm_dp_dpcd_read_link_status'
>>> to find if we have something connected at the sink.
>>>
>>> [0]: <https://www.ti.com/lit/gpn/SN65DSI86> (Pg. 32)
>>>
>>> Fixes: c312b0df3b13 ("drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP")
>>> Cc: Max Krummenacher <max.krummenacher@toradex.com>
>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>> ---
>>>
>>> v1 patch link which was sent as RFC:
>>> <https://patchwork.kernel.org/project/dri-devel/patch/20250424105432.255309-1-j-choudhary@ti.com/>
>>>
>>> Changelog v1->v2:
>>> - Drop additional property in bindings and use conditional.
>>> - Instead of register read for HPD state, use dpcd read which returns 0
>>>    for success and error codes for no connection
>>> - Add relevant history for the required change in commit message
>>> - Drop RFC subject-prefix in v2 as v2 is in better state after discussion
>>>    in v1 and Max's mail thread
>>> - Add "Cc:" tag
>>>
>>> This approach does not make suspend/resume no-op and no additional
>>> delay needs to be added in the detect hook which causes frame drops.
>>>
>>> Here, I am adding conditional to HPD_DISABLE bit even when we are
>>> not using the register read to get HPD state. This is to prevent
>>> unnecessary register updates in every resume call.
>>> (It was adding to latency and leading to ~2-3 frame drop every 10 sec)
>>>
>>> Tested and verified on TI's J784S4-EVM platform:
>>> - Display comes up
>>> - Detect utility works with a couple of seconds latency.
>>>    But I guess without interrupt support, this is acceptable.
>>> - No frame-drop observed
>>>
>>> Discussion thread for Max's patch:
>>> <https://patchwork.kernel.org/project/dri-devel/patch/20250501074805.3069311-1-max.oss.09@gmail.com/>
>>>
>>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 ++++++++++-------
>>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> Sorry for the delay in responding. Things got a little crazy over the
>> last few weeks.
>>
>>
>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>>> index 60224f476e1d..9489e78b6da3 100644
>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>>> @@ -352,8 +352,10 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata,
>>>           * change this to be conditional on someone specifying that HPD should
>>>           * be used.
>>>           */
>>> -       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);
>>
>> Given your an Max's testing, I'm totally on-board with the above.
>>
>>>
>>>          pdata->comms_enabled = true;
>>>
>>> @@ -1194,13 +1196,14 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>>>   {
>>>          struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>>          int val = 0;
>>> +       u8 link_status[DP_LINK_STATUS_SIZE];
>>>
>>> -       pm_runtime_get_sync(pdata->dev);
>>> -       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>>> -       pm_runtime_put_autosuspend(pdata->dev);
>>> +       val = drm_dp_dpcd_read_link_status(&pdata->aux, link_status);
>>>
>>> -       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>>> -                                        : connector_status_disconnected;
>>> +       if (val < 0)
>>> +               return connector_status_disconnected;
>>> +       else
>>> +               return connector_status_connected;
>>
>> I'd really rather not do this. It took me a little while to realize
>> why this was working and also not being slow like your 400ms delay
>> was. I believe that each time you do the AUX transfer it grabs a
>> pm_runtime reference and then puts it with "autosuspend". Then you're
>> relying on the fact that detect is called often enough so that the
>> autosuspend doesn't actually hit so the next time your function runs
>> then it's fast. Is that accurate?
>>
>> I'd rather see something like this in the bridge's probe (untested)
>>
>>    if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
>>      pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
>>
>>      /*
>>       * In order for DRM_BRIDGE_OP_DETECT to work in a reasonable
>>       * way we need to keep the bridge powered on all the time.
>>       * The bridge takes hundreds of milliseconds to debounce HPD
>>       * and we simply can't wait that amount of time in every call
>>       * to detect.
>>       */
>>      pm_runtime_get_sync(pdata->dev);
>>    }
>>
>> ...obviously you'd also need to find the right times to undo this in
>> error handling and in remove.
> 
> What about:
> - keeping pm_runtime_get()/put_autosuspend() in detect, but..
> - also adding .hpd_enable() / .hpd_disable() callbacks which would also
>    get and put the runtime PM, making sure that there is no additional
>    delay in .detect()?
> 

Keeping a reference alive via hpd_enable() fixes the issue.
Things works with the previous detect logic and I do not need to
rely on reading link status.


In hpd_enable()/disable(), I do not need to add anything else:

+static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+       pm_runtime_get_sync(pdata->dev);
+}
+
+static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+       pm_runtime_put_sync(pdata->dev);
+}
+

Posting v3 with these changes.

Warm Regards,
Jayesh

>>
>> Nicely, this would be the same type of solution needed for if we ever
>> enabled interrupts.
>