adv7511 EDID probing improvements
Subsystems (3)
v3 cover adv7511 EDID probing improvements
Wanted to send out v3 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.
The last two patches try to clean up and resue code to avoid 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, so suggestions will be very welcome.
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.
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(-)
Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
RFC PATCH v3 1/5 drm/bridge: adv7511: Use work_struct to defer hotplug handing to out of irq context
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.
---
v3: Rename irq_work to hpd_work and remove extra whitespace, as
suggested by Laurent
--- drivers/gpu/drm/bridge/adv7511/adv7511.h | 2 ++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 11 ++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-)
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 hpd_work; +struct drm_bridge bridge;struct drm_connector connector;
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +10 -1
@@ -402,6 +402,13 @@ static bool adv7511_hpd(struct adv7511 *adv7511)return false; } +static void adv7511_hpd_work (struct work_struct *work) +{ + struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_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 +426,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->hpd_work);if (irq0 &ADV7511_INT0_EDID_READY || irq1 &ADV7511_INT1_DDC_ERROR ) { adv7511->edid_read = true;
@@ -1006,6 +1013,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) goto err_i2c_unregister_edid; } +INIT_WORK (&adv7511->hpd_work, adv7511_hpd_work); +if (i2c->irq) {init_waitqueue_head (&adv7511->wq);
Signature · 1 line
2.7.4
RFC PATCH v3 2/5 drm/bridge: adv7511: Switch to using drm_kms_helper_hotplug_event()
In reply to John StultzIn 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.
---
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(-)
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +15 -1
@@ -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); +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); -drm_helper_hpd_irq_event (adv7511->connector.dev); + adv7511->connector.status = status; }static int adv7511_irq_process (struct adv7511 *adv7511,bool process_hpd)
Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
RFC PATCH v3 3/5 drm/bridge: adv7511: Enable HPD interrupts to support hotplug and improve monitor detection
In reply to John StultzFrom: Archit Taneja <[email protected]>
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.
--- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +5 -1
@@ -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 ); }
@@ -846,6 +846,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; }
Signature · 1 line
2.7.4
RFC PATCH v3 4/5 drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally
In reply to John StultzIn 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.
--- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +10 -4
@@ -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; }
Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
RFC PATCH v3 5/5 drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID
In reply to John StultzI'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.
--- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +3 -14
@@ -572,24 +572,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;
Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel