[PATCH 0/2] Fix nested sleep in adv7511 driver

[PATCH 0/2] Fix nested sleep in adv7511 driver

[Laurent Pinchart]

Hello,

This patch set fixes a nested sleep issue in the adv7511 driver. The rationale
is explained in http://lwn.net/Articles/628628/.

The first patch fixes a small DDC error interrupt bug in the driver, and the
second patch then addresses the nested sleep problem.

Laurent Pinchart (2):
  drm: adv7511: Fix DDC error interrupt reset
  drm: adv7511: Fix nested sleep when reading EDID

 drivers/gpu/drm/i2c/adv7511.c | 100 +++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 50 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[PATCH 1/2] drm: adv7511: Fix DDC error interrupt reset

[Laurent Pinchart]

The DDC error interrupt bit is located in REG_INT1, not REG_INT0. Update
the interrupt sources reset code accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/i2c/adv7511.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
index fa140e04d5fa..b082693ce709 100644
--- a/drivers/gpu/drm/i2c/adv7511.c
+++ b/drivers/gpu/drm/i2c/adv7511.c
@@ -528,7 +528,9 @@ static int adv7511_get_modes(struct drm_encoder *encoder,
 	/* Reading the EDID only works if the device is powered */
 	if (adv7511->dpms_mode != DRM_MODE_DPMS_ON) {
 		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
-			     ADV7511_INT0_EDID_READY | ADV7511_INT1_DDC_ERROR);
+			     ADV7511_INT0_EDID_READY);
+		regmap_write(adv7511->regmap, ADV7511_REG_INT(1),
+			     ADV7511_INT1_DDC_ERROR);
 		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
 				   ADV7511_POWER_POWER_DOWN, 0);
 		adv7511->current_edid_segment = -1;
@@ -563,7 +565,9 @@ static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
 		adv7511->current_edid_segment = -1;
 
 		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
-			     ADV7511_INT0_EDID_READY | ADV7511_INT1_DDC_ERROR);
+			     ADV7511_INT0_EDID_READY);
+		regmap_write(adv7511->regmap, ADV7511_REG_INT(1),
+			     ADV7511_INT1_DDC_ERROR);
 		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
 				   ADV7511_POWER_POWER_DOWN, 0);
 		/*
-- 
2.0.5

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

[PATCH 2/2] drm: adv7511: Fix nested sleep when reading EDID

[Laurent Pinchart]

The EDID read code waits for the read completion interrupt to occur
using wait_event_interruptible(). The condition passed to the macro
reads I2C registers. This results in sleeping with the task state set
to TASK_INTERRUPTIBLE, triggering a WARN_ON() introduced in commit
8eb23b9f35aae ("sched: Debug nested sleeps").

Fix this by reworking the EDID read code. Instead of checking whether
the read is complete through I2C reads, handle the interrupt registers
in the interrupt handler and update a new edid_read flag accordingly. As
a side effect both the IRQ and polling code paths now process the
interrupt sources through the same code path, simplifying the code.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/i2c/adv7511.c | 92 +++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
index b082693ce709..7bd25f069f9f 100644
--- a/drivers/gpu/drm/i2c/adv7511.c
+++ b/drivers/gpu/drm/i2c/adv7511.c
@@ -33,6 +33,7 @@ struct adv7511 {
 
 	unsigned int current_edid_segment;
 	uint8_t edid_buf[256];
+	bool edid_read;
 
 	wait_queue_head_t wq;
 	struct drm_encoder *encoder;
@@ -379,69 +380,69 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
 	return false;
 }
 
-static irqreturn_t adv7511_irq_handler(int irq, void *devid)
-{
-	struct adv7511 *adv7511 = devid;
-
-	if (adv7511_hpd(adv7511))
-		drm_helper_hpd_irq_event(adv7511->encoder->dev);
-
-	wake_up_all(&adv7511->wq);
-
-	return IRQ_HANDLED;
-}
-
-static unsigned int adv7511_is_interrupt_pending(struct adv7511 *adv7511,
-						 unsigned int irq)
+static int adv7511_irq_process(struct adv7511 *adv7511)
 {
 	unsigned int irq0, irq1;
-	unsigned int pending;
 	int ret;
 
 	ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
 	if (ret < 0)
-		return 0;
+		return ret;
+
 	ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(1), &irq1);
 	if (ret < 0)
-		return 0;
+		return ret;
+
+	regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
+	regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
 
-	pending = (irq1 << 8) | irq0;
+	if (irq0 & ADV7511_INT0_HDP)
+		drm_helper_hpd_irq_event(adv7511->encoder->dev);
+
+	if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
+		adv7511->edid_read = true;
+		wake_up_all(&adv7511->wq);
+	}
 
-	return pending & irq;
+	return 0;
 }
 
-static int adv7511_wait_for_interrupt(struct adv7511 *adv7511, int irq,
-				      int timeout)
+static irqreturn_t adv7511_irq_handler(int irq, void *devid)
+{
+	struct adv7511 *adv7511 = devid;
+	int ret;
+
+	ret = adv7511_irq_process(adv7511);
+	return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
+}
+
+/* -----------------------------------------------------------------------------
+ * EDID retrieval
+ */
+
+static int adv7511_wait_for_edid(struct adv7511 *adv7511, int timeout)
 {
-	unsigned int pending;
 	int ret;
 
 	if (adv7511->i2c_main->irq) {
 		ret = wait_event_interruptible_timeout(adv7511->wq,
-				adv7511_is_interrupt_pending(adv7511, irq),
-				msecs_to_jiffies(timeout));
-		if (ret <= 0)
-			return 0;
-		pending = adv7511_is_interrupt_pending(adv7511, irq);
+				adv7511->edid_read, msecs_to_jiffies(timeout));
 	} else {
-		if (timeout < 25)
-			timeout = 25;
-		do {
-			pending = adv7511_is_interrupt_pending(adv7511, irq);
-			if (pending)
+		for (; timeout > 0; timeout -= 25) {
+			ret = adv7511_irq_process(adv7511);
+			if (ret < 0)
 				break;
+
+			if (adv7511->edid_read)
+				break;
+
 			msleep(25);
-			timeout -= 25;
-		} while (timeout >= 25);
+		}
 	}
 
-	return pending;
+	return adv7511->edid_read ? 0 : -EIO;
 }
 
-/* -----------------------------------------------------------------------------
- * EDID retrieval
- */
-
 static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 				  size_t len)
 {
@@ -463,19 +464,14 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 			return ret;
 
 		if (status != 2) {
+			adv7511->edid_read = false;
 			regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT,
 				     block);
-			ret = adv7511_wait_for_interrupt(adv7511,
-					ADV7511_INT0_EDID_READY |
-					ADV7511_INT1_DDC_ERROR, 200);
-
-			if (!(ret & ADV7511_INT0_EDID_READY))
-				return -EIO;
+			ret = adv7511_wait_for_edid(adv7511, 200);
+			if (ret < 0)
+				return ret;
 		}
 
-		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
-			     ADV7511_INT0_EDID_READY | ADV7511_INT1_DDC_ERROR);
-
 		/* Break this apart, hopefully more I2C controllers will
 		 * support 64 byte transfers than 256 byte transfers
 		 */
-- 
2.0.5

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