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

[Doug Anderson]

Hi,

On Thu, May 22, 2025 at 6:14 AM Dmitry Baryshkov
<dmitry.baryshkov at oss.qualcomm.com> wrote:
>
> > > @@ -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..

I guess? The problem is that if the calls in pm_runtime_get() actually
cause the device to be resumed then detect() will not actually work.
The chip simply won't report HPD right after being powered on because
it needs the debouncing time. ...so having the calls there is
misleading. Instead, I'd rather have a comment in there about why we
_don't_ have the pm_runtime_get() calls there...


> - 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()?

Sounds reasonable to me and sounds like it works.