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

[Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>]

Hi Doug,

Quoting Doug Anderson (2022-03-10 23:10:12)
> Hi,
> 
> On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> >         pm_runtime_put_sync(pdata->dev);
> >  }
> >
> > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> > +{
> > +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +       int val;
> > +
> > +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> 
> Don't you need a pm_runtime_get_sync() before this and a
> put_autosuspend() after? The "detect" will be used in the yes-HPD but
> no-IRQ case, right? In that case there's nobody holding the pm_runtime
> reference.

Hrm ... I'll have to dig on this a bit. The polling is done by the DRM
core, so indeed I suspect it could be done outside of a context that
holds the pm runtime reference.

Equally a get and put on the reference doesn't hurt even if it's already
taken, so perhaps it's best to add it, but I'll try to confirm it's
requirement first.


> Also, a nit that it'd be great if you error checked the regmap_read().
> I know this driver isn't very good about it, but it's probably
> something to get better. i2c transactions can fail. I guess another
> alternative would be to init "val" to 0...

It's a good point indeed. If we can't read the device we should return
disconnected.

> 
> 
> > +       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> > +                                        : connector_status_disconnected;
> > +}
> > +
> > +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> > +{
> > +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > +       /* The device must remain active for HPD to function */
> > +       pm_runtime_get_sync(pdata->dev);
> > +       regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> > +                    IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> > +                    IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> > +}
> > +
> > +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?

> > @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> >         return 0;
> >  }
> >
> > +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> > +{
> > +       struct ti_sn65dsi86 *pdata = arg;
> > +       int ret;
> > +       unsigned int hpd;
> > +
> > +       ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> > +       if (ret || !hpd)
> > +               return IRQ_NONE;
> > +
> > +       if (hpd & IRQ_HPD_INSERTION_STATUS)
> > +               drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> > +
> > +       if (hpd & IRQ_HPD_REMOVAL_STATUS)
> > +               drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> > +
> > +       /* When replugged, ensure we trigger a detect to update the display */
> > +       if (hpd & IRQ_HPD_REPLUG_STATUS)
> > +               drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> 
> How does the ordering work here if _both_ insertion and removal are
> asserted? Is that somehow not possible? Should this be "else if" type
> statements then, or give a warn if more than one bit is set, or ... ?

As I understand it, that would trigger a REPLUG IRQ. However this is one
part I quite disliked about the drm_bridge_hpd_notify. The values here
are not taken as the hardware state anyway. A call to drm_bridge_hpd_notify will 
trigger a call on the detect function so a further read will occur to
determine the current state using the same function as is used with
polling.

The IRQ handler only cuts out the polling as far as I see.


> > +       /* reset the status registers */
> > +       regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> > +                    IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> > +                    IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
> 
> IMO this regmap_write() belongs right after the read and should be
> based on what you read--you shouldn't just clear all of them. AKA:
> 
> a) Read to see what interrupt are asserted.
> b) Ack the interrupts that you saw asserted.
> c) Process the interrupts that you saw asserted.
> 
> If you process before acking then you can miss interrupts (in other
> words if you do "a" then "c" then "b" then you can miss interrupts
> that come in after "b" but before "c".

Agreed, I'll respin.

> > @@ -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.

> 
> -Doug