Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection

[Doug Anderson <dianders@chromium.org>]

Hi,

On Thu, Mar 10, 2022 at 9:47 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> > > +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> > > +{
> > > +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > > +
> > > +       regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> > > +       pm_runtime_put_autosuspend(pdata->dev);
> >
> > Before doing the pm_runtime_put_autosuspend() it feels like you should
> > ensure that the interrupt has finished. Otherwise we could be midway
> > through processing an interrupt and the pm_runtime reference could go
> > away, right? Maybe we just disable the irq which I think will wait for
> > anything outstanding to finish?
>
> Should the IRQ handler also call pm_runtime_get/put then?

I thought about that, but I suspect it's cleaner to disable the IRQ
handler (and block waiting for it to finish if it was running). That
will ensure that the core isn't notified about HPD after HPD was
disabled.  Once you do that then there's no need to get/put in the irq
handler since we always hold a pm_runtime reference when the IRQ
handler is enabled.


> > > @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > >         pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> > >                            ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> > >
> > > -       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> > >                 pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > >
> > > +               if (!pdata->no_hpd)
> > > +                       pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> > > +       }
> > > +
> > > +       if (!pdata->no_hpd && pdata->irq > 0) {
> > > +               dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
> > > +
> > > +               ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> > > +                                               ti_sn65dsi86_irq_handler,
> > > +                                               IRQF_ONESHOT, "sn65dsi86-irq",
> > > +                                               pdata);
> > > +               if (ret)
> > > +                       return dev_err_probe(pdata->dev, ret,
> > > +                                            "Failed to register DP interrupt\n");
> > > +
> > > +               /* Enable IRQ based HPD */
> > > +               regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
> >
> > Why not put the above regmap_write() in the ti_sn_bridge_hpd_enable() call?
>
> I assumed the IRQ handler may get used by other non-HPD events. Which is
> also why it was originally registered in the main probe(). HPD is just
> one feature of the interrupts. Of course it's only used for HPD now
> though. I guess I could have solved the bridge dependency by splitting
> the IRQ handler to have a dedicated HPD handler function which would
> return if the bridge wasn't initialised, but went with the deferred
> registration of the handler.
>
> I can move this and then leave it to anyone else implementing further
> IRQ features to refactor if needed.

Sounds good. In general the pm_runtime_get reference need to go with
the IRQ enabling, so if someone else finds a non-HPD need then they'll
have to move that too.