adv7511 EDID probing improvements

I was recently seeing issues with EDID probing, where the logic to wait for the EDID read bit to be set by the IRQ wasn't happening and the code would time out and fail.

Digging deeper, I found this was due to the fact that IRQs were disabled as we were running in IRQ context from the HPD signal.

Thus this patch changes the logic to handle the HPD signal via a work_struct so we can be out of irq context.

With this patch, the EDID probing on hotplug does not time out.

Cc David Airlie <[email protected]>

Cc Archit Taneja <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Lars-Peter Clausen <[email protected]>

Cc Laurent Pinchart <[email protected]>

Cc [email protected]

Signed-off-by John Stultz <[email protected]>

---
v2: Reworked to properly fix the issue rather then
just delaying for 200ms

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 992d76c..2a1e722 100644

drivers/gpu/drm/bridge/adv7511/adv7511.h +0 -0

 	struct drm_bridge bridge;
 	struct drm_connector connector;
 

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b38e743 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +3 -1

 static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
 {
 	unsigned int irq0, irq1;

@@ -419,7 +427,7 @@ static int adv7511_irq_process(struct adv7511 *adv7511,
 bool process_hpd) regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
 
 	if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
-		drm_helper_hpd_irq_event(adv7511->connector.dev);
+		schedule_work(&adv7511->irq_work);
 
 	if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
 		adv7511->edid_read = true;

@@ -1006,6 +1014,8 @@ static int adv7511_probe(struct i2c_client *i2c, const
 struct i2c_device_id *id) goto err_i2c_unregister_edid;
 	}
 
