| Message ID | 1695848028-18023-8-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | incorporate pm runtime framework and eDP clean up | expand |
On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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@quicinc.com> > --- > 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? > - > - /* 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? > - > 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. > > pm_runtime_mark_last_busy(&dp->pdev->dev); > pm_runtime_put_autosuspend(&dp->pdev->dev); > -- > 2.7.4 > -- With best wishes Dmitry
On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote: > On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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@quicinc.com> >> --- >> 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. 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() 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. >> - >> 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. 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
On Tue, 3 Oct 2023 at 19:44, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote: > > On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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@quicinc.com> > >> --- > >> 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? > 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() Step 1 should be optional. DP should be functional even if the .hpd_enable was not called. Have you tested this usecase? > > 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? > > > > >> - > >> 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
On 10/3/2023 10:53 AM, Dmitry Baryshkov wrote: > On Tue, 3 Oct 2023 at 19:44, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >> >> On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote: >>> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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@quicinc.com> >>>> --- >>>> 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(). 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. > >> 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. 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. >> 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). 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. >> >> >>>> - >>>> 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 > >
On Wed, 4 Oct 2023 at 01:12, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > On 10/3/2023 10:53 AM, Dmitry Baryshkov wrote: > > On Tue, 3 Oct 2023 at 19:44, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > >> > >> On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote: > >>> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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@quicinc.com> > >>>> --- > >>>> 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 > > 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? > > 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. > > > >> > >> > >>>> - > >>>> 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 > > > >
On 10/3/2023 3:36 PM, Dmitry Baryshkov wrote: > On Wed, 4 Oct 2023 at 01:12, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >> >> On 10/3/2023 10:53 AM, Dmitry Baryshkov wrote: >>> On Tue, 3 Oct 2023 at 19:44, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >>>> On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote: >>>>> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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@quicinc.com> >>>>>> --- >>>>>> 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 >>> > >
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); - - /* 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); - 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; pm_runtime_mark_last_busy(&dp->pdev->dev); pm_runtime_put_autosuspend(&dp->pdev->dev);
After incorporated pm_runtime framework into eDP/DP driver, the 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@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_display.c | 113 ++---------------------------------- 1 file changed, 5 insertions(+), 108 deletions(-)