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

[Andrzej Hajda <a.hajda@xxxxxxxxxxx>]

Hi Dmitry,

W dniu 17.01.2021 o 01:23, Dmitry Baryshkov pisze:
> 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.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
> Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> 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..209e39923914 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));

No need for !!, int->bool implicit conversion will do the thing.

> +		schedule_work(&lt9611uxc->work);
>   	}
>   
>   	return IRQ_HANDLED;
>   }
>   
> +static 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);


I am little bit worried about accessing lt9611uxc->hdmi_connected - it 
is set in different thread, and there is no explicit sync code between 
them. I guess it is possible to loss proper HPD signal, especially if 
the HPD line is unstable (is there signal debouncing?).


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


Again no no need for !!.

I saw in v2 there was R-B tags added by Bjorn to other two patches, 
please do not forgot them next time.


Regards

Andrzej


>   	}
> +	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);
>   
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel