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 airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org
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(-)
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 airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org --- 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(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 992d76c..2a1e722 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -317,6 +317,8 @@ struct adv7511 { bool edid_read;
wait_queue_head_t wq; + struct work_struct irq_work; + struct drm_bridge bridge; struct drm_connector connector;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b38e743 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -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); + + drm_helper_hpd_irq_event(adv7511->connector.dev); +} + + 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.
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 airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org
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(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 992d76c..2a1e722 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -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.
struct drm_bridge bridge; struct drm_connector connector;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b38e743 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -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);
- drm_helper_hpd_irq_event(adv7511->connector.dev);
+}
One blank line is enough.
Apart from that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
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);
On Mon, Nov 28, 2016 at 10:46 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi John,
Thank you for the patch.
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 airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org
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(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 992d76c..2a1e722 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -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.
struct drm_bridge bridge; struct drm_connector connector;diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b38e743 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -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);
drm_helper_hpd_irq_event(adv7511->connector.dev);+}
One blank line is enough.
Apart from that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Thanks so much for the review! I've made your suggested changes! -john
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 airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index b38e743..2caca0c 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -406,7 +406,7 @@ static void adv7511_irq_work(struct work_struct *work) { struct adv7511 *adv7511 = container_of(work, struct adv7511, irq_work);
- drm_helper_hpd_irq_event(adv7511->connector.dev); + drm_kms_helper_hotplug_event(adv7511->connector.dev); }
Hi John,
Thank you for the patch.
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 airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index b38e743..2caca0c 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -406,7 +406,7 @@ static void adv7511_irq_work(struct work_struct *work) { struct adv7511 *adv7511 = container_of(work, struct adv7511,
irq_work);
- 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).
}
On Mon, Nov 28, 2016 at 10:50 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi John,
Thank you for the patch.
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 airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index b38e743..2caca0c 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -406,7 +406,7 @@ static void adv7511_irq_work(struct work_struct *work) { struct adv7511 *adv7511 = container_of(work, struct adv7511,
irq_work);
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).
Apologies for not seeing that subtlety. Thanks for pointing it out, I've updated my patch.
thanks -john
From: Archit Taneja architt@codeaurora.org
On some adv7511 implementations, we can get some spurious disconnect signals which can cause monitor probing to fail.
This patch enables HPD (hot plug detect) interrupt support which allows the monitor to be properly re-initialized when the spurious disconnect signal goes away.
This also enables proper hotplug support.
Cc: David Airlie airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Originally-by: Archit Taneja architt@codeaurora.org [jstultz: Added proper commit message] Signed-off-by: John Stultz john.stultz@linaro.org --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 2caca0c..9f8dffd 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -338,7 +338,7 @@ static void adv7511_power_on(struct adv7511 *adv7511) * Still, let's be safe and stick to the documentation. */ regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0), - ADV7511_INT0_EDID_READY); + ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD); regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1), ADV7511_INT1_DDC_ERROR); } @@ -833,6 +833,10 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge) if (adv->type == ADV7533) ret = adv7533_attach_dsi(adv);
+ if (adv->i2c_main->irq) + regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0), + ADV7511_INT0_HPD); + return ret; }
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 airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org --- v2: Split into two patches to make change more clear
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 9f8dffd..1948968 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -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;
@@ -359,24 +359,30 @@ static void adv7511_power_on(struct adv7511 *adv7511) * Most of the registers are reset during power down or when HPD is low. */ 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 airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org --- 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(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 1948968..487b33d 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -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); - if (adv7511->i2c_main->irq) { - regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0), - ADV7511_INT0_EDID_READY); - regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1), - 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;
On 11/29/2016 10:34 AM, 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.
Thus this patch changes the EDID probing logic so that we re-use the __adv7511_power_on/off() calls.
Cc: David Airlie airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org
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(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 1948968..487b33d 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -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);if (adv7511->i2c_main->irq) {regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),ADV7511_INT0_EDID_READY);regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),ADV7511_INT1_DDC_ERROR);}adv7511->current_edid_segment = -1;- }
- if (!adv7511->powered)
__adv7511_power_on(adv7511);
In terms of changes in terms of register configurations, replacing with __adv7511_power_on() here would add a register write to ADV7511_REG_POWER2 as discussed before.
Also, after the patch that enables HPD, we'd also be adding the ADV7511_INT0_HPD bit in ADV7511_REG_INT_ENABLE(0).
I am not entirely sure about what effect adding POWER2 would have. If we don't see any negative effects because of it, I think we should be fine.
The extra HPD bit in INT_ENABLE(0) shouldn't be a problem, though. In fact, it prevents the HPD mask from being cleared after we get_modes is called, which may have resulted in us missing out on HPD interrupts.
Thanks, Archit
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,
On 11/29/2016 10:34 AM, John Stultz wrote:
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!
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?
Thanks, Archit
thanks -john
Cc: David Airlie airlied@linux.ie Cc: Archit Taneja architt@codeaurora.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: dri-devel@lists.freedesktop.org
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(-)
On Mon, Dec 12, 2016 at 12:40 AM, Archit Taneja architt@codeaurora.org wrote:
Hi,
On 11/29/2016 10:34 AM, John Stultz wrote:
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!
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?
I'll be re-sending the series (v3) later today to include fixups suggested by Laurent and others.
thanks -john
dri-devel@lists.freedesktop.org
-
Archit Taneja -
John Stultz -
Laurent Pinchart