* [PATCH] drm/msm/dp: stop event kernel thread when DP unbind
@ 2022-04-12 0:08 Kuogee Hsieh
2022-04-12 0:21 ` Stephen Boyd
0 siblings, 1 reply; 5+ messages in thread
From: Kuogee Hsieh @ 2022-04-12 0:08 UTC (permalink / raw)
To: robdclark, sean, swboyd, vkoul, daniel, airlied, agross,
dmitry.baryshkov, bjorn.andersson
Cc: quic_abhinavk, quic_aravindh, quic_khsieh, quic_sbillaka,
freedreno, dri-devel, linux-arm-msm, linux-kernel
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().
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
drivers/gpu/drm/msm/dp/dp_display.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..fa1ef8e 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;
@@ -273,6 +274,8 @@ static int dp_display_bind(struct device *dev, struct device *master,
return rc;
}
+static void dp_hpd_event_stop(struct dp_display_private *dp_priv);
+
static void dp_display_unbind(struct device *dev, struct device *master,
void *data)
{
@@ -280,6 +283,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);
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp[dp->id] = NULL;
@@ -1054,7 +1059,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),
@@ -1137,7 +1142,22 @@ 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");
+ 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 irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/msm/dp: stop event kernel thread when DP unbind
2022-04-12 0:08 [PATCH] drm/msm/dp: stop event kernel thread when DP unbind Kuogee Hsieh
@ 2022-04-12 0:21 ` Stephen Boyd
2022-04-12 0:22 ` Dmitry Baryshkov
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2022-04-12 0:21 UTC (permalink / raw)
To: Kuogee Hsieh, agross, airlied, bjorn.andersson, daniel,
dmitry.baryshkov, robdclark, sean, vkoul
Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
dri-devel, linux-arm-msm, linux-kernel
Quoting Kuogee Hsieh (2022-04-11 17:08:49)
> 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().
>
> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Should add a Reported-by tag from Dmitry here.
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 01453db..fa1ef8e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -273,6 +274,8 @@ static int dp_display_bind(struct device *dev, struct device *master,
> return rc;
> }
>
> +static void dp_hpd_event_stop(struct dp_display_private *dp_priv);
Why can't the function be defined here?
> +
> static void dp_display_unbind(struct device *dev, struct device *master,
> void *data)
> {
> @@ -280,6 +283,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);
> dp_power_client_deinit(dp->power);
> dp_aux_unregister(dp->aux);
> priv->dp[dp->id] = NULL;
> @@ -1054,7 +1059,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),
> @@ -1137,7 +1142,22 @@ 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");
> + 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");
Why can't we error out? Why can't this kthread be started in probe?
> +}
> +
> +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;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/msm/dp: stop event kernel thread when DP unbind
2022-04-12 0:21 ` Stephen Boyd
@ 2022-04-12 0:22 ` Dmitry Baryshkov
2022-04-12 0:29 ` Abhinav Kumar
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Baryshkov @ 2022-04-12 0:22 UTC (permalink / raw)
To: Stephen Boyd, Kuogee Hsieh, agross, airlied, bjorn.andersson,
daniel, robdclark, sean, vkoul
Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
dri-devel, linux-arm-msm, linux-kernel
On 12/04/2022 03:21, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-04-11 17:08:49)
>> 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().
>>
>> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>
> Should add a Reported-by tag from Dmitry here.
>
>> ---
>> drivers/gpu/drm/msm/dp/dp_display.c | 24 ++++++++++++++++++++++--
>> 1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 01453db..fa1ef8e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -273,6 +274,8 @@ static int dp_display_bind(struct device *dev, struct device *master,
>> return rc;
>> }
>>
>> +static void dp_hpd_event_stop(struct dp_display_private *dp_priv);
>
> Why can't the function be defined here?
>
>> +
>> static void dp_display_unbind(struct device *dev, struct device *master,
>> void *data)
>> {
>> @@ -280,6 +283,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);
>> dp_power_client_deinit(dp->power);
>> dp_aux_unregister(dp->aux);
>> priv->dp[dp->id] = NULL;
>> @@ -1054,7 +1059,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),
>> @@ -1137,7 +1142,22 @@ 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");
>> + 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");
>
> Why can't we error out? Why can't this kthread be started in probe?
Just my 2c. I don't think starting it in probe is a good idea. The
driver uses components, so, in my opinion, the thread should be started
from bind and stopped in unbind.
>
>> +}
>> +
>> +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;
>> }
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/msm/dp: stop event kernel thread when DP unbind
2022-04-12 0:22 ` Dmitry Baryshkov
@ 2022-04-12 0:29 ` Abhinav Kumar
2022-04-12 4:22 ` Stephen Boyd
0 siblings, 1 reply; 5+ messages in thread
From: Abhinav Kumar @ 2022-04-12 0:29 UTC (permalink / raw)
To: Dmitry Baryshkov, Stephen Boyd, Kuogee Hsieh, agross, airlied,
bjorn.andersson, daniel, robdclark, sean, vkoul
Cc: quic_aravindh, quic_sbillaka, freedreno, dri-devel,
linux-arm-msm, linux-kernel
On 4/11/2022 5:22 PM, Dmitry Baryshkov wrote:
> On 12/04/2022 03:21, Stephen Boyd wrote:
>> Quoting Kuogee Hsieh (2022-04-11 17:08:49)
>>> 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().
>>>
>>> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on
>>> Snapdragon Chipsets")
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>
>> Should add a Reported-by tag from Dmitry here.
>>
>>> ---
>>> drivers/gpu/drm/msm/dp/dp_display.c | 24 ++++++++++++++++++++++--
>>> 1 file changed, 22 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index 01453db..fa1ef8e 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -273,6 +274,8 @@ static int dp_display_bind(struct device *dev,
>>> struct device *master,
>>> return rc;
>>> }
>>>
>>> +static void dp_hpd_event_stop(struct dp_display_private *dp_priv);
>>
>> Why can't the function be defined here?
>>
>>> +
>>> static void dp_display_unbind(struct device *dev, struct device
>>> *master,
>>> void *data)
>>> {
>>> @@ -280,6 +283,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);
>>> dp_power_client_deinit(dp->power);
>>> dp_aux_unregister(dp->aux);
>>> priv->dp[dp->id] = NULL;
>>> @@ -1054,7 +1059,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),
>>> @@ -1137,7 +1142,22 @@ 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");
>>> + 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");
>>
>> Why can't we error out? Why can't this kthread be started in probe?
>
> Just my 2c. I don't think starting it in probe is a good idea. The
> driver uses components, so, in my opinion, the thread should be started
> from bind and stopped in unbind.
Yes, I also agree it should be started in bind and stopped in unbind.
>
>>
>>> +}
>>> +
>>> +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;
>>> }
>>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/msm/dp: stop event kernel thread when DP unbind
2022-04-12 0:29 ` Abhinav Kumar
@ 2022-04-12 4:22 ` Stephen Boyd
0 siblings, 0 replies; 5+ messages in thread
From: Stephen Boyd @ 2022-04-12 4:22 UTC (permalink / raw)
To: Abhinav Kumar, Dmitry Baryshkov, Kuogee Hsieh, agross, airlied,
bjorn.andersson, daniel, robdclark, sean, vkoul
Cc: quic_aravindh, quic_sbillaka, freedreno, dri-devel,
linux-arm-msm, linux-kernel
Quoting Abhinav Kumar (2022-04-11 17:29:17)
>
>
> On 4/11/2022 5:22 PM, Dmitry Baryshkov wrote:
> > On 12/04/2022 03:21, Stephen Boyd wrote:
> >> Quoting Kuogee Hsieh (2022-04-11 17:08:49)
> >>> - kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
> >>> + 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");
> >>
> >> Why can't we error out? Why can't this kthread be started in probe?
> >
> > Just my 2c. I don't think starting it in probe is a good idea. The
> > driver uses components, so, in my opinion, the thread should be started
> > from bind and stopped in unbind.
>
> Yes, I also agree it should be started in bind and stopped in unbind.
>
Sounds good to me! I forgot that this is a component.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-12 4:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-12 0:08 [PATCH] drm/msm/dp: stop event kernel thread when DP unbind Kuogee Hsieh
2022-04-12 0:21 ` Stephen Boyd
2022-04-12 0:22 ` Dmitry Baryshkov
2022-04-12 0:29 ` Abhinav Kumar
2022-04-12 4:22 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).