[v4,7/8] drm/msm/dp: add pm_runtime_force_suspend()/resume()

  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(-)
  

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
>>>
>
>