Re: [PATCH v4 7/8] drm/msm/dp: add pm_runtime_force_suspend()/resume()
From: Kuogee Hsieh
Date: Tue Oct 03 2023 - 18:55:08 EST
On 10/3/2023 3:36 PM, Dmitry Baryshkov wrote:
On Wed, 4 Oct 2023 at 01:12, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
On 10/3/2023 10:53 AM, Dmitry Baryshkov wrote:
On Tue, 3 Oct 2023 at 19:44, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote:
On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
After incorporated pm_runtime framework into eDP/DP driver, the
incorporating
original dp_pm_suspend() to handle power off both DP phy and
controller during suspend and dp_pm_resume() to handle power on
both DP phy and controller during resume are not necessary since
those function are replaced by dp_pm_runtime_suspend() and
dp_pm_runtime_resume() through pm runtime framework.
Therefore add pm framework provides functions,
pm_runtime_force_suspend()/resume() to complete incorporating pm
runtime framework into DP driver.
Changes in v4:
-- drop both dp_pm_prepare() and dp_pm_compete() from this change
-- delete ST_SUSPENDED state
-- rewording commit text to add more details regrading the purpose
of this change
Changes in v3:
-- replace dp_pm_suspend() with pm_runtime_force_suspend()
-- replace dp_pm_resume() with pm_runtime_force_resume()
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/dp/dp_display.c | 113 ++----------------------------------
1 file changed, 5 insertions(+), 108 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 9158a2c..711d262 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -49,7 +49,6 @@ enum {
ST_CONNECTED,
ST_DISCONNECT_PENDING,
ST_DISPLAY_OFF,
- ST_SUSPENDED,
};
enum {
@@ -560,7 +559,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
- if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
+ if (state == ST_DISPLAY_OFF) {
mutex_unlock(&dp->event_mutex);
return 0;
}
@@ -674,7 +673,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
- if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
+ if (state == ST_DISPLAY_OFF) {
mutex_unlock(&dp->event_mutex);
return 0;
}
@@ -1321,110 +1320,10 @@ static int dp_pm_runtime_resume(struct device *dev)
return 0;
}
-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;
- int sink_count = 0;
-
- dp = container_of(dp_display, struct dp_display_private, dp_display);
-
- mutex_lock(&dp->event_mutex);
-
- drm_dbg_dp(dp->drm_dev,
- "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
- dp->dp_display.connector_type, dp->core_initialized,
- dp->phy_initialized, dp_display->power_on);
-
- /* start from disconnected state */
- dp->hpd_state = ST_DISCONNECTED;
-
- /* turn on dp ctrl/phy */
- dp_display_host_init(dp);
-
- if (dp_display->is_edp)
- dp_catalog_ctrl_hpd_enable(dp->catalog);
-
- if (dp_catalog_link_is_connected(dp->catalog)) {
- /*
- * set sink to normal operation mode -- D0
- * before dpcd read
- */
- dp_display_host_phy_init(dp);
- dp_link_psm_config(dp->link, &dp->panel->link_info, false);
- sink_count = drm_dp_read_sink_count(dp->aux);
- if (sink_count < 0)
- sink_count = 0;
-
- dp_display_host_phy_exit(dp);
- }
-
- dp->link->sink_count = sink_count;
- /*
- * can not declared display is connected unless
- * HDMI cable is plugged in and sink_count of
- * dongle become 1
- * also only signal audio when disconnected
- */
- if (dp->link->sink_count) {
- dp->dp_display.link_ready = true;
- } else {
- dp->dp_display.link_ready = false;
- dp_display_handle_plugged_change(dp_display, false);
- }
-
- drm_dbg_dp(dp->drm_dev,
- "After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
- dp->dp_display.connector_type, dp->link->sink_count,
- dp->dp_display.link_ready, dp->core_initialized,
- dp->phy_initialized, dp_display->power_on);
-
- mutex_unlock(&dp->event_mutex);
-
- return 0;
-}
-
-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;
-
- dp = container_of(dp_display, struct dp_display_private, dp_display);
-
- mutex_lock(&dp->event_mutex);
-
- drm_dbg_dp(dp->drm_dev,
- "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
- dp->dp_display.connector_type, dp->core_initialized,
- dp->phy_initialized, dp_display->power_on);
-
- /* mainlink enabled */
- if (dp_power_clk_status(dp->power, DP_CTRL_PM))
- dp_ctrl_off_link_stream(dp->ctrl);
-
- dp_display_host_phy_exit(dp);
I was under the impression that dp_pm_runtime_suspend / _resume
functions perform phy init/exit only in eDP cases. Can we really drop
the main suspend/resume functions?
yes on eDP case since it is embedded.
Let me ask the same question in a different way:
dp_pm_suspend() / dp_pm_resume() functions contain several calls to DP
functions. Why can we drop them now? Maybe they had to be dropped in
one of the previous patches, when you have added proper runtime PM
support?
Could you please confirm that after each patch the DP driver is
working, that there are no hidden dependencies between patches?
patch #5 ==> drm/msm/dp: incorporate pm_runtime framework into DP driver
patch #6 ==> drm/msm/dp: delete EV_HPD_INIT_SETUP
patch #7 ==> drm/msm/dp: add pm_runtime_force_suspend()/resume() <==
both dp_pm_suspend() and dp_pm_resume() are dropped here
Patch #5 is this patch and dp_pm_suspend() and dp_pm_resume() still kept.
patch #7 drop both dp_pm_suspend() and dp_pm_resume().
This makes me wonder if squashing #5 and #7 would have resulted in a
cleaner and easier to understand patch. This way we'd be able to
observer code moved from suspend/resume to runtime_suspend /
runtime_resume
ok, i will merge both two patches together
In order to keep every patch work for suspend/resume test, I drop
dp_pm_susend() and dp_pm_resuem() at patch #7.
yes, i confirm each patch DP driver is working.
Ack, thank you.
for external DP case, there are two steps
step 1: enable DP controller's hpd block and start waiting for hpd
interrupts at dp_display_hpd_enable()
The step number I mentioned here is for hpd_internal == true case.
Step 1 should be optional. DP should be functional even if the
.hpd_enable was not called. Have you tested this usecase?
yes, for hpd_internal == false, step #1 is not required.
however I do not have device to test it.
You have. Just add a dp-connector with hpd-gpios to any of your
DP-enabled devices and change the pinctrl for hpd pin to use the gpio
function instead of HPD.
But i think it should work since pm_runtime_resume_and_get() and
pm_runtime_put_sync() to dp_hpd_plug_handle() and dp_hpd_unplug_handle()
respectively.
Ack.
step 2: at plugin interrupts, dp_display_host_phy_init()
step 3: at unplug interrupt: dp_bridge_atomic_post_disable()
dp_display_host_phy_exi()
at runtime, there is loop between step 2 and step 3
step 4: disable DP controller's hpd block
-
- /* host_init will be called at pm_resume */
- dp_display_host_deinit(dp);
-
- dp->hpd_state = ST_SUSPENDED;
-
- drm_dbg_dp(dp->drm_dev,
- "After, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
- dp->dp_display.connector_type, dp->core_initialized,
- dp->phy_initialized, dp_display->power_on);
-
- mutex_unlock(&dp->event_mutex);
-
- return 0;
-}
-
static const struct dev_pm_ops dp_pm_ops = {
SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
- .suspend = dp_pm_suspend,
- .resume = dp_pm_resume,
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
};
static struct platform_driver dp_display_driver = {
@@ -1658,9 +1557,6 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
dp_display = container_of(dp, struct dp_display_private, dp_display);
- if (dp->is_edp)
- dp_hpd_unplug_handle(dp_display, 0);
Why?
dp_hpd_unplug_handle() does not tear down phy.
Therefore eDP does not need to call unplug handle.
I don't fully understand your argument here. Could you please
describe, why this function call was necessary beforehand and what is
being changed now, so that it becomes unnecessary?
dp_hpd_unplug_handle() is not necessary for eDP from very beginning
since dp_bridge_atomic_enable() do it all (tear down link and phy).
Excuse me? Where does dp_bridge_atomic_enable() tear down the link?
my bad, it is dp_bridge_atomic_post_disable()
I think it was added long time ago mistakenly just like to be
compatible with external DP since external DP always have
dp_hpd_unplug_handle() called from irq_handle().
i can restore it back if you insist it.
I insist on having clean documented patches. If function call was not
necessary beforehand, please drop it in the separate patch with proper
description of why and what.
ok, will restore it back.
-
mutex_lock(&dp_display->event_mutex);
state = dp_display->hpd_state;
@@ -1748,6 +1644,7 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
dp_catalog_ctrl_hpd_disable(dp->catalog);
dp_display->internal_hpd = false;
+ dp->hpd_state = ST_DISCONNECTED;
Why? We have only disabled sending of the HPD events. The dongle might
still be connected.
dp_bridge_hpd_disable() disable dp controller hpd block (no more hpd
interrupt will be received).
dp_bridge_hpd_disable() should happen after DP main link had been teared
down already.
No, this assumption is incorrect. hpd_disable can happen at any point
during runtime.
It merely disables HPD interrupt generation, it has nothing to do with
the DP block being enabled or not.
Therefore hpd_state need to be in default state so that next plugin
handle will be start with correct state.
pm_runtime_mark_last_busy(&dp->pdev->dev);
pm_runtime_put_autosuspend(&dp->pdev->dev);
--
2.7.4
--
With best wishes
Dmitry