All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
@ 2022-05-12 19:43 ` Kuogee Hsieh
  0 siblings, 0 replies; 6+ messages in thread
From: Kuogee Hsieh @ 2022-05-12 19:43 UTC (permalink / raw)
  To: robdclark, sean, swboyd, dianders, 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

dp_catalog_ctrl_reset() will software reset DP controller. But it will
not reset programmable registers to default value. DP driver still have
to clear mask bits to interrupt status registers to disable interrupts
after software reset of controller.

At current implementation, dp_ctrl_reset_irq_ctrl() will software reset dp
controller but did not call dp_catalog_ctrl_enable_irq(false) to clear hpd
related interrupt mask bits to disable hpd related interrupts due to it
mistakenly think hpd related interrupt mask bits will be cleared by software
reset of dp controller automatically. This mistake may cause system to crash
during suspending procedure due to unexpected irq fired and trigger event
thread to access dp controller registers with controller clocks are disabled.

This patch fixes system crash during suspending problem by removing "enable"
flag condition checking at dp_ctrl_reset_irq_ctrl() so that hpd related
interrupt mask bits are cleared to prevent unexpected from happening.
In addition, this patch also add suspended flag to prevent new events be
added into event Q to wake up event thread after system suspended.

Changes in v2:
-- add more details commit text

Changes in v3:
-- add synchrons_irq()
-- add atomic_t suspended

Changes in v4:
-- correct Fixes's commit ID
-- remove synchrons_irq()

Changes in v5:
-- revise commit text

Changes in v6:
-- add event_lock to protect "suspended"

Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt received")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c    |  9 +++++++--
 drivers/gpu/drm/msm/dp/dp_display.c | 25 ++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index af7a80c..f3e333e 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
 
 	dp_catalog_ctrl_reset(ctrl->catalog);
 
-	if (enable)
-		dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
+	/*
+	 * all dp controller programmable registers will not
+	 * be reset to default value after DP_SW_RESET
+	 * therefore interrupt mask bits have to be updated
+	 * to enable/disable interrupts
+	 */
+	dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
 }
 
 void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index c388323..ab691aa 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -98,6 +98,8 @@ struct dp_display_private {
 	struct dp_ctrl    *ctrl;
 	struct dp_debug   *debug;
 
+	bool suspended;
+
 	struct dp_usbpd_cb usbpd_cb;
 	struct dp_display_mode dp_mode;
 	struct msm_dp dp_display;
@@ -187,6 +189,11 @@ static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
 	int pndx;
 
 	spin_lock_irqsave(&dp_priv->event_lock, flag);
+	if (dp_priv->suspended) {
+		spin_unlock_irqrestore(&dp_priv->event_lock, flag);
+		return -ENOENT;
+	}
+
 	pndx = dp_priv->event_pndx + 1;
 	pndx %= DP_EVENT_Q_MAX;
 	if (pndx == dp_priv->event_gndx) {
@@ -454,7 +461,6 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
 		dp->dp_display.connector_type, dp->core_initialized,
 		dp->phy_initialized);
 
-	dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
 	dp_aux_deinit(dp->aux);
 	dp_power_deinit(dp->power);
 	dp->core_initialized = false;
@@ -1112,7 +1118,12 @@ static int hpd_event_thread(void *data)
 			wait_event_interruptible(dp_priv->event_q,
 				(dp_priv->event_pndx != dp_priv->event_gndx));
 		}
+
 		spin_lock_irqsave(&dp_priv->event_lock, flag);
+		if (dp_priv->suspended) {
+			spin_unlock_irqrestore(&dp_priv->event_lock, flag);
+			continue;
+		}
 		todo = &dp_priv->event_list[dp_priv->event_gndx];
 		if (todo->delay) {
 			struct dp_event *todo_next;
@@ -1351,6 +1362,7 @@ static int dp_pm_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct msm_dp *dp_display = platform_get_drvdata(pdev);
 	struct dp_display_private *dp;
+	unsigned long flag;
 	int sink_count = 0;
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
@@ -1362,6 +1374,10 @@ static int dp_pm_resume(struct device *dev)
 		dp->dp_display.connector_type, dp->core_initialized,
 		dp->phy_initialized, dp_display->power_on);
 
+	spin_lock_irqsave(&dp->event_lock, flag);
+	dp->suspended = false;
+	spin_unlock_irqrestore(&dp->event_lock, flag);
+
 	/* start from disconnected state */
 	dp->hpd_state = ST_DISCONNECTED;
 
@@ -1421,6 +1437,7 @@ static int dp_pm_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct msm_dp *dp_display = platform_get_drvdata(pdev);
 	struct dp_display_private *dp;
+	unsigned long flag;
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
@@ -1431,6 +1448,12 @@ static int dp_pm_suspend(struct device *dev)
 		dp->dp_display.connector_type, dp->core_initialized,
 		dp->phy_initialized, dp_display->power_on);
 
+	dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+
+	spin_lock_irqsave(&dp->event_lock, flag);
+	dp->suspended = true;
+	spin_unlock_irqrestore(&dp->event_lock, flag);
+
 	/* mainlink enabled */
 	if (dp_power_clk_status(dp->power, DP_CTRL_PM))
 		dp_ctrl_off_link_stream(dp->ctrl);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v6] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
@ 2022-05-12 19:43 ` Kuogee Hsieh
  0 siblings, 0 replies; 6+ messages in thread
