Re: [RFC][PATCH 1/5 v2] drm/bridge: adv7511: Use work_struct to defer hotplug handing to out of irq context

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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);

-- 
Regards,

Laurent Pinchart