* [PATCH v3] drm/msm/dp: power off DP phy at suspend
@ 2021-06-01 23:50 Kuogee Hsieh
2021-06-02 2:00 ` Stephen Boyd
0 siblings, 1 reply; 3+ messages in thread
From: Kuogee Hsieh @ 2021-06-01 23:50 UTC (permalink / raw)
To: robdclark, sean, swboyd, vkoul, agross, bjorn.andersson
Cc: abhinavk, aravindh, khsieh, freedreno, linux-arm-msm, linux-kernel
Normal DP suspend operation contains two steps, display off followed
by dp suspend, to complete system wide suspending cycle if display is
up at that time. In this case, DP phy will be powered off at display
off. However there is an exception case that depending on the timing
of dongle plug in during system wide suspending, sometimes display off
procedure may be skipped and dp suspend was called directly. In this
case, dp phy is stay at powered on (phy->power_count = 1) so that at
next resume dp driver crash at main link clock enable due to phy is
not physically powered on. This patch will call dp_ctrl_off_link_stream()
to tear down main link and power off phy at dp_pm_suspend() if main link
had been brought up.
Changes in V2:
-- stashed changes into dp_ctrl.c
-- add is_phy_on to monitor phy state
Changes in V3:
-- delete is_phy_on
-- call dp_ctrl_off_link_stream() from dp_pm_suspend()
Fixes: 0114f31a2903 ("drm/msm/dp: handle irq_hpd with sink_count = 0 correctly)
Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 10 +++++++++-
drivers/gpu/drm/msm/dp/dp_display.c | 4 +++-
drivers/gpu/drm/msm/dp/dp_power.c | 15 +++++++++++++++
3 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index dbd8943..8324a453 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1414,6 +1414,7 @@ void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
phy = dp_io->phy;
dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
+
phy_exit(phy);
DRM_DEBUG_DP("Host deinitialized successfully\n");
@@ -1457,6 +1458,7 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
return ret;
}
phy_power_off(phy);
+
/* hw recommended delay before re-enabling clocks */
msleep(20);
@@ -1488,6 +1490,7 @@ static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)
}
phy_power_off(phy);
+
phy_exit(phy);
return 0;
@@ -1816,12 +1819,16 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
struct phy *phy;
- int ret;
+ int ret = 0;
ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
dp_io = &ctrl->parser->io;
phy = dp_io->phy;
+ /* main link is off */
+ if (!dp_power_clk_status(ctrl->power, DP_CTRL_PM))
+ return ret;
+
/* set dongle to D3 (power off) mode */
dp_link_psm_config(ctrl->link, &ctrl->panel->link_info, true);
@@ -1894,6 +1901,7 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
}
phy_power_off(phy);
+
phy_exit(phy);
DRM_DEBUG_DP("DP off done\n");
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index cdec0a3..5abd769 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1327,8 +1327,10 @@ static int dp_pm_suspend(struct device *dev)
mutex_lock(&dp->event_mutex);
- if (dp->core_initialized == true)
+ if (dp->core_initialized == true) {
+ dp_ctrl_off_link_stream(dp->ctrl);
dp_display_host_deinit(dp);
+ }
dp->hpd_state = ST_SUSPENDED;
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index 9c4ea00..980924a9 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -262,6 +262,21 @@ int dp_power_clk_enable(struct dp_power *dp_power,
}
dp_power->core_clks_on = true;
}
+ } else {
+ if (pm_type == DP_CORE_PM && !dp_power->core_clks_on) {
+ DRM_DEBUG_DP("core clks already disabled\n");
+ return 0;
+ }
+
+ if (pm_type == DP_CTRL_PM && !dp_power->link_clks_on) {
+ DRM_DEBUG_DP("links clks already disabled\n");
+ return 0;
+ }
+
+ if (pm_type == DP_STREAM_PM && !dp_power->stream_clks_on) {
+ DRM_DEBUG_DP("pixel clks already disabled\n");
+ return 0;
+ }
}
rc = dp_power_clk_set_rate(power, pm_type, enable);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] drm/msm/dp: power off DP phy at suspend
2021-06-01 23:50 [PATCH v3] drm/msm/dp: power off DP phy at suspend Kuogee Hsieh
@ 2021-06-02 2:00 ` Stephen Boyd
2021-06-02 15:49 ` khsieh
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Boyd @ 2021-06-02 2:00 UTC (permalink / raw)
To: Kuogee Hsieh, agross, bjorn.andersson, robdclark, sean, vkoul
Cc: abhinavk, aravindh, freedreno, linux-arm-msm, linux-kernel, dri-devel
Please add dri-devel@lists.freedesktop.org next time
Quoting Kuogee Hsieh (2021-06-01 16:50:08)
> Normal DP suspend operation contains two steps, display off followed
> by dp suspend, to complete system wide suspending cycle if display is
> up at that time. In this case, DP phy will be powered off at display
> off. However there is an exception case that depending on the timing
> of dongle plug in during system wide suspending, sometimes display off
> procedure may be skipped and dp suspend was called directly. In this
> case, dp phy is stay at powered on (phy->power_count = 1) so that at
> next resume dp driver crash at main link clock enable due to phy is
> not physically powered on. This patch will call dp_ctrl_off_link_stream()
> to tear down main link and power off phy at dp_pm_suspend() if main link
> had been brought up.
>
> Changes in V2:
> -- stashed changes into dp_ctrl.c
> -- add is_phy_on to monitor phy state
>
> Changes in V3:
> -- delete is_phy_on
> -- call dp_ctrl_off_link_stream() from dp_pm_suspend()
>
> Fixes: 0114f31a2903 ("drm/msm/dp: handle irq_hpd with sink_count = 0 correctly)
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 10 +++++++++-
> drivers/gpu/drm/msm/dp/dp_display.c | 4 +++-
> drivers/gpu/drm/msm/dp/dp_power.c | 15 +++++++++++++++
> 3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index dbd8943..8324a453 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1414,6 +1414,7 @@ void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
> phy = dp_io->phy;
>
> dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
> +
> phy_exit(phy);
>
> DRM_DEBUG_DP("Host deinitialized successfully\n");
> @@ -1457,6 +1458,7 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
> return ret;
> }
> phy_power_off(phy);
> +
> /* hw recommended delay before re-enabling clocks */
> msleep(20);
>
> @@ -1488,6 +1490,7 @@ static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)
> }
>
> phy_power_off(phy);
> +
> phy_exit(phy);
>
> return 0;
None of these hunks are useful. Can we drop them?
> @@ -1816,12 +1819,16 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
> struct dp_ctrl_private *ctrl;
> struct dp_io *dp_io;
> struct phy *phy;
> - int ret;
> + int ret = 0;
Drop this.
>
> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> dp_io = &ctrl->parser->io;
> phy = dp_io->phy;
>
> + /* main link is off */
> + if (!dp_power_clk_status(ctrl->power, DP_CTRL_PM))
> + return ret;
and then return 0?
> +
> /* set dongle to D3 (power off) mode */
> dp_link_psm_config(ctrl->link, &ctrl->panel->link_info, true);
>
> @@ -1894,6 +1901,7 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
> }
>
> phy_power_off(phy);
> +
> phy_exit(phy);
>
> DRM_DEBUG_DP("DP off done\n");
Drop?
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index cdec0a3..5abd769 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1327,8 +1327,10 @@ static int dp_pm_suspend(struct device *dev)
>
> mutex_lock(&dp->event_mutex);
>
> - if (dp->core_initialized == true)
> + if (dp->core_initialized == true) {
> + dp_ctrl_off_link_stream(dp->ctrl);
Why not just check here for dp_power_clk_status()?
> dp_display_host_deinit(dp);
> + }
>
> dp->hpd_state = ST_SUSPENDED;
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index 9c4ea00..980924a9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -262,6 +262,21 @@ int dp_power_clk_enable(struct dp_power *dp_power,
> }
> dp_power->core_clks_on = true;
> }
> + } else {
> + if (pm_type == DP_CORE_PM && !dp_power->core_clks_on) {
> + DRM_DEBUG_DP("core clks already disabled\n");
> + return 0;
> + }
> +
> + if (pm_type == DP_CTRL_PM && !dp_power->link_clks_on) {
> + DRM_DEBUG_DP("links clks already disabled\n");
> + return 0;
> + }
> +
> + if (pm_type == DP_STREAM_PM && !dp_power->stream_clks_on) {
> + DRM_DEBUG_DP("pixel clks already disabled\n");
> + return 0;
> + }
> }
If this happens isn't something wrong? Like we've somehow lost track of
the proper state and no we're trying to disable clks when we don't need
to. And given that clks already manage their own refcount that would be
pretty obvious if it went wrong
>
> rc = dp_power_clk_set_rate(power, pm_type, enable);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] drm/msm/dp: power off DP phy at suspend
2021-06-02 2:00 ` Stephen Boyd
@ 2021-06-02 15:49 ` khsieh
0 siblings, 0 replies; 3+ messages in thread
From: khsieh @ 2021-06-02 15:49 UTC (permalink / raw)
To: Stephen Boyd
Cc: agross, bjorn.andersson, robdclark, sean, vkoul, abhinavk,
aravindh, freedreno, linux-arm-msm, linux-kernel, dri-devel
On 2021-06-01 19:00, Stephen Boyd wrote:
> Please add dri-devel@lists.freedesktop.org next time
>
> Quoting Kuogee Hsieh (2021-06-01 16:50:08)
>> Normal DP suspend operation contains two steps, display off followed
>> by dp suspend, to complete system wide suspending cycle if display is
>> up at that time. In this case, DP phy will be powered off at display
>> off. However there is an exception case that depending on the timing
>> of dongle plug in during system wide suspending, sometimes display off
>> procedure may be skipped and dp suspend was called directly. In this
>> case, dp phy is stay at powered on (phy->power_count = 1) so that at
>> next resume dp driver crash at main link clock enable due to phy is
>> not physically powered on. This patch will call
>> dp_ctrl_off_link_stream()
>> to tear down main link and power off phy at dp_pm_suspend() if main
>> link
>> had been brought up.
>>
>> Changes in V2:
>> -- stashed changes into dp_ctrl.c
>> -- add is_phy_on to monitor phy state
>>
>> Changes in V3:
>> -- delete is_phy_on
>> -- call dp_ctrl_off_link_stream() from dp_pm_suspend()
>>
>> Fixes: 0114f31a2903 ("drm/msm/dp: handle irq_hpd with sink_count = 0
>> correctly)
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 10 +++++++++-
>> drivers/gpu/drm/msm/dp/dp_display.c | 4 +++-
>> drivers/gpu/drm/msm/dp/dp_power.c | 15 +++++++++++++++
>> 3 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index dbd8943..8324a453 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1414,6 +1414,7 @@ void dp_ctrl_host_deinit(struct dp_ctrl
>> *dp_ctrl)
>> phy = dp_io->phy;
>>
>> dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
>> +
>> phy_exit(phy);
>>
>> DRM_DEBUG_DP("Host deinitialized successfully\n");
>> @@ -1457,6 +1458,7 @@ static int dp_ctrl_reinitialize_mainlink(struct
>> dp_ctrl_private *ctrl)
>> return ret;
>> }
>> phy_power_off(phy);
>> +
>> /* hw recommended delay before re-enabling clocks */
>> msleep(20);
>>
>> @@ -1488,6 +1490,7 @@ static int dp_ctrl_deinitialize_mainlink(struct
>> dp_ctrl_private *ctrl)
>> }
>>
>> phy_power_off(phy);
>> +
>> phy_exit(phy);
>>
>> return 0;
>
> None of these hunks are useful. Can we drop them?
>
>> @@ -1816,12 +1819,16 @@ int dp_ctrl_off_link_stream(struct dp_ctrl
>> *dp_ctrl)
>> struct dp_ctrl_private *ctrl;
>> struct dp_io *dp_io;
>> struct phy *phy;
>> - int ret;
>> + int ret = 0;
>
> Drop this.
>
>>
>> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> dp_io = &ctrl->parser->io;
>> phy = dp_io->phy;
>>
>> + /* main link is off */
>> + if (!dp_power_clk_status(ctrl->power, DP_CTRL_PM))
>> + return ret;
>
> and then return 0?
>
>> +
>> /* set dongle to D3 (power off) mode */
>> dp_link_psm_config(ctrl->link, &ctrl->panel->link_info, true);
>>
>> @@ -1894,6 +1901,7 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>> }
>>
>> phy_power_off(phy);
>> +
>> phy_exit(phy);
>>
>> DRM_DEBUG_DP("DP off done\n");
>
> Drop?
>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index cdec0a3..5abd769 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1327,8 +1327,10 @@ static int dp_pm_suspend(struct device *dev)
>>
>> mutex_lock(&dp->event_mutex);
>>
>> - if (dp->core_initialized == true)
>> + if (dp->core_initialized == true) {
>> + dp_ctrl_off_link_stream(dp->ctrl);
>
> Why not just check here for dp_power_clk_status()?
>
>> dp_display_host_deinit(dp);
>> + }
>>
>> dp->hpd_state = ST_SUSPENDED;
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c
>> b/drivers/gpu/drm/msm/dp/dp_power.c
>> index 9c4ea00..980924a9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>> @@ -262,6 +262,21 @@ int dp_power_clk_enable(struct dp_power
>> *dp_power,
>> }
>> dp_power->core_clks_on = true;
>> }
>> + } else {
>> + if (pm_type == DP_CORE_PM && !dp_power->core_clks_on)
>> {
>> + DRM_DEBUG_DP("core clks already disabled\n");
>> + return 0;
>> + }
>> +
>> + if (pm_type == DP_CTRL_PM && !dp_power->link_clks_on)
>> {
>> + DRM_DEBUG_DP("links clks already disabled\n");
>> + return 0;
>> + }
>> +
>> + if (pm_type == DP_STREAM_PM &&
>> !dp_power->stream_clks_on) {
>> + DRM_DEBUG_DP("pixel clks already disabled\n");
>> + return 0;
>> + }
>> }
>
> If this happens isn't something wrong? Like we've somehow lost track of
> the proper state and no we're trying to disable clks when we don't need
> to. And given that clks already manage their own refcount that would be
> pretty obvious if it went wrong
yes,
The problem is at suspend the link training has been done (link clk had
been enabled)
but has no user space frame work response to response display up uevent
so that stream clock is not enabled.
I will drop this but create a dedicated dp_ctrl_off_link() for suspend
purpose.
>
>>
>> rc = dp_power_clk_set_rate(power, pm_type, enable);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-02 15:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-01 23:50 [PATCH v3] drm/msm/dp: power off DP phy at suspend Kuogee Hsieh
2021-06-02 2:00 ` Stephen Boyd
2021-06-02 15:49 ` khsieh
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).