+	INIT_WORK(&adv7511->irq_work, adv7511_irq_work);
+
 	if (i2c->irq) {
 		init_waitqueue_head(&adv7511->wq);

Hi John,

Thank you for the patch.

Quoted 34 lines · On Monday 28 Nov 2016 21:04:40 John Stultz wrote:

I was recently seeing issues with EDID probing, where the logic to wait for the EDID read bit to be set by the IRQ wasn't happening and the code would time out and fail.

Digging deeper, I found this was due to the fact that IRQs were disabled as we were running in IRQ context from the HPD signal.

Thus this patch changes the logic to handle the HPD signal via a work_struct so we can be out of irq context.

With this patch, the EDID probing on hotplug does not time out.

Cc David Airlie <[email protected]>

Cc Archit Taneja <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Lars-Peter Clausen <[email protected]>

Cc Laurent Pinchart <[email protected]>

Cc [email protected]

Signed-off-by John Stultz <[email protected]>

---
v2: Reworked to properly fix the issue rather then
just delaying for 200ms

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 992d76c..2a1e722 100644

drivers/gpu/drm/bridge/adv7511/adv7511.h struct adv7511 { · line 321

Quoted 5 lines

drivers/gpu/drm/bridge/adv7511/adv7511.h +2 -0

@@ -317,6 +317,8 @@ struct adv7511 {
      bool edid_read;
 
      wait_queue_head_t wq;
+     struct work_struct irq_work;
+

I'd call this hpd_work.

Quoted 9 lines

drivers/gpu/drm/bridge/adv7511/adv7511.h +0 -0

      struct drm_bridge bridge;
      struct drm_connector connector;
 

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b38e743 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static bool adv7511_hpd(struct adv7511 *adv7511) · line 407

Quoted 6 lines

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +3 -0

@@ -402,6 +402,14 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
      return false;
 }
 
+static void adv7511_irq_work(struct work_struct *work)
+{
+     struct adv7511 *adv7511 = container_of(work, struct adv7511,

irq_work);

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static bool adv7511_hpd(struct adv7511 *adv7511) · line 412

Quoted 5 lines

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +5 -0

+
+     drm_helper_hpd_irq_event(adv7511->connector.dev);
+}
+
+

One blank line is enough.

Apart from that,

Reviewed-by Laurent Pinchart <[email protected]>

In chasing down a previous issue with EDID probing from calling drm_helper_hpd_irq_event() from irq context, Laurent noticed that the DRM documentation suggests that drm_kms_helper_hotplug_event() should be used instead.

Thus this patch replaces drm_helper_hpd_irq_event() with drm_kms_helper_hotplug_event().

Cc David Airlie <[email protected]>

Cc Archit Taneja <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Lars-Peter Clausen <[email protected]>

Cc Laurent Pinchart <[email protected]>

Cc [email protected]

Signed-off-by John Stultz <[email protected]>

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index b38e743..2caca0c 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +0 -0

 }

Hi John,

Thank you for the patch.

Quoted 25 lines · On Monday 28 Nov 2016 21:04:41 John Stultz wrote:

In chasing down a previous issue with EDID probing from calling drm_helper_hpd_irq_event() from irq context, Laurent noticed that the DRM documentation suggests that drm_kms_helper_hotplug_event() should be used instead.

Thus this patch replaces drm_helper_hpd_irq_event() with drm_kms_helper_hotplug_event().

Cc David Airlie <[email protected]>

Cc Archit Taneja <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Lars-Peter Clausen <[email protected]>

Cc Laurent Pinchart <[email protected]>

Cc [email protected]

Signed-off-by John Stultz <[email protected]>

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index b38e743..2caca0c 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static void adv7511_irq_work(struct work_struct *work) · line 407

Quoted 2 lines

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +0 -0

@@ -406,7 +406,7 @@ static void adv7511_irq_work(struct work_struct *work)
 {
      struct adv7511 *adv7511 = container_of(work, struct adv7511,

irq_work);

Quoted 1 line

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static void adv7511_irq_work(struct work_struct *work) · line 409

Quoted 2 lines

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +1 -1

-     drm_helper_hpd_irq_event(adv7511->connector.dev);
+     drm_kms_helper_hotplug_event(adv7511->connector.dev);

That's nice, but you must first update adv7511->connector.status (and as an optimization only call drm_kms_helper_hotplug_event() if the status has changed).

I've found that by just turning the chip on and off via the POWER_DOWN register, I end up getting i2c_transfer errors on HiKey.

Investigating further, it seems some of the register state in the regmap cache is somehow getting lost. Using the logic in __adv7511_power_on/off() which syncs and dirtys the cache avoids this issue.

Thus this patch changes the EDID probing logic so that we re-use the __adv7511_power_on/off() calls.

Cc David Airlie <[email protected]>

Cc Archit Taneja <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Lars-Peter Clausen <[email protected]>

Cc Laurent Pinchart <[email protected]>

Cc [email protected]

Signed-off-by John Stultz <[email protected]>

---
v2: Split into two patches to make the change more clear.
Also provided more rational as to why the change is important.

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +0 -3

index 1948968..487b33d 100644
@@ -559,24 +559,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 	unsigned int count;
 
 	/* Reading the EDID only works if the device is powered */
-	if (!adv7511->powered) {
-		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-				   ADV7511_POWER_POWER_DOWN, 0);

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +1 -3

 	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
 
 	if (!adv7511->powered)
-		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-				   ADV7511_POWER_POWER_DOWN,
-				   ADV7511_POWER_POWER_DOWN);
+		__adv7511_power_off(adv7511);
 
 	kfree(adv7511->edid);
 	adv7511->edid = edid;
 

Wanted to send out v2 of this patch set improving the EDID probing on the adv7511 used on HiKey.

The first three patches are fixups that are hopefully straight forward, integrating feedback I got from Laurant.

One of the previous patches that Laurant had concerns about, I broke into two patches, which are the last two in this series. The core issue seems to be something going wrong with the regmap cache state for the ADV7511_REG_EDID_I2C_ADDR (0x43) register which results in i2c_transfer errors if we don't do the regcache_sync/_mark_dirty() calls. I suspect there might be a better solution there, so suggestions will be very welcome.

Thoughts and feedback would be appreciated!

thanks
-john

Cc David Airlie <[email protected]>

Cc Archit Taneja <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Lars-Peter Clausen <[email protected]>

Cc Laurent Pinchart <[email protected]>

Cc [email protected]

Archit Taneja (1):
drm/bridge: adv7511: Enable HPD interrupts to support hotplug and
improve monitor detection

John Stultz (4):
drm/bridge: adv7511: Use work_struct to defer hotplug handing to out
of irq context
drm/bridge: adv7511: Switch to using drm_kms_helper_hotplug_event() drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be
reused internally
drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 49 ++++++++++++++++------------
 2 files changed, 31 insertions(+), 20 deletions(-)

Hi,

Quoted 15 lines · John Stultz

Wanted to send out v2 of this patch set improving the EDID probing on the adv7511 used on HiKey.

The first three patches are fixups that are hopefully straight forward, integrating feedback I got from Laurant.

One of the previous patches that Laurant had concerns about, I broke into two patches, which are the last two in this series. The core issue seems to be something going wrong with the regmap cache state for the ADV7511_REG_EDID_I2C_ADDR (0x43) register which results in i2c_transfer errors if we don't do the regcache_sync/_mark_dirty() calls. I suspect there might be a better solution there, so suggestions will be very welcome.

Quoted 1 line

Thoughts and feedback would be appreciated!

I tested this for ADV7533 on DB410c and it works well. We can pull this if it works fine for ADV7511 too. Laurent, could we get an Ack for the series from you?