From: Kuogee Hsieh @ 2022-05-12 19:43 UTC (permalink / raw)
  To: robdclark, sean, swboyd, dianders, vkoul, daniel, airlied,
	agross, dmitry.baryshkov, bjorn.andersson
  Cc: quic_sbillaka, linux-arm-msm, quic_abhinavk, dri-devel,
	quic_khsieh, quic_aravindh, freedreno, linux-kernel

dp_catalog_ctrl_reset() will software reset DP controller. But it will
not reset programmable registers to default value. DP driver still have
to clear mask bits to interrupt status registers to disable interrupts
after software reset of controller.

At current implementation, dp_ctrl_reset_irq_ctrl() will software reset dp
controller but did not call dp_catalog_ctrl_enable_irq(false) to clear hpd
related interrupt mask bits to disable hpd related interrupts due to it
mistakenly think hpd related interrupt mask bits will be cleared by software
reset of dp controller automatically. This mistake may cause system to crash
during suspending procedure due to unexpected irq fired and trigger event
thread to access dp controller registers with controller clocks are disabled.

This patch fixes system crash during suspending problem by removing "enable"
flag condition checking at dp_ctrl_reset_irq_ctrl() so that hpd related
interrupt mask bits are cleared to prevent unexpected from happening.
In addition, this patch also add suspended flag to prevent new events be
added into event Q to wake up event thread after system suspended.

Changes in v2:
-- add more details commit text

Changes in v3:
-- add synchrons_irq()
-- add atomic_t suspended

Changes in v4:
-- correct Fixes's commit ID
-- remove synchrons_irq()

Changes in v5:
-- revise commit text

Changes in v6:
-- add event_lock to protect "suspended"

Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt received")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c    |  9 +++++++--
 drivers/gpu/drm/msm/dp/dp_display.c | 25 ++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index af7a80c..f3e333e 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
 
 	dp_catalog_ctrl_reset(ctrl->catalog);
 
-	if (enable)
-		dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
+	/*
+	 * all dp controller programmable registers will not
+	 * be reset to default value after DP_SW_RESET
+	 * therefore interrupt mask bits have to be updated
+	 * to enable/disable interrupts
+	 */
+	dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
 }
 
 void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index c388323..ab691aa 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -98,6 +98,8 @@ struct dp_display_private {
 	struct dp_ctrl    *ctrl;
 	struct dp_debug   *debug;
 
+	bool suspended;
+
 	struct dp_usbpd_cb usbpd_cb;
 	struct dp_display_mode dp_mode;
 	struct msm_dp dp_display;
@@ -187,6 +189,11 @@ static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
 	int pndx;
 
 	spin_lock_irqsave(&dp_priv->event_lock, flag);
+	if (dp_priv->suspended) {
+		spin_unlock_irqrestore(&dp_priv->event_lock, flag);
+		return -ENOENT;
+	}
+
 	pndx = dp_priv->event_pndx + 1;
 	pndx %= DP_EVENT_Q_MAX;
 	if (pndx == dp_priv->event_gndx) {
@@ -454,7 +461,6 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
 		dp->dp_display.connector_type, dp->core_initialized,
 		dp->phy_initialized);
 
-	dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
 	dp_aux_deinit(dp->aux);
 	dp_power_deinit(dp->power);
 	dp->core_initialized = false;
@@ -1112,7 +1118,12 @@ static int hpd_event_thread(void *data)
 			wait_event_interruptible(dp_priv->event_q,
 				(dp_priv->event_pndx != dp_priv->event_gndx));
 		}
+
 		spin_lock_irqsave(&dp_priv->event_lock, flag);
+		if (dp_priv->suspended) {
+			spin_unlock_irqrestore(&dp_priv->event_lock, flag);
+			continue;
+		}
 		todo = &dp_priv->event_list[dp_priv->event_gndx];
 		if (todo->delay) {
 			struct dp_event *todo_next;
@@ -1351,6 +1362,7 @@ static int dp_pm_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct msm_dp *dp_display = platform_get_drvdata(pdev);
 	struct dp_display_private *dp;
+	unsigned long flag;
 	int sink_count = 0;
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
@@ -1362,6 +1374,10 @@ static int dp_pm_resume(struct device *dev)
 		dp->dp_display.connector_type, dp->core_initialized,
 		dp->phy_initialized, dp_display->power_on);
 
+	spin_lock_irqsave(&dp->event_lock, flag);
+	dp->suspended = false;
+	spin_unlock_irqrestore(&dp->event_lock, flag);
+
 	/* start from disconnected state */
 	dp->hpd_state = ST_DISCONNECTED;
 
@@ -1421,6 +1437,7 @@ static int dp_pm_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct msm_dp *dp_display = platform_get_drvdata(pdev);
 	struct dp_display_private *dp;
+	unsigned long flag;
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
@@ -1431,6 +1448,12 @@ static int dp_pm_suspend(struct device *dev)
 		dp->dp_display.connector_type, dp->core_initialized,
 		dp->phy_initialized, dp_display->power_on);
 
+	dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+
+	spin_lock_irqsave(&dp->event_lock, flag);
+	dp->suspended = true;
+	spin_unlock_irqrestore(&dp->event_lock, flag);
+
 	/* mainlink enabled */
 	if (dp_power_clk_status(dp->power, DP_CTRL_PM))
 		dp_ctrl_off_link_stream(dp->ctrl);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v6] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
  2022-05-12 19:43 ` Kuogee Hsieh
@ 2022-05-17  8:25   ` Stephen Boyd
  -1 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2022-05-17  8:25 UTC (permalink / raw)
  To: Kuogee Hsieh, agross, airlied, bjorn.andersson, daniel, dianders,
	dmitry.baryshkov, robdclark, sean, vkoul
  Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
	dri-devel, linux-arm-msm, linux-kernel

Quoting Kuogee Hsieh (2022-05-12 12:43:18)
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index af7a80c..f3e333e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
>
>         dp_catalog_ctrl_reset(ctrl->catalog);
>
> -       if (enable)
> -               dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
> +       /*
> +        * all dp controller programmable registers will not
> +        * be reset to default value after DP_SW_RESET
> +        * therefore interrupt mask bits have to be updated
> +        * to enable/disable interrupts
> +        */
> +       dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);

