[PATCH v2] drm/msm/dp: stop event kernel thread when DP unbind

[PATCH v2] drm/msm/dp: stop event kernel thread when DP unbind

[Kuogee Hsieh]

Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().

Changes in v2:
-- start event thread at dp_display_bind()

Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 40 +++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..943e4f1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -113,6 +113,7 @@ struct dp_display_private {
 	u32 hpd_state;
 	u32 event_pndx;
 	u32 event_gndx;
+	struct task_struct *ev_tsk;
 	struct dp_event event_list[DP_EVENT_Q_MAX];
 	spinlock_t event_lock;
 
@@ -230,6 +231,31 @@ void dp_display_signal_audio_complete(struct msm_dp *dp_display)
 	complete_all(&dp->audio_comp);
 }
 
+static int hpd_event_thread(void *data);
+
+static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
+{
+	init_waitqueue_head(&dp_priv->event_q);
+	spin_lock_init(&dp_priv->event_lock);
+
+	dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
+
+	if (IS_ERR(dp_priv->ev_tsk))
+		DRM_ERROR("failed to create DP event thread\n");
+}
+
+static void dp_hpd_event_stop(struct dp_display_private *dp_priv)
+{
+	if (IS_ERR(dp_priv->ev_tsk))
+		return;
+
+	kthread_stop(dp_priv->ev_tsk);
+
+	/* reset event q to empty */
+	dp_priv->event_gndx = 0;
+	dp_priv->event_pndx = 0;
+}
+
 static int dp_display_bind(struct device *dev, struct device *master,
 			   void *data)
 {
@@ -269,6 +295,7 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	if (rc)
 		DRM_ERROR("Audio registration Dp failed\n");
 
+	dp_hpd_event_setup(dp); /* start event thread */
 end:
 	return rc;
 }
