Re: [PATCH v3 3/3] drm/bridge/lontium-lt9611uxc: move HPD notification out of IRQ handler

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

Hello,

On Thu, 21 Jan 2021 at 14:45, Andrzej Hajda <a.hajda@xxxxxxxxxxx> wrote:
> W dniu 17.01.2021 o 01:23, Dmitry Baryshkov pisze:
> > drm hotplug handling code (drm_client_dev_hotplug()) can wait on mutex,
> > thus delaying further lt9611uxc IRQ events processing.  It was observed
> > occasionally during bootups, when drm_client_modeset_probe() was waiting
> > for EDID ready event, which was delayed because IRQ handler was stuck
> > trying to deliver hotplug event.
> > Move hotplug notifications from IRQ handler to separate work to be able
> > to process IRQ events without delays.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 30 +++++++++++++++++-----
> >   1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> > index b708700e182d..209e39923914 100644
> > --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> > +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> > @@ -14,6 +14,7 @@
> >   #include <linux/regmap.h>
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/wait.h>
> > +#include <linux/workqueue.h>
> >
> >   #include <sound/hdmi-codec.h>
> >
> > @@ -36,6 +37,7 @@ struct lt9611uxc {
> >       struct mutex ocm_lock;
> >
> >       struct wait_queue_head wq;
> > +     struct work_struct work;
> >
> >       struct device_node *dsi0_node;
> >       struct device_node *dsi1_node;
> > @@ -52,6 +54,7 @@ struct lt9611uxc {
> >
> >       bool hpd_supported;
> >       bool edid_read;
> > +     bool hdmi_connected;
> >       uint8_t fw_version;
> >   };
> >
> > @@ -151,15 +154,26 @@ static irqreturn_t lt9611uxc_irq_thread_handler(int irq, void *dev_id)
> >       }
> >
> >       if (irq_status & BIT(1)) {
> > -             if (lt9611uxc->connector.dev)
> > -                     drm_kms_helper_hotplug_event(lt9611uxc->connector.dev);
> > -             else
> > -                     drm_bridge_hpd_notify(&lt9611uxc->bridge, !!(hpd_status & BIT(1)));
> > +             lt9611uxc->hdmi_connected = !!(hpd_status & BIT(1));
>
> No need for !!, int->bool implicit conversion will do the thing.

Ack.

>
> > +             schedule_work(&lt9611uxc->work);
> >       }
> >
> >       return IRQ_HANDLED;
> >   }
> >
> > +static void lt9611uxc_hpd_work(struct work_struct *work)
> > +{
> > +     struct lt9611uxc *lt9611uxc = container_of(work, struct lt9611uxc, work);
> > +
> > +     if (lt9611uxc->connector.dev)
> > +             drm_kms_helper_hotplug_event(lt9611uxc->connector.dev);
> > +     else
> > +             drm_bridge_hpd_notify(&lt9611uxc->bridge,
> > +                                   lt9611uxc->hdmi_connected ?
> > +                                   connector_status_connected :
> > +                                   connector_status_disconnected);
>
>
> I am little bit worried about accessing lt9611uxc->hdmi_connected - it
> is set in different thread, and there is no explicit sync code between
> them. I guess it is possible to loss proper HPD signal, especially if
> the HPD line is unstable (is there signal debouncing?).

I'll protect access to the hdmi_connected by the lt9611uxc_lock/ocm_mutex,

> > +}
> > +
> >   static void lt9611uxc_reset(struct lt9611uxc *lt9611uxc)
> >   {
> >       gpiod_set_value_cansleep(lt9611uxc->reset_gpio, 1);
> > @@ -447,7 +461,7 @@ static enum drm_connector_status lt9611uxc_bridge_detect(struct drm_bridge *brid
> >       struct lt9611uxc *lt9611uxc = bridge_to_lt9611uxc(bridge);
> >       unsigned int reg_val = 0;
> >       int ret;
> > -     int connected = 1;
> > +     bool connected = true;
> >
> >       if (lt9611uxc->hpd_supported) {
> >               lt9611uxc_lock(lt9611uxc);
> > @@ -457,8 +471,9 @@ static enum drm_connector_status lt9611uxc_bridge_detect(struct drm_bridge *brid
> >               if (ret)
> >                       dev_err(lt9611uxc->dev, "failed to read hpd status: %d\n", ret);
> >               else
> > -                     connected  = reg_val & BIT(1);
> > +                     connected  = !!(reg_val & BIT(1));
>
>
> Again no no need for !!.

Ack

>
> I saw in v2 there was R-B tags added by Bjorn to other two patches,
> please do not forgot them next time.

Ack

> Andrzej
>
>
> >       }
> > +     lt9611uxc->hdmi_connected = connected;
> >
> >       return connected ?  connector_status_connected :
> >                               connector_status_disconnected;
> > @@ -931,6 +946,8 @@ static int lt9611uxc_probe(struct i2c_client *client,
> >       lt9611uxc->fw_version = ret;
> >
> >       init_waitqueue_head(&lt9611uxc->wq);
> > +     INIT_WORK(&lt9611uxc->work, lt9611uxc_hpd_work);
> > +
> >       ret = devm_request_threaded_irq(dev, client->irq, NULL,
> >                                       lt9611uxc_irq_thread_handler,
> >                                       IRQF_ONESHOT, "lt9611uxc", lt9611uxc);
> > @@ -967,6 +984,7 @@ static int lt9611uxc_remove(struct i2c_client *client)
> >       struct lt9611uxc *lt9611uxc = i2c_get_clientdata(client);
> >
> >       disable_irq(client->irq);
> > +     flush_scheduled_work();
> >       lt9611uxc_audio_exit(lt9611uxc);
> >       drm_bridge_remove(&lt9611uxc->bridge);
> >



-- 
With best wishes
Dmitry
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel