[PATCH v3 3/3] drm/bridge/lontium-lt9611uxc: move HPD notification out of IRQ handler
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Thu Jan 21 23:10:15 UTC 2021
Hello,
On Thu, 21 Jan 2021 at 14:45, Andrzej Hajda <a.hajda at samsung.com> 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 at linaro.org>
> > Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
> > Reviewed-by: Bjorn Andersson <bjorn.andersson at linaro.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > ---
> > 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(<9611uxc->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(<9611uxc->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(<9611uxc->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(<9611uxc->wq);
> > + INIT_WORK(<9611uxc->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(<9611uxc->bridge);
> >
--
With best wishes
Dmitry
More information about the dri-devel
mailing list