| Message ID | 20210117002355.435860-4-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/bridge/lontium-lt9611uxc: fix handling of EDID/HPD | expand |
Hi Dmitry, 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@linaro.org> > Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge") > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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. > + 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?). > +} > + > 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 !!. I saw in v2 there was R-B tags added by Bjorn to other two patches, please do not forgot them next time. Regards 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); >
Hello, On Thu, 21 Jan 2021 at 14:45, Andrzej Hajda <a.hajda@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@linaro.org> > > Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge") > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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
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)); + 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); +} + 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)); } + 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);