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

[Dmitry Baryshkov]

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 at 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 at toradex.com>
> > Signed-off-by: Jayesh Choudhary <j-choudhary at 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()?

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

-- 
With best wishes
Dmitry