I think Dmitry requested that this part be split off to a different
patch. It's fixing the call to dp_ctrl_reset_irq_ctrl() for the disable
case.

>  }
>
>  void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index c388323..ab691aa 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -98,6 +98,8 @@ struct dp_display_private {
>         struct dp_ctrl    *ctrl;
>         struct dp_debug   *debug;
>
> +       bool suspended;
> +

Can we not have this flag? I also don't understand how this patch waits
for the event queue to drain. There are now multiple places we check to
see if we've suspended, but is it even possible to be in those places
during suspend? What is wrong with moving to an irq thread? Is it
because we want to wait for a modeset (EV_USER_NOTIFICATION)?

>         struct dp_usbpd_cb usbpd_cb;
>         struct dp_display_mode dp_mode;
>         struct msm_dp dp_display;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
@ 2022-05-17  8:25   ` Stephen Boyd
  0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2022-05-17  8:25 UTC (permalink / raw)
  To: Kuogee Hsieh, agross, airlied, bjorn.andersson, daniel, dianders,
	dmitry.baryshkov, robdclark, sean, vkoul
  Cc: quic_sbillaka, linux-arm-msm, quic_abhinavk, dri-devel,
	linux-kernel, quic_aravindh, freedreno

Quoting Kuogee Hsieh (2022-05-12 12:43:18)
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index af7a80c..f3e333e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
>
>         dp_catalog_ctrl_reset(ctrl->catalog);
>
> -       if (enable)
> -               dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
> +       /*
> +        * all dp controller programmable registers will not
> +        * be reset to default value after DP_SW_RESET
> +        * therefore interrupt mask bits have to be updated
> +        * to enable/disable interrupts
> +        */
> +       dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);

I think Dmitry requested that this part be split off to a different
patch. It's fixing the call to dp_ctrl_reset_irq_ctrl() for the disable
case.

>  }
>
>  void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index c388323..ab691aa 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -98,6 +98,8 @@ struct dp_display_private {
>         struct dp_ctrl    *ctrl;
>         struct dp_debug   *debug;
>
> +       bool suspended;
> +

Can we not have this flag? I also don't understand how this patch waits
for the event queue to drain. There are now multiple places we check to
see if we've suspended, but is it even possible to be in those places
during suspend? What is wrong with moving to an irq thread? Is it
because we want to wait for a modeset (EV_USER_NOTIFICATION)?

>         struct dp_usbpd_cb usbpd_cb;
>         struct dp_display_mode dp_mode;
>         struct msm_dp dp_display;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
  2022-05-17  8:25   ` Stephen Boyd
@ 2022-05-17 15:55     ` Kuogee Hsieh
  -1 siblings, 0 replies; 6+ messages in thread
From: Kuogee Hsieh @ 2022-05-17 15:55 UTC (permalink / raw)
  To: Stephen Boyd, agross, airlied, bjorn.andersson, daniel, dianders,
	dmitry.baryshkov, robdclark, sean, vkoul
  Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
	dri-devel, linux-arm-msm, linux-kernel


On 5/17/2022 1:25 AM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-05-12 12:43:18)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index af7a80c..f3e333e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
>>
>>          dp_catalog_ctrl_reset(ctrl->catalog);
>>
>> -       if (enable)
>> -               dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
>> +       /*
>> +        * all dp controller programmable registers will not
>> +        * be reset to default value after DP_SW_RESET
>> +        * therefore interrupt mask bits have to be updated
>> +        * to enable/disable interrupts
>> +        */
>> +       dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
> I think Dmitry requested that this part be split off to a different
> patch. It's fixing the call to dp_ctrl_reset_irq_ctrl() for the disable
> case.
ok, will do that
>>   }
>>
>>   void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index c388323..ab691aa 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -98,6 +98,8 @@ struct dp_display_private {
>>          struct dp_ctrl    *ctrl;
>>          struct dp_debug   *debug;
>>
>> +       bool suspended;
>> +
> Can we not have this flag? I also don't understand how this patch waits
> for the event queue to drain. There are now multiple places we check to
> see if we've suspended, but is it even possible to be in those places
> during suspend? What is wrong with moving to an irq thread? Is it
> because we want to wait for a modeset (EV_USER_NOTIFICATION)?

dp_pm_suspend happen after display off. therefore event q should be 
emptied when dp_pm_suspend called.

since dp_pm_suspend and event_thread share an mutex, if suspended flag 
set at dp_pm_suspend which guarantee

event thread will not be wakeed up to process since event q is empty.

We used event q is to guarantee all events (irqs) are kept in event q 
with timing order (no any events missed) and executed later in timing 
order too to work the scenario likes dongle plug/unplugged several times 
consecutively.

We used to workqueue but cause many synchronization issues.

I will try thread_irq to see it will yield any advantage over kernel thread.

>>          struct dp_usbpd_cb usbpd_cb;
>>          struct dp_display_mode dp_mode;
>>          struct msm_dp dp_display;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
@ 2022-05-17 15:55     ` Kuogee Hsieh
  0 siblings, 0 replies; 6+ messages in thread
From: Kuogee Hsieh @ 2022-05-17 15:55 UTC (permalink / raw)
  To: Stephen Boyd, agross, airlied, bjorn.andersson, daniel, dianders,
	dmitry.baryshkov, robdclark, sean, vkoul
  Cc: quic_sbillaka, linux-arm-msm, quic_abhinavk, dri-devel,
	linux-kernel, quic_aravindh, freedreno


On 5/17/2022 1:25 AM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-05-12 12:43:18)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index af7a80c..f3e333e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
>>
>>          dp_catalog_ctrl_reset(ctrl->catalog);
>>
>> -       if (enable)
>> -               dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
>> +       /*
>> +        * all dp controller programmable registers will not
>> +        * be reset to default value after DP_SW_RESET
>> +        * therefore interrupt mask bits have to be updated
>> +        * to enable/disable interrupts
>> +        */
>> +       dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
> I think Dmitry requested that this part be split off to a different
> patch. It's fixing the call to dp_ctrl_reset_irq_ctrl() for the disable
> case.
ok, will do that
>>   }
>>
>>   void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index c388323..ab691aa 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -98,6 +98,8 @@ struct dp_display_private {
>>          struct dp_ctrl    *ctrl;
>>          struct dp_debug   *debug;
>>
>> +       bool suspended;
>> +
> Can we not have this flag? I also don't understand how this patch waits
> for the event queue to drain. There are now multiple places we check to
> see if we've suspended, but is it even possible to be in those places
> during suspend? What is wrong with moving to an irq thread? Is it
> because we want to wait for a modeset (EV_USER_NOTIFICATION)?

dp_pm_suspend happen after display off. therefore event q should be 
emptied when dp_pm_suspend called.

since dp_pm_suspend and event_thread share an mutex, if suspended flag 
set at dp_pm_suspend which guarantee

event thread will not be wakeed up to process since event q is empty.

We used event q is to guarantee all events (irqs) are kept in event q 
with timing order (no any events missed) and executed later in timing 
order too to work the scenario likes dongle plug/unplugged several times 
consecutively.

We used to workqueue but cause many synchronization issues.

I will try thread_irq to see it will yield any advantage over kernel thread.

>>          struct dp_usbpd_cb usbpd_cb;
>>          struct dp_display_mode dp_mode;
>>          struct msm_dp dp_display;

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-05-17 15:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 19:43 [PATCH v6] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl() Kuogee Hsieh
2022-05-12 19:43 ` Kuogee Hsieh
2022-05-17  8:25 ` Stephen Boyd
2022-05-17  8:25   ` Stephen Boyd
2022-05-17 15:55   ` Kuogee Hsieh
2022-05-17 15:55     ` Kuogee Hsieh

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.