[PATCH v3 0/3] drm/bridge/lontium-lt9611uxc: fix handling of EDID/HPD

[PATCH v3 0/3] drm/bridge/lontium-lt9611uxc: fix handling of EDID/HPD

[Dmitry Baryshkov]

These three patches provide fixes for HPD handling and EDID readout for
Lontium lt9611uxc DSI-to-HDMI bridge driver.

Changes since v2:
 - Declare lt9611uxc_hpd_work as static

Changes since v1:
 - Split first patch into two smaller patches
 - Add Fixes tags

Dmitry Baryshkov (3):
      drm/bridge/lontium-lt9611uxc: fix waiting for EDID to become available
      drm/bridge/lontium-lt9611uxc: fix get_edid return code
      drm/bridge/lontium-lt9611uxc: move HPD notification out of IRQ handler

 drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 41 +++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 9 deletions(-)

[PATCH v3 0/3] drm/bridge/lontium-lt9611uxc: fix handling of EDID/HPD

[Dmitry Baryshkov]

These three patches provide fixes for HPD handling and EDID readout for
Lontium lt9611uxc DSI-to-HDMI bridge driver.

Changes since v2:
 - Declare lt9611uxc_hpd_work as static

Changes since v1:
 - Split first patch into two smaller patches
 - Add Fixes tags

Dmitry Baryshkov (3):
      drm/bridge/lontium-lt9611uxc: fix waiting for EDID to become available
      drm/bridge/lontium-lt9611uxc: fix get_edid return code
      drm/bridge/lontium-lt9611uxc: move HPD notification out of IRQ handler

 drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 41 +++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 9 deletions(-)


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/3] drm/bridge/lontium-lt9611uxc: fix waiting for EDID to become available

[Dmitry Baryshkov]

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

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
---
 drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
index 0c98d27f84ac..a59e811f1705 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(&lt9611uxc->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)
-- 
2.29.2

[PATCH v3 1/3] drm/bridge/lontium-lt9611uxc: fix waiting for EDID to become available

[Dmitry Baryshkov]

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

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
---
 drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
index 0c98d27f84ac..a59e811f1705 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(&lt9611uxc->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)
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/3] drm/bridge/lontium-lt9611uxc: fix get_edid return code

[Dmitry Baryshkov]

Return NULL pointer from get_edid() callback rather than ERR_PTR()
pointer, as DRM code does NULL checks rather than IS_ERR(). Also while
we are at it, return NULL if getting EDID timed out.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
---
 drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
index a59e811f1705..b708700e182d 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
@@ -505,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

[PATCH v3 2/3] drm/bridge/lontium-lt9611uxc: fix get_edid return code

[Dmitry Baryshkov]

Return NULL pointer from get_edid() callback rather than ERR_PTR()
pointer, as DRM code does NULL checks rather than IS_ERR(). Also while
we are at it, return NULL if getting EDID timed out.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
---
 drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
index a59e811f1705..b708700e182d 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
@@ -505,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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dmitry Baryshkov]

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(&lt9611uxc->bridge, !!(hpd_status & BIT(1)));
+		lt9611uxc->hdmi_connected = !!(hpd_status & BIT(1));
+		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);
+}
+
 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(&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);
 
-- 
2.29.2

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

[Dmitry Baryshkov]

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(&lt9611uxc->bridge, !!(hpd_status & BIT(1)));
+		lt9611uxc->hdmi_connected = !!(hpd_status & BIT(1));
+		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);
+}
+
 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(&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);
 
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrzej Hajda]

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(&lt9611uxc->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(&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?).


> +}
> +
>   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(&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);
>   

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

[Andrzej Hajda]

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(&lt9611uxc->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(&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?).


> +}
> +
>   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(&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);
>   
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dmitry Baryshkov]

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(&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

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

[Dmitry Baryshkov]

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(&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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel