adv7511 EDID probing improvements

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(), which requires we update the connector.status entry and only call _hotplug_event() when the status changes.

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

---
v3: Update connector.status value and only call __hotplug_event()
when that status changes, as suggested by Laurent.

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 4fcea44..d93d66f 100644

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

+	enum drm_connector_status status;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, &val);
+	if (ret < 0)
+		status = connector_status_disconnected;

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

 }
 
 static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)

Hi John,

Thank you for the patch.

Quoted 31 lines · On Tuesday 03 Jan 2017 11:41:39 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(), which requires we update the connector.status entry and only call _hotplug_event() when the status changes.

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

---
v3: Update connector.status value and only call __hotplug_event()

when that status changes, as suggested by Laurent.

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 4fcea44..d93d66f 100644

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

Quoted 5 lines

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

@@ -405,8 +405,22 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
 
 static void adv7511_hpd_work(struct work_struct *work)
 {
 
 	struct adv7511 *adv7511 = container_of(work, struct adv7511,

hpd_work);

Quoted 15 lines

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

+	enum drm_connector_status status;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, &val);
+	if (ret < 0)
+		status = connector_status_disconnected;
+	else if (val & ADV7511_STATUS_HPD)
+		status = connector_status_connected;
+	else
+		status = connector_status_disconnected;
+
+	if (adv7511->connector.status != status)
+		drm_kms_helper_hotplug_event(adv7511->connector.dev);
 

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

Quoted 2 lines

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

-	drm_helper_hpd_irq_event(adv7511->connector.dev);
+	adv7511->connector.status = status;

Shouldn't you update the status before calling drm_kms_helper_hotplug_event() ? Doing it after not only creates a small race condition as
drm_kms_helper_hotplug_event() sends an event to userspace that could result in an ioctl call to retrieve the status, but the status is also checked by drm_setup_crtcs() called by drm_fb_helper_hotplug_event().

Quoted 3 lines

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

 }

static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)

Quoted 59 lines · On Monday 16 Jan 2017 17:47:46 Laurent Pinchart wrote:

Hi John,

Thank you for the patch.

Quoted 31 lines · On Tuesday 03 Jan 2017 11:41:39 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(), which requires we update the connector.status entry and only call _hotplug_event() when the status changes.

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

---
v3: Update connector.status value and only call __hotplug_event()

when that status changes, as suggested by Laurent.

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 4fcea44..d93d66f 100644

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

Quoted 5 lines

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

@@ -405,8 +405,22 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
 
 static void adv7511_hpd_work(struct work_struct *work)
 {
 
    struct adv7511 *adv7511 = container_of(work, struct adv7511,

hpd_work);

Quoted 15 lines

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

+   enum drm_connector_status status;
+   unsigned int val;
+   int ret;
+
+   ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, &val);
+   if (ret < 0)
+           status = connector_status_disconnected;
+   else if (val & ADV7511_STATUS_HPD)
+           status = connector_status_connected;
+   else
+           status = connector_status_disconnected;
+
+   if (adv7511->connector.status != status)
+           drm_kms_helper_hotplug_event(adv7511->connector.dev);
 

Quoted 9 lines

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

Quoted 2 lines

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

-   drm_helper_hpd_irq_event(adv7511->connector.dev);
+   adv7511->connector.status = status;

Shouldn't you update the status before calling drm_kms_helper_hotplug_event() ? Doing it after not only creates a small race condition as
drm_kms_helper_hotplug_event() sends an event to userspace that could result in an ioctl call to retrieve the status, but the status is also checked by drm_setup_crtcs() called by drm_fb_helper_hotplug_event().

With

        if (adv7511->connector.status != status) {
                adv7511->connector.status = status;
                drm_kms_helper_hotplug_event(adv7511->connector.dev);
        }

Reviewed-by Laurent Pinchart <[email protected]>

Tested-by Laurent Pinchart <[email protected]>

In chasing down issues with EDID probing, I found some duplicated but incomplete logic used to power the chip on and off.

This patch refactors the adv7511_power_on/off functions, so they can be used for internal needs.

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 | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 4b90975..dbdb71c 100644

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

@@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511
 *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
 }
 
-static void adv7511_power_on(struct adv7511 *adv7511)
+static void __adv7511_power_on(struct adv7511 *adv7511)
 {
 	adv7511->current_edid_segment = -1;
 

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

 	 */
 	regcache_sync(adv7511->regmap);
+}
 
+static void adv7511_power_on(struct adv7511 *adv7511)
+{
+	__adv7511_power_on(adv7511);
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_on(adv7511);
-
 	adv7511->powered = true;
 }
 
-static void adv7511_power_off(struct adv7511 *adv7511)
+static void __adv7511_power_off(struct adv7511 *adv7511)
 {
 	/* TODO: setup additional power down modes */
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
 			   ADV7511_POWER_POWER_DOWN,
 			   ADV7511_POWER_POWER_DOWN);
 	regcache_mark_dirty(adv7511->regmap);
+}
 
+static void adv7511_power_off(struct adv7511 *adv7511)
+{
+	__adv7511_power_off(adv7511);
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_off(adv7511);
-
 	adv7511->powered = false;
 }

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

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

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index dbdb71c..24573e0 100644

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

-				     ADV7511_INT1_DDC_ERROR);
-		}
-		adv7511->current_edid_segment = -1;
-	}
+	if (!adv7511->powered)
+		__adv7511_power_on(adv7511);
 
 	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;

Hi John,

Thank you for the patch.

Quoted 9 lines · On Tuesday 03 Jan 2017 11:41:42 John Stultz wrote:

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.

Quoted 2 lines

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

regcache_sync() is quite costly as it will write a bunch of registers. Wouldn't it be more efficient to only write the registers that are needed for EDID access ?

Quoted 10 lines · Laurent Pinchart

Quoted 9 lines · On Tuesday 03 Jan 2017 11:41:42 John Stultz wrote:

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.

Quoted 6 lines

Quoted 2 lines

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

regcache_sync() is quite costly as it will write a bunch of registers. Wouldn't it be more efficient to only write the registers that are needed for EDID access ?

So yes, you've mentioned this concern before, and I did spend some time to narrow which lost-register state (0x43
- ADV7511_REG_EDID_I2C_ADDR) was causing the trouble with i2c
trasnfer errors I was seeing:
https://lkml.org/lkml/2016/11/22/677

However, I didn't get much feedback on that, and it seems (to me at least) concerning that we are losing the underlying state of a register in the cache, so just syncing that one register back to the hardware might solve the issue I was seeing, but I worry what other registers might also be out of sync.

The comment above the regmap_sync in adv7511_power_on after all states:
"Most of the registers are reset during power down or when HPD is low."

So it seems like if we're setting the power down (and setting HPD in cases where Archit had a patch to add HPD pulsing to the adv7511_get_modes path), it seems reasonable to do the same regmap_sync()?

But, I'm not really picky here, and I'm very open to other approaches (including something like the patch in the link above) if you have suggestions/preferences. I just want it to work reliably on my hardware. :)

And just so I can better understand it, can you explain some about the impact of your efficiency concerns?

Laurent: Would something like the following be preferred? Seems to work as well for me..

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, likely as the device registers were reset during power off.

Thus this patch simply re-writes the i2c address to the ADV7511_REG_EDID_I2C_ADDR register to ensure its properly set before we try to read the EDID data.

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 | 5 +++++
 1 file changed, 5 insertions(+)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 405e460..32c59cb 100644

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

 	}
 
 	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);

Hi John,

Thank you for the patch.

Quoted 2 lines · On Monday 16 Jan 2017 15:16:51 John Stultz wrote:

Laurent: Would something like the following be preferred? Seems to work as well for me..

That looks good to me. Feel free to still de-duplicate the power on/off code if you want (but of course without adding the regcache_sync to the common power on function this time).

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static int adv7511_get_modes(struct adv7511 *adv7511, · line 582

Quoted 6 lines

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

@@ -576,6 +578,9 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
                                   ADV7511_INT1_DDC_ERROR);
              }
              adv7511->current_edid_segment = -1;
+
+             /* Reset the EDID_I2C_ADDR register as it may have been

cleared */

Review static int adv7511_get_modes(struct adv7511 *adv7511, · line 583

Quoted 1 line

(quoted diff) +1 -0

+             regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,

edid_i2c_addr);

As powering the device off called regcache_mark_dirty(), this will perform an I2C write and cache the value. If we then try to read the EDID a second time without going through a full power on/off sequence, I believe that regmap will skip the write the second time, as the cache will contain the same value and won't be marked as dirty. Should we call regcache_mark_dirty() when powering the device down further down this function ?

Hope everyone had a good newyears!

Wanted to re-send out v3 of this patch set improving the EDID probing on the adv7511 used on HiKey, for consideration for merging for 4.11

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

The last two patches try to clean up and resue code, which as a side effect avoids an issue I'm seeing where something is 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, but have not gotten any other suggestions so I wanted to go ahead and submit these.

Thoughts and feedback would be appreciated!

thanks
-john

New in v3:
* Addressed naming improvements and drm_kms_helper_hotplug_event usage corrections as suggested by Laurent.

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 | 62 +++++++++++++++++++---------
 2 files changed, 44 insertions(+), 20 deletions(-)

Hi,

Quoted 19 lines · John Stultz

Hope everyone had a good newyears!

Wanted to re-send out v3 of this patch set improving the EDID probing on the adv7511 used on HiKey, for consideration for merging for 4.11

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

The last two patches try to clean up and resue code, which as a side effect avoids an issue I'm seeing where something is 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, but have not gotten any other suggestions so I wanted to go ahead and submit these.

Quoted 1 line

Thoughts and feedback would be appreciated!

Tested on DB410c on 4.10-rc3. Works well for me.

Quoted 22 lines · Archit Taneja

Hi,

Quoted 19 lines · John Stultz

Hope everyone had a good newyears!

Wanted to re-send out v3 of this patch set improving the EDID probing on the adv7511 used on HiKey, for consideration for merging for 4.11

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

The last two patches try to clean up and resue code, which as a side effect avoids an issue I'm seeing where something is 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, but have not gotten any other suggestions so I wanted to go ahead and submit these.

Quoted 4 lines

Quoted 1 line

Thoughts and feedback would be appreciated!

Tested on DB410c on 4.10-rc3. Works well for me.

Archit: Thanks for the testing! Is there anything else you need from me (or others) to queue this for 4.11? Or should I be submitting it to someone else?

thanks
-john

Hope everyone had a good newyears!

Wanted to re-send out v3 of this patch set improving the EDID probing on the adv7511 used on HiKey, for consideration for merging for 4.11

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

The last two patches try to clean up and resue code, which as a side effect avoids an issue I'm seeing where something is 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, but have not gotten any other suggestions so I wanted to go ahead and submit these.

Thoughts and feedback would be appreciated!

New in v3:
* Addressed naming improvements and drm_kms_helper_hotplug_event usage corrections as suggested by Laurent.

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

+++++++++++++++++++--------- 2 files changed, 44 insertions(+), 20 deletions(-)