Re: [PATCH 2/2] drm/bridge/lontium-lt9611uxc: move HPD notification out of IRQ handler

[Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>]

On Fri 27 Nov 03:23 CST 2020, Dmitry Baryshkov wrote:

> drm hotplug handling code (drm_client_dev_hotplug()) can wait on mutex,
> thus delaying further lt9611uxc IRQ events processing.  It was observed
> occasionally during bootups, when drm_client_modeset_probe() was waiting
> for EDID ready event, which was delayed because IRQ handler was stuck
> trying to deliver hotplug event.
> Move hotplug notifications from IRQ handler to separate work to be able
> to process IRQ events without delays.
> 

I see a couple of other drivers doing the same, and the patch looks
good.

Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>

Regards,
Bjorn

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 30 +++++++++++++++++-----
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> index b708700e182d..88630bc2921f 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> @@ -14,6 +14,7 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/wait.h>
> +#include <linux/workqueue.h>
>  
>  #include <sound/hdmi-codec.h>
>  
> @@ -36,6 +37,7 @@ struct lt9611uxc {
>  	struct mutex ocm_lock;
>  
>  	struct wait_queue_head wq;
> +	struct work_struct work;
>  
>  	struct device_node *dsi0_node;
>  	struct device_node *dsi1_node;
> @@ -52,6 +54,7 @@ struct lt9611uxc {
>  
>  	bool hpd_supported;
>  	bool edid_read;
> +	bool hdmi_connected;
>  	uint8_t fw_version;
>  };
>  
> @@ -151,15 +154,26 @@ static irqreturn_t lt9611uxc_irq_thread_handler(int irq, void *dev_id)
>  	}
>  
>  	if (irq_status & BIT(1)) {
> -		if (lt9611uxc->connector.dev)
> -			drm_kms_helper_hotplug_event(lt9611uxc->connector.dev);
> -		else
> -			drm_bridge_hpd_notify(&lt9611uxc->bridge, !!(hpd_status & BIT(1)));
> +		lt9611uxc->hdmi_connected = !!(hpd_status & BIT(1));
> +		schedule_work(&lt9611uxc->work);
>  	}
>  
>  	return IRQ_HANDLED;
>  }
>  
> +void lt9611uxc_hpd_work(struct work_struct *work)
> +{
> +	struct lt9611uxc *lt9611uxc = container_of(work, struct lt9611uxc, work);
> +
> +	if (lt9611uxc->connector.dev)
> +		drm_kms_helper_hotplug_event(lt9611uxc->connector.dev);
> +	else
> +		drm_bridge_hpd_notify(&lt9611uxc->bridge,
> +				      lt9611uxc->hdmi_connected ?
> +				      connector_status_connected :
> +				      connector_status_disconnected);
> +}
> +
>  static void lt9611uxc_reset(struct lt9611uxc *lt9611uxc)
>  {
>  	gpiod_set_value_cansleep(lt9611uxc->reset_gpio, 1);
> @@ -447,7 +461,7 @@ static enum drm_connector_status lt9611uxc_bridge_detect(struct drm_bridge *brid
>  	struct lt9611uxc *lt9611uxc = bridge_to_lt9611uxc(bridge);
>  	unsigned int reg_val = 0;
>  	int ret;
> -	int connected = 1;
> +	bool connected = true;
>  
>  	if (lt9611uxc->hpd_supported) {
>  		lt9611uxc_lock(lt9611uxc);
> @@ -457,8 +471,9 @@ static enum drm_connector_status lt9611uxc_bridge_detect(struct drm_bridge *brid
>  		if (ret)
>  			dev_err(lt9611uxc->dev, "failed to read hpd status: %d\n", ret);
>  		else
> -			connected  = reg_val & BIT(1);
> +			connected  = !!(reg_val & BIT(1));
>  	}
> +	lt9611uxc->hdmi_connected = connected;
>  
>  	return connected ?  connector_status_connected :
>  				connector_status_disconnected;
> @@ -931,6 +946,8 @@ static int lt9611uxc_probe(struct i2c_client *client,
>  	lt9611uxc->fw_version = ret;
>  
>  	init_waitqueue_head(&lt9611uxc->wq);
> +	INIT_WORK(&lt9611uxc->work, lt9611uxc_hpd_work);
> +
>  	ret = devm_request_threaded_irq(dev, client->irq, NULL,
>  					lt9611uxc_irq_thread_handler,
>  					IRQF_ONESHOT, "lt9611uxc", lt9611uxc);
> @@ -967,6 +984,7 @@ static int lt9611uxc_remove(struct i2c_client *client)
>  	struct lt9611uxc *lt9611uxc = i2c_get_clientdata(client);
>  
>  	disable_irq(client->irq);
> +	flush_scheduled_work();
>  	lt9611uxc_audio_exit(lt9611uxc);
>  	drm_bridge_remove(&lt9611uxc->bridge);
>  
> -- 
> 2.29.2
>