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

[Andrzej Hajda]

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.barysh...@linaro.org>
> Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
> Reviewed-by: Bjorn Andersson <bjorn.anders...@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
> ---
>   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel