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

[Doug Anderson]

Hi,

On Wed, Jun 11, 2025 at 9:39 PM Jayesh Choudhary <j-choudhary at ti.com> wrote:
>
> Hello Doug,
>
> On 12/06/25 03:08, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Jun 10, 2025 at 10:29 PM Jayesh Choudhary <j-choudhary at ti.com> wrote:
> >>
> >> @@ -1195,9 +1203,17 @@ 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;
> >>
> >> -       pm_runtime_get_sync(pdata->dev);
> >> +       /*
> >> +        * The chip won't report HPD right after being powered on as
> >> +        * HPD_DEBOUNCED_STATE reflects correct state only after the
> >> +        * debounce time (~100-400 ms).
> >> +        * So having pm_runtime_get_sync() and immediately reading
> >> +        * the register in detect() won't work, and adding delay()
> >> +        * in detect will have performace impact in display.
> >> +        * So remove runtime calls here.
> >
> > That last sentence makes sense in a commit message, but not long term.
> > Someone reading the code later won't understand what "remove" means.
> > If you change "remove" to "omit" then it all makes sense, though. You
> > could also say that a pm_runtime reference will be grabbed by
> > ti_sn_bridge_hpd_enable().
>
> Okay. Will edit this.
>
> >
> >
> >> +        */
> >> +
> >>          regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> >> -       pm_runtime_put_autosuspend(pdata->dev);
> >>
> >>          return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> >>                                           : connector_status_disconnected;
> >> @@ -1220,6 +1236,20 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
> >>          debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
> >>   }
> >>
> >> +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);
> >> +}
> >> +
> >>   static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >>          .attach = ti_sn_bridge_attach,
> >>          .detach = ti_sn_bridge_detach,
> >> @@ -1234,6 +1264,8 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >>          .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> >>          .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> >>          .debugfs_init = ti_sn65dsi86_debugfs_init,
> >> +       .hpd_enable = ti_sn_bridge_hpd_enable,
> >> +       .hpd_disable = ti_sn_bridge_hpd_disable,
> >>   };
> >>
> >>   static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> >> @@ -1322,7 +1354,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >>                             ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> >>
> >>          if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> >> -               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
> >> +               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
> >> +                                   DRM_BRIDGE_OP_HPD;
> >
> > I think you also need this in the "DRM_MODE_CONNECTOR_DisplayPort" if test:
> >
> > /*
> >   * If comms were already enabled they would have been enabled
> >   * with the wrong value of HPD_DISABLE. Update it now. Comms
> >   * could be enabled if anyone is holding a pm_runtime reference
> >   * (like if a GPIO is in use). Note that in most cases nobody
> >   * is doing AUX channel xfers before the bridge is added so
> >   * HPD doesn't _really_ matter then. The only exception is in
> >   * the eDP case where the panel wants to read the EDID before
> >   * the bridge is added. We always consistently have HPD disabled
> >   * for eDP.
> >   */
> > mutex_lock(&pdata->comms_mutex);
> > if (pdata->comms_enabled)
> >    regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> >      HPD_DISABLE, 0);
> > mutex_unlock(&pdata->comms_mutex);
> >
> > Does that sound right?
>
>
> Here I don't think it is necessary to add this because enable_comms
> will be called again after probe either in hpd_enable() (in case
> refclk exist) or pre_enable() (in case it doesn't) with correct value.

I don't think that's necessarily true, is it? From my memory, this happens:

1. Main driver probe and we create the sub-devices, like the GPIO,
backlight, and AUX.

2. As soon as the GPIO probe happens, someone could conceivably claim
one of the GPIOs and set it as an output, which would cause a
"pm_runtime" reference to be held indefinitely.

3. After AUX probes, we create the bridge sub-device.

4. When the bridge probe runs, comms will still be enabled because the
"pm_runtime" reference keeps them on.

...there's also the issue that we use "autosuspend" and thus comms can
still be left on for a chunk of time even after there are no
"pm_runtime" references left.

-Doug