- Call wake_up() when EDID ready event is received to wake wait_event_interruptible_timeout()
- Increase waiting timeout, reading EDID can take longer than 100ms, so let's be on a safe side.
- Return NULL pointer from get_edid() callback rather than ERR_PTR() pointer, as DRM code does NULL checks rather than IS_ERR().
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c index 0c98d27f84ac..b708700e182d 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c @@ -145,8 +145,10 @@ static irqreturn_t lt9611uxc_irq_thread_handler(int irq, void *dev_id)
lt9611uxc_unlock(lt9611uxc);
- if (irq_status & BIT(0)) + if (irq_status & BIT(0)) { lt9611uxc->edid_read = !!(hpd_status & BIT(0)); + wake_up_all(<9611uxc->wq); + }
if (irq_status & BIT(1)) { if (lt9611uxc->connector.dev) @@ -465,7 +467,7 @@ static enum drm_connector_status lt9611uxc_bridge_detect(struct drm_bridge *brid static int lt9611uxc_wait_for_edid(struct lt9611uxc *lt9611uxc) { return wait_event_interruptible_timeout(lt9611uxc->wq, lt9611uxc->edid_read, - msecs_to_jiffies(100)); + msecs_to_jiffies(500)); }
static int lt9611uxc_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) @@ -503,7 +505,10 @@ static struct edid *lt9611uxc_bridge_get_edid(struct drm_bridge *bridge, ret = lt9611uxc_wait_for_edid(lt9611uxc); if (ret < 0) { dev_err(lt9611uxc->dev, "wait for EDID failed: %d\n", ret); - return ERR_PTR(ret); + return NULL; + } else if (ret == 0) { + dev_err(lt9611uxc->dev, "wait for EDID timeout\n"); + return NULL; }
return drm_do_get_edid(connector, lt9611uxc_get_edid_block, lt9611uxc);
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 --- 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..88630bc2921f 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; }
+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);
On Fri 27 Nov 03:23 CST 2020, Dmitry Baryshkov wrote:
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.
I see a couple of other drivers doing the same, and the patch looks good.
Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org
Regards, Bjorn
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..88630bc2921f 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;
}
+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);
-- 2.29.2
Gracious ping for these two patches
On Fri, 27 Nov 2020 at 12:23, Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
Call wake_up() when EDID ready event is received to wake wait_event_interruptible_timeout()
Increase waiting timeout, reading EDID can take longer than 100ms, so let's be on a safe side.
Return NULL pointer from get_edid() callback rather than ERR_PTR() pointer, as DRM code does NULL checks rather than IS_ERR().
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c index 0c98d27f84ac..b708700e182d 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c @@ -145,8 +145,10 @@ static irqreturn_t lt9611uxc_irq_thread_handler(int irq, void *dev_id)
lt9611uxc_unlock(lt9611uxc);
if (irq_status & BIT(0))
if (irq_status & BIT(0)) { lt9611uxc->edid_read = !!(hpd_status & BIT(0));
wake_up_all(<9611uxc->wq);
} if (irq_status & BIT(1)) { if (lt9611uxc->connector.dev)
@@ -465,7 +467,7 @@ static enum drm_connector_status lt9611uxc_bridge_detect(struct drm_bridge *brid static int lt9611uxc_wait_for_edid(struct lt9611uxc *lt9611uxc) { return wait_event_interruptible_timeout(lt9611uxc->wq, lt9611uxc->edid_read,
msecs_to_jiffies(100));
msecs_to_jiffies(500));
}
static int lt9611uxc_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) @@ -503,7 +505,10 @@ static struct edid *lt9611uxc_bridge_get_edid(struct drm_bridge *bridge, ret = lt9611uxc_wait_for_edid(lt9611uxc); if (ret < 0) { dev_err(lt9611uxc->dev, "wait for EDID failed: %d\n", ret);
return ERR_PTR(ret);
return NULL;
} else if (ret == 0) {
dev_err(lt9611uxc->dev, "wait for EDID timeout\n");
return NULL; } return drm_do_get_edid(connector, lt9611uxc_get_edid_block, lt9611uxc);
-- 2.29.2
Hi Dmitry,
On Tue, Jan 05, 2021 at 08:20:39PM +0300, Dmitry Baryshkov wrote:
Gracious ping for these two patches
I'm afraid I have very little time for DRM bridge maintenance at the moment :-S Judging by the lack of replies, it seems the other maintainers are also AWOL. We seem to have a shortage of maintainers for this part of the subsystem.
On Fri, 27 Nov 2020 at 12:23, Dmitry Baryshkov wrote:
Call wake_up() when EDID ready event is received to wake wait_event_interruptible_timeout()
Increase waiting timeout, reading EDID can take longer than 100ms, so let's be on a safe side.
Return NULL pointer from get_edid() callback rather than ERR_PTR() pointer, as DRM code does NULL checks rather than IS_ERR().
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c index 0c98d27f84ac..b708700e182d 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c @@ -145,8 +145,10 @@ static irqreturn_t lt9611uxc_irq_thread_handler(int irq, void *dev_id)
lt9611uxc_unlock(lt9611uxc);
if (irq_status & BIT(0))
if (irq_status & BIT(0)) { lt9611uxc->edid_read = !!(hpd_status & BIT(0));
wake_up_all(<9611uxc->wq);
} if (irq_status & BIT(1)) { if (lt9611uxc->connector.dev)
@@ -465,7 +467,7 @@ static enum drm_connector_status lt9611uxc_bridge_detect(struct drm_bridge *brid static int lt9611uxc_wait_for_edid(struct lt9611uxc *lt9611uxc) { return wait_event_interruptible_timeout(lt9611uxc->wq, lt9611uxc->edid_read,
msecs_to_jiffies(100));
msecs_to_jiffies(500));
}
static int lt9611uxc_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) @@ -503,7 +505,10 @@ static struct edid *lt9611uxc_bridge_get_edid(struct drm_bridge *bridge, ret = lt9611uxc_wait_for_edid(lt9611uxc); if (ret < 0) { dev_err(lt9611uxc->dev, "wait for EDID failed: %d\n", ret);
return ERR_PTR(ret);
return NULL;
} else if (ret == 0) {
dev_err(lt9611uxc->dev, "wait for EDID timeout\n");
return NULL; } return drm_do_get_edid(connector, lt9611uxc_get_edid_block, lt9611uxc);
On Fri 27 Nov 03:23 CST 2020, Dmitry Baryshkov wrote:
Call wake_up() when EDID ready event is received to wake wait_event_interruptible_timeout()
Increase waiting timeout, reading EDID can take longer than 100ms, so let's be on a safe side.
Return NULL pointer from get_edid() callback rather than ERR_PTR() pointer, as DRM code does NULL checks rather than IS_ERR().
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c index 0c98d27f84ac..b708700e182d 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c @@ -145,8 +145,10 @@ static irqreturn_t lt9611uxc_irq_thread_handler(int irq, void *dev_id)
lt9611uxc_unlock(lt9611uxc);
- if (irq_status & BIT(0))
- if (irq_status & BIT(0)) { lt9611uxc->edid_read = !!(hpd_status & BIT(0));
wake_up_all(<9611uxc->wq);
This seems like a really good idea :)
}
if (irq_status & BIT(1)) { if (lt9611uxc->connector.dev)
@@ -465,7 +467,7 @@ static enum drm_connector_status lt9611uxc_bridge_detect(struct drm_bridge *brid static int lt9611uxc_wait_for_edid(struct lt9611uxc *lt9611uxc) { return wait_event_interruptible_timeout(lt9611uxc->wq, lt9611uxc->edid_read,
msecs_to_jiffies(100));
msecs_to_jiffies(500));
}
static int lt9611uxc_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) @@ -503,7 +505,10 @@ static struct edid *lt9611uxc_bridge_get_edid(struct drm_bridge *bridge, ret = lt9611uxc_wait_for_edid(lt9611uxc); if (ret < 0) { dev_err(lt9611uxc->dev, "wait for EDID failed: %d\n", ret);
return ERR_PTR(ret);
return NULL;
- } else if (ret == 0) {
dev_err(lt9611uxc->dev, "wait for EDID timeout\n");
return NULL;
This looks like it should be a separate patch.
Regards, Bjorn
}
return drm_do_get_edid(connector, lt9611uxc_get_edid_block, lt9611uxc);
2.29.2
dri-devel@lists.freedesktop.org