@@ -280,6 +307,8 @@ static void dp_display_unbind(struct device *dev, struct device *master,
 	struct drm_device *drm = dev_get_drvdata(master);
 	struct msm_drm_private *priv = drm->dev_private;
 
+	disable_irq(dp->irq);
+	dp_hpd_event_stop(dp); /* stop event thread */
 	dp_power_client_deinit(dp->power);
 	dp_aux_unregister(dp->aux);
 	priv->dp[dp->id] = NULL;
@@ -1054,7 +1083,7 @@ static int hpd_event_thread(void *data)
 
 	dp_priv = (struct dp_display_private *)data;
 
-	while (1) {
+	while (!kthread_should_stop()) {
 		if (timeout_mode) {
 			wait_event_timeout(dp_priv->event_q,
 				(dp_priv->event_pndx == dp_priv->event_gndx),
@@ -1132,13 +1161,6 @@ static int hpd_event_thread(void *data)
 	return 0;
 }
 
-static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
-{
-	init_waitqueue_head(&dp_priv->event_q);
-	spin_lock_init(&dp_priv->event_lock);
-
-	kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
-}
 
 static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
 {
@@ -1441,8 +1463,6 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
-	dp_hpd_event_setup(dp);
-
 	dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] drm/msm/dp: stop event kernel thread when DP unbind

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-04-13 14:04:25)
> Current DP driver implementation, event thread is kept running
> after DP display is unbind. This patch fix this problem by disabling
> DP irq and stop event thread to exit gracefully at dp_display_unbind().
>
> Changes in v2:
> -- start event thread at dp_display_bind()
>
> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 40 +++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 01453db..943e4f1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -113,6 +113,7 @@ struct dp_display_private {
>         u32 hpd_state;
>         u32 event_pndx;
>         u32 event_gndx;
> +       struct task_struct *ev_tsk;
>         struct dp_event event_list[DP_EVENT_Q_MAX];
>         spinlock_t event_lock;
>
> @@ -230,6 +231,31 @@ void dp_display_signal_audio_complete(struct msm_dp *dp_display)
>         complete_all(&dp->audio_comp);
>  }
>
> +static int hpd_event_thread(void *data);

Is there a reason why this is needed vs. defining the kthread start
function after hpd_event_thread()?

> +
> +static void dp_hpd_event_setup(struct dp_display_private *dp_priv)

Maybe dp_hpd_event_thread_start()?

> +{
> +       init_waitqueue_head(&dp_priv->event_q);
> +       spin_lock_init(&dp_priv->event_lock);
> +
> +       dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
> +
> +       if (IS_ERR(dp_priv->ev_tsk))
> +               DRM_ERROR("failed to create DP event thread\n");

Can we return an error from this function?

> +}
> +
> +static void dp_hpd_event_stop(struct dp_display_private *dp_priv)

Maybe dp_hpd_event_thread_stop()?

> +{
> +       if (IS_ERR(dp_priv->ev_tsk))
> +               return;

If we handled the error then this check becomes impossible.

> +
> +       kthread_stop(dp_priv->ev_tsk);
> +
> +       /* reset event q to empty */
> +       dp_priv->event_gndx = 0;
> +       dp_priv->event_pndx = 0;
> +}
> +
>  static int dp_display_bind(struct device *dev, struct device *master,
>                            void *data)
>  {
> @@ -269,6 +295,7 @@ static int dp_display_bind(struct device *dev, struct device *master,
>         if (rc)
>                 DRM_ERROR("Audio registration Dp failed\n");
>
> +       dp_hpd_event_setup(dp); /* start event thread */

The comment is useless, please remove.

>  end:
>         return rc;
>  }
> @@ -280,6 +307,8 @@ static void dp_display_unbind(struct device *dev, struct device *master,
>         struct drm_device *drm = dev_get_drvdata(master);
>         struct msm_drm_private *priv = drm->dev_private;
>
> +       disable_irq(dp->irq);

Is the disable_irq() necessary? It would be nicer to silence the
hardware and remove the disable_irq() so that we can reason about the
code assuming the irq is always enabled after it is requested.

> +       dp_hpd_event_stop(dp); /* stop event thread */
>         dp_power_client_deinit(dp->power);
>         dp_aux_unregister(dp->aux);
>         priv->dp[dp->id] = NULL;

Re: [PATCH v2] drm/msm/dp: stop event kernel thread when DP unbind

[Dmitry Baryshkov]

On 14/04/2022 00:04, Kuogee Hsieh wrote:
> Current DP driver implementation, event thread is kept running
> after DP display is unbind. This patch fix this problem by disabling
> DP irq and stop event thread to exit gracefully at dp_display_unbind().
> 
> Changes in v2:
> -- start event thread at dp_display_bind()
> 
> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 40 +++++++++++++++++++++++++++----------
>   1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 01453db..943e4f1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -113,6 +113,7 @@ struct dp_display_private {
>   	u32 hpd_state;
>   	u32 event_pndx;
>   	u32 event_gndx;
> +	struct task_struct *ev_tsk;
>   	struct dp_event event_list[DP_EVENT_Q_MAX];
>   	spinlock_t event_lock;
>   
> @@ -230,6 +231,31 @@ void dp_display_signal_audio_complete(struct msm_dp *dp_display)
>   	complete_all(&dp->audio_comp);
>   }
>   
> +static int hpd_event_thread(void *data);
> +
> +static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
> +{
> +	init_waitqueue_head(&dp_priv->event_q);
> +	spin_lock_init(&dp_priv->event_lock);

This can go to dp_probe()

> +
> +	dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
> +
> +	if (IS_ERR(dp_priv->ev_tsk))
> +		DRM_ERROR("failed to create DP event thread\n");
> +}
> +
> +static void dp_hpd_event_stop(struct dp_display_private *dp_priv)
> +{
> +	if (IS_ERR(dp_priv->ev_tsk))
> +		return;
> +
> +	kthread_stop(dp_priv->ev_tsk);
> +
> +	/* reset event q to empty */
> +	dp_priv->event_gndx = 0;
> +	dp_priv->event_pndx = 0;
> +}
> +
>   static int dp_display_bind(struct device *dev, struct device *master,
>   			   void *data)
>   {
> @@ -269,6 +295,7 @@ static int dp_display_bind(struct device *dev, struct device *master,
>   	if (rc)
>   		DRM_ERROR("Audio registration Dp failed\n");
>   
> +	dp_hpd_event_setup(dp); /* start event thread */
>   end:
>   	return rc;
>   }
> @@ -280,6 +307,8 @@ static void dp_display_unbind(struct device *dev, struct device *master,
>   	struct drm_device *drm = dev_get_drvdata(master);
>   	struct msm_drm_private *priv = drm->dev_private;
>   
> +	disable_irq(dp->irq);
> +	dp_hpd_event_stop(dp); /* stop event thread */
>   	dp_power_client_deinit(dp->power);
>   	dp_aux_unregister(dp->aux);
>   	priv->dp[dp->id] = NULL;
> @@ -1054,7 +1083,7 @@ static int hpd_event_thread(void *data)
>   
>   	dp_priv = (struct dp_display_private *)data;
>   
> -	while (1) {
> +	while (!kthread_should_stop()) {
>   		if (timeout_mode) {
>   			wait_event_timeout(dp_priv->event_q,
>   				(dp_priv->event_pndx == dp_priv->event_gndx),
> @@ -1132,13 +1161,6 @@ static int hpd_event_thread(void *data)
>   	return 0;
>   }
>   
> -static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
> -{
> -	init_waitqueue_head(&dp_priv->event_q);
> -	spin_lock_init(&dp_priv->event_lock);
> -
> -	kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
> -}
>   
>   static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
>   {
> @@ -1441,8 +1463,6 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>   
>   	dp = container_of(dp_display, struct dp_display_private, dp_display);
>   
> -	dp_hpd_event_setup(dp);
> -
>   	dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>   }
>   


-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dp: stop event kernel thread when DP unbind

[Kuogee Hsieh]

On 4/13/2022 4:19 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-04-13 14:04:25)
>> Current DP driver implementation, event thread is kept running
>> after DP display is unbind. This patch fix this problem by disabling
>> DP irq and stop event thread to exit gracefully at dp_display_unbind().
>>
>> Changes in v2:
>> -- start event thread at dp_display_bind()
>>
>> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 40 +++++++++++++++++++++++++++----------
>>   1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 01453db..943e4f1 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -113,6 +113,7 @@ struct dp_display_private {
>>          u32 hpd_state;
>>          u32 event_pndx;
>>          u32 event_gndx;
>> +       struct task_struct *ev_tsk;
>>          struct dp_event event_list[DP_EVENT_Q_MAX];
>>          spinlock_t event_lock;
>>
>> @@ -230,6 +231,31 @@ void dp_display_signal_audio_complete(struct msm_dp *dp_display)
>>          complete_all(&dp->audio_comp);
>>   }
>>
>> +static int hpd_event_thread(void *data);
> Is there a reason why this is needed vs. defining the kthread start
> function after hpd_event_thread()?
too many code need to be relocated.
>
>> +
>> +static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
> Maybe dp_hpd_event_thread_start()?
>
>> +{
>> +       init_waitqueue_head(&dp_priv->event_q);
>> +       spin_lock_init(&dp_priv->event_lock);
>> +
>> +       dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
>> +
>> +       if (IS_ERR(dp_priv->ev_tsk))
>> +               DRM_ERROR("failed to create DP event thread\n");
> Can we return an error from this function?
>
>> +}
>> +
>> +static void dp_hpd_event_stop(struct dp_display_private *dp_priv)
> Maybe dp_hpd_event_thread_stop()?
>
>> +{
>> +       if (IS_ERR(dp_priv->ev_tsk))
>> +               return;
> If we handled the error then this check becomes impossible.
>
>> +
>> +       kthread_stop(dp_priv->ev_tsk);
>> +
>> +       /* reset event q to empty */
>> +       dp_priv->event_gndx = 0;
>> +       dp_priv->event_pndx = 0;
>> +}
>> +
>>   static int dp_display_bind(struct device *dev, struct device *master,
>>                             void *data)
>>   {
>> @@ -269,6 +295,7 @@ static int dp_display_bind(struct device *dev, struct device *master,
>>          if (rc)
>>                  DRM_ERROR("Audio registration Dp failed\n");
>>
>> +       dp_hpd_event_setup(dp); /* start event thread */
> The comment is useless, please remove.
>
>>   end:
>>          return rc;
>>   }
>> @@ -280,6 +307,8 @@ static void dp_display_unbind(struct device *dev, struct device *master,
>>          struct drm_device *drm = dev_get_drvdata(master);
>>          struct msm_drm_private *priv = drm->dev_private;
>>
>> +       disable_irq(dp->irq);
> Is the disable_irq() necessary? It would be nicer to silence the
> hardware and remove the disable_irq() so that we can reason about the
> code assuming the irq is always enabled after it is requested.
>
>> +       dp_hpd_event_stop(dp); /* stop event thread */
>>          dp_power_client_deinit(dp->power);
>>          dp_aux_unregister(dp->aux);
>>          priv->dp[dp->id] = NULL;