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@xxxxxxxx>
> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
> Cc: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
> 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);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
