[v5,5/7] drm/msm/dp: incorporate pm_runtime framework into DP driver

  Currently DP driver is executed independent of PM runtime framework.
This leads msm eDP panel can not being detected by edp_panel driver during
generic_edp_panel_probe() due to AUX DPCD read failed at edp panel driver.
Incorporate PM runtime framework into DP driver so that host controller's
power and clocks are enable/disable through PM runtime mechanism.
Once PM runtime framework is incorporated into DP driver, waking up device
from power up path is not necessary. Hence remove it.

After incorporating pm_runtime framework into eDP/DP driver, 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. Therefore both dp_pm_suspend() and dp_pm_resume() are
dropped and replace with dp_pm_runtime_suspend() and dp_pm_runtime_resume()
respectively.

Changes in v5:
-- remove pm_runtime_put_autosuspend feature, use pm_runtime_put_sync() directly
-- squash add pm_runtime_force_suspend()/resume() patch into this patch

Changes in v4:
-- reworded commit text to explain why pm_framework is required for edp panel
-- reworded commit text to explain autosuspend is choiced
-- delete EV_POWER_PM_GET and PM_EV_POWER_PUT from changes #3
-- delete dp_display_pm_get() and dp_display_pm_Put() from changes #3
-- return value from pm_runtime_resume_and_get() directly
-- check return value of devm_pm_runtime_enable()
-- delete pm_runtime_xxx from dp_display_remove()
-- drop dp_display_host_init() from EV_HPD_INIT_SETUP
-- 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:
-- incorporate removing pm_runtime_xx() from dp_pwer.c to this patch
-- use pm_runtime_resume_and_get() instead of pm_runtime_get()
-- error checking pm_runtime_resume_and_get() return value
-- add EV_POWER_PM_GET and PM_EV_POWER_PUT to handle HPD_GPIO case
-- 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>
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
 drivers/gpu/drm/msm/dp/dp_display.c | 166 ++++++++++++------------------------
 drivers/gpu/drm/msm/dp/dp_power.c   |  16 ----
 drivers/gpu/drm/msm/dp/dp_power.h   |  11 ---
 4 files changed, 59 insertions(+), 139 deletions(-)
  

On 04/10/2023 19:26, Kuogee Hsieh wrote:
> Currently DP driver is executed independent of PM runtime framework.
> This leads msm eDP panel can not being detected by edp_panel driver during
> generic_edp_panel_probe() due to AUX DPCD read failed at edp panel driver.
> Incorporate PM runtime framework into DP driver so that host controller's
> power and clocks are enable/disable through PM runtime mechanism.
> Once PM runtime framework is incorporated into DP driver, waking up device
> from power up path is not necessary. Hence remove it.
> 
> After incorporating pm_runtime framework into eDP/DP driver, 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. Therefore both dp_pm_suspend() and dp_pm_resume() are
> dropped and replace with dp_pm_runtime_suspend() and dp_pm_runtime_resume()
> respectively.
> 
> Changes in v5:
> -- remove pm_runtime_put_autosuspend feature, use pm_runtime_put_sync() directly
> -- squash add pm_runtime_force_suspend()/resume() patch into this patch

Ok, this is much better now. Thank you!

> 
> Changes in v4:
> -- reworded commit text to explain why pm_framework is required for edp panel
> -- reworded commit text to explain autosuspend is choiced
> -- delete EV_POWER_PM_GET and PM_EV_POWER_PUT from changes #3
> -- delete dp_display_pm_get() and dp_display_pm_Put() from changes #3
> -- return value from pm_runtime_resume_and_get() directly
> -- check return value of devm_pm_runtime_enable()
> -- delete pm_runtime_xxx from dp_display_remove()
> -- drop dp_display_host_init() from EV_HPD_INIT_SETUP
> -- 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:
> -- incorporate removing pm_runtime_xx() from dp_pwer.c to this patch
> -- use pm_runtime_resume_and_get() instead of pm_runtime_get()
> -- error checking pm_runtime_resume_and_get() return value
> -- add EV_POWER_PM_GET and PM_EV_POWER_PUT to handle HPD_GPIO case
> -- 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>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
>   drivers/gpu/drm/msm/dp/dp_display.c | 166 ++++++++++++------------------------
>   drivers/gpu/drm/msm/dp/dp_power.c   |  16 ----
>   drivers/gpu/drm/msm/dp/dp_power.h   |  11 ---
>   4 files changed, 59 insertions(+), 139 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 8e3b677..10b6eeb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -291,6 +291,10 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>   		return -EINVAL;
>   	}
>   
> +	ret = pm_runtime_resume_and_get(dp_aux->dev);
> +	if (ret)
> +		return  ret;
> +
>   	mutex_lock(&aux->mutex);
>   	if (!aux->initted) {
>   		ret = -EIO;
> @@ -364,6 +368,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>   
>   exit:
>   	mutex_unlock(&aux->mutex);
> +	pm_runtime_put_sync(dp_aux->dev);
>   
>   	return ret;
>   }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index e4942fc..42efe148b 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 {
> @@ -310,15 +309,10 @@ static void dp_display_unbind(struct device *dev, struct device *master,
>   	struct dp_display_private *dp = dev_get_dp_display_private(dev);
>   	struct msm_drm_private *priv = dev_get_drvdata(master);
>   
> -	/* disable all HPD interrupts */
> -	if (dp->core_initialized)
> -		dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
> -
>   	kthread_stop(dp->ev_tsk);
>   
>   	of_dp_aux_depopulate_bus(dp->aux);
>   
> -	dp_power_client_deinit(dp->power);
>   	dp_unregister_audio_driver(dev, dp->audio);
>   	dp_aux_unregister(dp->aux);
>   	dp->drm_dev = NULL;
> @@ -559,7 +553,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;
>   	}
> @@ -576,6 +570,14 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>   		return 0;
>   	}
>   
> +	if (!dp->dp_display.is_edp) {

Why?

> +		ret = pm_runtime_resume_and_get(&dp->pdev->dev);
> +		if (ret) {
> +			DRM_ERROR("failed to pm_runtime_resume\n");
> +			return ret;
> +		}
> +	}
> +
>   	ret = dp_display_usbpd_configure_cb(&dp->pdev->dev);
>   	if (ret) {	/* link train failed */
>   		dp->hpd_state = ST_DISCONNECTED;
> @@ -658,6 +660,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>   			dp->dp_display.connector_type, state);
>   
>   	/* uevent will complete disconnection part */
> +	if (!dp->dp_display.is_edp)
> +		pm_runtime_put_sync(&dp->pdev->dev);
>   	mutex_unlock(&dp->event_mutex);
>   	return 0;
>   }
> @@ -673,7 +677,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;
>   	}
> @@ -1086,7 +1090,6 @@ static int hpd_event_thread(void *data)
>   
>   		switch (todo->event_id) {
>   		case EV_HPD_INIT_SETUP:
> -			dp_display_host_init(dp_priv);
>   			break;
>   		case EV_HPD_PLUG_INT:
>   			dp_hpd_plug_handle(dp_priv, todo->data);
> @@ -1264,6 +1267,10 @@ static int dp_display_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, &dp->dp_display);
>   
> +	rc = devm_pm_runtime_enable(&pdev->dev);
> +	if (rc)
> +		goto err;
> +
>   	rc = dp_display_request_irq(dp);

Here we have a problem now. The device can be runtime suspended, ceasing 
register access. However the IRQ enablement status is no longer 
synchronised to the DP's runtime status. Thus, if for some reason the 
IRQ handler is triggered, we can get unclocked register access / sync 
error.

Please correct me if I'm wrong, but I think that the request_irq should 
also get the IRQF_NO_AUTOEN flag and the suspend / resume paths should 
also have enable_irq() / disable_irq() calls.

>   	if (rc)
>   		goto err;
> @@ -1293,109 +1300,36 @@ static int dp_display_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> -static int dp_pm_resume(struct device *dev)
> +static int dp_pm_runtime_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;
> -	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;
> +	struct dp_display_private *dp = dev_get_dp_display_private(dev);
>   
> +	if (dp->dp_display.is_edp) {

Why is it only for eDP case?

My expectation is that if the DP device is suspended, then the PHY and 
the host parts are disabled. The driver, panel-edp / DP_AUX and the DRM 
framework should take care of resuming the DP device at the right time.


>   		dp_display_host_phy_exit(dp);
> +		dp_catalog_ctrl_hpd_disable(dp->catalog);
>   	}
> -
> -	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);
> +	dp_display_host_deinit(dp);
>   
>   	return 0;
>   }
>   
> -static int dp_pm_suspend(struct device *dev)
> +static int dp_pm_runtime_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;
> -
> -	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);
> +	struct dp_display_private *dp = dev_get_dp_display_private(dev);
>   
> -	mutex_unlock(&dp->event_mutex);
> +	dp_display_host_init(dp);
> +	if (dp->dp_display.is_edp) {

Why?

> +		dp_catalog_ctrl_hpd_enable(dp->catalog);
> +		dp_display_host_phy_init(dp);
> +	}
>   
>   	return 0;
>   }
>   
>   static const struct dev_pm_ops dp_pm_ops = {
> -	.suspend = dp_pm_suspend,
> -	.resume =  dp_pm_resume,
> +	SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                                pm_runtime_force_resume)
>   };
>   
>   static struct platform_driver dp_display_driver = {
> @@ -1478,10 +1412,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>   	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>   
>   	if (aux_bus && dp->is_edp) {
> -		dp_display_host_init(dp_priv);
> -		dp_catalog_ctrl_hpd_enable(dp_priv->catalog);
> -		dp_display_host_phy_init(dp_priv);
> -
>   		/*
>   		 * The code below assumes that the panel will finish probing
>   		 * by the time devm_of_dp_aux_populate_ep_devices() returns.
> @@ -1583,6 +1513,11 @@ void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>   		dp_hpd_plug_handle(dp_display, 0);
>   
>   	mutex_lock(&dp_display->event_mutex);
> +	if (pm_runtime_resume_and_get(&dp_display->pdev->dev)) {
> +		DRM_ERROR("failed to pm_runtime_resume\n");
> +		mutex_unlock(&dp_display->event_mutex);
> +		return;
> +	}
>   
>   	state = dp_display->hpd_state;
>   	if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
> @@ -1647,10 +1582,9 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
>   	mutex_lock(&dp_display->event_mutex);
>   
>   	state = dp_display->hpd_state;
> -	if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED) {
> -		mutex_unlock(&dp_display->event_mutex);
> -		return;
> -	}
> +	if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED)
> +		drm_dbg_dp(dp->drm_dev, "type=%d wrong hpd_state=%d\n",
> +			   dp->connector_type, state);
>   
>   	dp_display_disable(dp_display);
>   
> @@ -1663,6 +1597,8 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
>   	}
>   
>   	drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
> +
> +	pm_runtime_put_sync(&dp_display->pdev->dev);
>   	mutex_unlock(&dp_display->event_mutex);
>   }
>   
> @@ -1702,6 +1638,12 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
>   	struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
>   
>   	mutex_lock(&dp->event_mutex);
> +	if (pm_runtime_resume_and_get(&dp->pdev->dev)) {
> +		DRM_ERROR("failed to start power\n");
> +		mutex_unlock(&dp->event_mutex);
> +		return;
> +	}
> +
>   	dp_catalog_ctrl_hpd_enable(dp->catalog);
>   
>   	/* enable HDP interrupts */
> @@ -1723,6 +1665,9 @@ 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;

As I wrote in the comment to the previous series, this is incorrect. The 
.hpd_disable() doesn't mean that the display gets disconnected. Neither 
does .hpd_enable().

> +
> +	pm_runtime_put_sync(&dp->pdev->dev);
>   	mutex_unlock(&dp->event_mutex);
>   }
>   
> @@ -1737,13 +1682,10 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
>   	if (dp_display->internal_hpd)
>   		return;
>   
> -	if (!dp->core_initialized) {
> -		drm_dbg_dp(dp->drm_dev, "not initialized\n");
> -		return;
> -	}
> -
> -	if (!dp_display->link_ready && status == connector_status_connected)
> +	if (!dp_display->link_ready && status == connector_status_connected) {
> +		dp->hpd_state = ST_DISCONNECTED;
>   		dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> -	else if (dp_display->link_ready && status == connector_status_disconnected)
> +	} else if (dp_display->link_ready && status == connector_status_disconnected) {
>   		dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> +	}
>   }
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index 5cb84ca..863c766 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -152,20 +152,9 @@ int dp_power_client_init(struct dp_power *dp_power)
>   
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
> -	pm_runtime_enable(power->dev);
> -
>   	return dp_power_clk_init(power);
>   }
>   
> -void dp_power_client_deinit(struct dp_power *dp_power)
> -{
> -	struct dp_power_private *power;
> -
> -	power = container_of(dp_power, struct dp_power_private, dp_power);
> -
> -	pm_runtime_disable(power->dev);
> -}
> -
>   int dp_power_init(struct dp_power *dp_power)
>   {
>   	int rc = 0;
> @@ -173,11 +162,7 @@ int dp_power_init(struct dp_power *dp_power)
>   
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
> -	pm_runtime_get_sync(power->dev);
> -
>   	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> -	if (rc)
> -		pm_runtime_put_sync(power->dev);
>   
>   	return rc;
>   }
> @@ -189,7 +174,6 @@ int dp_power_deinit(struct dp_power *dp_power)
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	dp_power_clk_enable(dp_power, DP_CORE_PM, false);
> -	pm_runtime_put_sync(power->dev);
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.h b/drivers/gpu/drm/msm/dp/dp_power.h
> index a3dec20..55ada51 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.h
> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
> @@ -81,17 +81,6 @@ int dp_power_clk_enable(struct dp_power *power, enum dp_pm_type pm_type,
>   int dp_power_client_init(struct dp_power *power);
>   
>   /**
> - * dp_power_clinet_deinit() - de-initialize clock and regulator modules
> - *
> - * @power: instance of power module
> - * return: 0 for success, error for failure.
> - *
> - * This API will de-initialize the DisplayPort's clocks and regulator
> - * modules.
> - */
> -void dp_power_client_deinit(struct dp_power *power);
> -
> -/**
>    * dp_power_get() - configure and get the DisplayPort power module data
>    *
>    * @parser: instance of parser module
  

On 10/6/2023 4:56 AM, Dmitry Baryshkov wrote:
> On 04/10/2023 19:26, Kuogee Hsieh wrote:
>> Currently DP driver is executed independent of PM runtime framework.
>> This leads msm eDP panel can not being detected by edp_panel driver 
>> during
>> generic_edp_panel_probe() due to AUX DPCD read failed at edp panel 
>> driver.
>> Incorporate PM runtime framework into DP driver so that host 
>> controller's
>> power and clocks are enable/disable through PM runtime mechanism.
>> Once PM runtime framework is incorporated into DP driver, waking up 
>> device
>> from power up path is not necessary. Hence remove it.
>>
>> After incorporating pm_runtime framework into eDP/DP driver, 
>> 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. Therefore both dp_pm_suspend() and dp_pm_resume() are
>> dropped and replace with dp_pm_runtime_suspend() and 
>> dp_pm_runtime_resume()
>> respectively.
>>
>> Changes in v5:
>> -- remove pm_runtime_put_autosuspend feature, use 
>> pm_runtime_put_sync() directly
>> -- squash add pm_runtime_force_suspend()/resume() patch into this patch
>
> Ok, this is much better now. Thank you!
>
>>
>> Changes in v4:
>> -- reworded commit text to explain why pm_framework is required for 
>> edp panel
>> -- reworded commit text to explain autosuspend is choiced
>> -- delete EV_POWER_PM_GET and PM_EV_POWER_PUT from changes #3
>> -- delete dp_display_pm_get() and dp_display_pm_Put() from changes #3
>> -- return value from pm_runtime_resume_and_get() directly
>> -- check return value of devm_pm_runtime_enable()
>> -- delete pm_runtime_xxx from dp_display_remove()
>> -- drop dp_display_host_init() from EV_HPD_INIT_SETUP
>> -- 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:
>> -- incorporate removing pm_runtime_xx() from dp_pwer.c to this patch
>> -- use pm_runtime_resume_and_get() instead of pm_runtime_get()
>> -- error checking pm_runtime_resume_and_get() return value
>> -- add EV_POWER_PM_GET and PM_EV_POWER_PUT to handle HPD_GPIO case
>> -- 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>
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
>>   drivers/gpu/drm/msm/dp/dp_display.c | 166 
>> ++++++++++++------------------------
>>   drivers/gpu/drm/msm/dp/dp_power.c   |  16 ----
>>   drivers/gpu/drm/msm/dp/dp_power.h   |  11 ---
>>   4 files changed, 59 insertions(+), 139 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 8e3b677..10b6eeb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -291,6 +291,10 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>> *dp_aux,
>>           return -EINVAL;
>>       }
>>   +    ret = pm_runtime_resume_and_get(dp_aux->dev);
>> +    if (ret)
>> +        return  ret;
>> +
>>       mutex_lock(&aux->mutex);
>>       if (!aux->initted) {
>>           ret = -EIO;
>> @@ -364,6 +368,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>> *dp_aux,
>>     exit:
>>       mutex_unlock(&aux->mutex);
>> +    pm_runtime_put_sync(dp_aux->dev);
>>         return ret;
>>   }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index e4942fc..42efe148b 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 {
>> @@ -310,15 +309,10 @@ static void dp_display_unbind(struct device 
>> *dev, struct device *master,
>>       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>       struct msm_drm_private *priv = dev_get_drvdata(master);
>>   -    /* disable all HPD interrupts */
>> -    if (dp->core_initialized)
>> -        dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, 
>> false);
>> -
>>       kthread_stop(dp->ev_tsk);
>>         of_dp_aux_depopulate_bus(dp->aux);
>>   -    dp_power_client_deinit(dp->power);
>>       dp_unregister_audio_driver(dev, dp->audio);
>>       dp_aux_unregister(dp->aux);
>>       dp->drm_dev = NULL;
>> @@ -559,7 +553,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;
>>       }
>> @@ -576,6 +570,14 @@ static int dp_hpd_plug_handle(struct 
>> dp_display_private *dp, u32 data)
>>           return 0;
>>       }
>>   +    if (!dp->dp_display.is_edp) {
>
> Why?

This is mainly for hpd_internal == false case (any possible way of 
delivering an HPD event)

for both eDP and "hpd_internal == true" cases, 
pm_runtime_resume_and_get() had been called already.

will it better? if it was  removed the condition (dp->dp_display.is_edp) 
check?

>
>> +        ret = pm_runtime_resume_and_get(&dp->pdev->dev);
>> +        if (ret) {
>> +            DRM_ERROR("failed to pm_runtime_resume\n");
>> +            return ret;
>> +        }
>> +    }
>> +
>>       ret = dp_display_usbpd_configure_cb(&dp->pdev->dev);
>>       if (ret) {    /* link train failed */
>>           dp->hpd_state = ST_DISCONNECTED;
>> @@ -658,6 +660,8 @@ static int dp_hpd_unplug_handle(struct 
>> dp_display_private *dp, u32 data)
>>               dp->dp_display.connector_type, state);
>>         /* uevent will complete disconnection part */
>> +    if (!dp->dp_display.is_edp)
>> +        pm_runtime_put_sync(&dp->pdev->dev);
>>       mutex_unlock(&dp->event_mutex);
>>       return 0;
>>   }
>> @@ -673,7 +677,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;
>>       }
>> @@ -1086,7 +1090,6 @@ static int hpd_event_thread(void *data)
>>             switch (todo->event_id) {
>>           case EV_HPD_INIT_SETUP:
>> -            dp_display_host_init(dp_priv);
>>               break;
>>           case EV_HPD_PLUG_INT:
>>               dp_hpd_plug_handle(dp_priv, todo->data);
>> @@ -1264,6 +1267,10 @@ static int dp_display_probe(struct 
>> platform_device *pdev)
>>         platform_set_drvdata(pdev, &dp->dp_display);
>>   +    rc = devm_pm_runtime_enable(&pdev->dev);
>> +    if (rc)
>> +        goto err;
>> +
>>       rc = dp_display_request_irq(dp);
>
> Here we have a problem now. The device can be runtime suspended, 
> ceasing register access. However the IRQ enablement status is no 
> longer synchronised to the DP's runtime status. Thus, if for some 
> reason the IRQ handler is triggered, we can get unclocked register 
> access / sync error.
>
> Please correct me if I'm wrong, but I think that the request_irq 
> should also get the IRQF_NO_AUTOEN flag and the suspend / resume paths 
> should also have enable_irq() / disable_irq() calls.
>
got it.


>>       if (rc)
>>           goto err;
>> @@ -1293,109 +1300,36 @@ static int dp_display_remove(struct 
>> platform_device *pdev)
>>       return 0;
>>   }
>>   -static int dp_pm_resume(struct device *dev)
>> +static int dp_pm_runtime_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;
>> -    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;
>> +    struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>   +    if (dp->dp_display.is_edp) {
>
> Why is it only for eDP case?
>
> My expectation is that if the DP device is suspended, then the PHY and 
> the host parts are disabled. The driver, panel-edp / DP_AUX and the 
> DRM framework should take care of resuming the DP device at the right 
> time.
>
eDP and DP are implemented differently.

for external DP case,

1) dp_pm_runtime_resume() only enable dp host.

2) at dp_bridge_hpd_enable(), enable hdp block only and have hpd irq 
enabled and start waiting for plugin interrupt.  but leave phy is not 
initialized.

3) DP phy is initialized at plugin interrupt handle at the time right 
before link training.

for eDP case, hpd irq is always disable (no hpd irq required for eDP).

1) DP host, HDP block and phy are enabled (initialized) at same time at 
dp_pm_runtime_resume()



           dp_display_host_phy_exit(dp);
>> + dp_catalog_ctrl_hpd_disable(dp->catalog);
>>       }
>> -
>> -    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);
>> +    dp_display_host_deinit(dp);
>>         return 0;
>>   }
>>   -static int dp_pm_suspend(struct device *dev)
>> +static int dp_pm_runtime_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;
>> -
>> -    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);
>> +    struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>   -    mutex_unlock(&dp->event_mutex);
>> +    dp_display_host_init(dp);
>> +    if (dp->dp_display.is_edp) {
>
> Why?
Same as above comments.
>
>> + dp_catalog_ctrl_hpd_enable(dp->catalog);
>> +        dp_display_host_phy_init(dp);
>> +    }
>>         return 0;
>>   }
>>     static const struct dev_pm_ops dp_pm_ops = {
>> -    .suspend = dp_pm_suspend,
>> -    .resume =  dp_pm_resume,
>> +    SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, 
>> NULL)
>> +    SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +                                pm_runtime_force_resume)
>>   };
>>     static struct platform_driver dp_display_driver = {
>> @@ -1478,10 +1412,6 @@ static int dp_display_get_next_bridge(struct 
>> msm_dp *dp)
>>       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>>         if (aux_bus && dp->is_edp) {
>> -        dp_display_host_init(dp_priv);
>> -        dp_catalog_ctrl_hpd_enable(dp_priv->catalog);
>> -        dp_display_host_phy_init(dp_priv);
>> -
>>           /*
>>            * The code below assumes that the panel will finish probing
>>            * by the time devm_of_dp_aux_populate_ep_devices() returns.
>> @@ -1583,6 +1513,11 @@ void dp_bridge_atomic_enable(struct drm_bridge 
>> *drm_bridge,
>>           dp_hpd_plug_handle(dp_display, 0);
>>         mutex_lock(&dp_display->event_mutex);
>> +    if (pm_runtime_resume_and_get(&dp_display->pdev->dev)) {
>> +        DRM_ERROR("failed to pm_runtime_resume\n");
>> +        mutex_unlock(&dp_display->event_mutex);
>> +        return;
>> +    }
>>         state = dp_display->hpd_state;
>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>> @@ -1647,10 +1582,9 @@ void dp_bridge_atomic_post_disable(struct 
>> drm_bridge *drm_bridge,
>>       mutex_lock(&dp_display->event_mutex);
>>         state = dp_display->hpd_state;
>> -    if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED) {
>> -        mutex_unlock(&dp_display->event_mutex);
>> -        return;
>> -    }
>> +    if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED)
>> +        drm_dbg_dp(dp->drm_dev, "type=%d wrong hpd_state=%d\n",
>> +               dp->connector_type, state);
>>         dp_display_disable(dp_display);
>>   @@ -1663,6 +1597,8 @@ void dp_bridge_atomic_post_disable(struct 
>> drm_bridge *drm_bridge,
>>       }
>>         drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
>> +
>> +    pm_runtime_put_sync(&dp_display->pdev->dev);
>>       mutex_unlock(&dp_display->event_mutex);
>>   }
>>   @@ -1702,6 +1638,12 @@ void dp_bridge_hpd_enable(struct drm_bridge 
>> *bridge)
>>       struct dp_display_private *dp = container_of(dp_display, struct 
>> dp_display_private, dp_display);
>>         mutex_lock(&dp->event_mutex);
>> +    if (pm_runtime_resume_and_get(&dp->pdev->dev)) {
>> +        DRM_ERROR("failed to start power\n");
>> +        mutex_unlock(&dp->event_mutex);
>> +        return;
>> +    }
>> +
>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>         /* enable HDP interrupts */
>> @@ -1723,6 +1665,9 @@ 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;
>
> As I wrote in the comment to the previous series, this is incorrect. 
> The .hpd_disable() doesn't mean that the display gets disconnected. 
> Neither does .hpd_enable().
ok,
>
>> +
>> +    pm_runtime_put_sync(&dp->pdev->dev);
>>       mutex_unlock(&dp->event_mutex);
>>   }
>>   @@ -1737,13 +1682,10 @@ void dp_bridge_hpd_notify(struct drm_bridge 
>> *bridge,
>>       if (dp_display->internal_hpd)
>>           return;
>>   -    if (!dp->core_initialized) {
>> -        drm_dbg_dp(dp->drm_dev, "not initialized\n");
>> -        return;
>> -    }
>> -
>> -    if (!dp_display->link_ready && status == 
>> connector_status_connected)
>> +    if (!dp_display->link_ready && status == 
>> connector_status_connected) {
>> +        dp->hpd_state = ST_DISCONNECTED;
>>           dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
>> -    else if (dp_display->link_ready && status == 
>> connector_status_disconnected)
>> +    } else if (dp_display->link_ready && status == 
>> connector_status_disconnected) {
>>           dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
>> +    }
>>   }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c 
>> b/drivers/gpu/drm/msm/dp/dp_power.c
>> index 5cb84ca..863c766 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>> @@ -152,20 +152,9 @@ int dp_power_client_init(struct dp_power *dp_power)
>>         power = container_of(dp_power, struct dp_power_private, 
>> dp_power);
>>   -    pm_runtime_enable(power->dev);
>> -
>>       return dp_power_clk_init(power);
>>   }
>>   -void dp_power_client_deinit(struct dp_power *dp_power)
>> -{
>> -    struct dp_power_private *power;
>> -
>> -    power = container_of(dp_power, struct dp_power_private, dp_power);
>> -
>> -    pm_runtime_disable(power->dev);
>> -}
>> -
>>   int dp_power_init(struct dp_power *dp_power)
>>   {
>>       int rc = 0;
>> @@ -173,11 +162,7 @@ int dp_power_init(struct dp_power *dp_power)
>>         power = container_of(dp_power, struct dp_power_private, 
>> dp_power);
>>   -    pm_runtime_get_sync(power->dev);
>> -
>>       rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
>> -    if (rc)
>> -        pm_runtime_put_sync(power->dev);
>>         return rc;
>>   }
>> @@ -189,7 +174,6 @@ int dp_power_deinit(struct dp_power *dp_power)
>>       power = container_of(dp_power, struct dp_power_private, dp_power);
>>         dp_power_clk_enable(dp_power, DP_CORE_PM, false);
>> -    pm_runtime_put_sync(power->dev);
>>       return 0;
>>   }
>>   diff --git a/drivers/gpu/drm/msm/dp/dp_power.h 
>> b/drivers/gpu/drm/msm/dp/dp_power.h
>> index a3dec20..55ada51 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_power.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
>> @@ -81,17 +81,6 @@ int dp_power_clk_enable(struct dp_power *power, 
>> enum dp_pm_type pm_type,
>>   int dp_power_client_init(struct dp_power *power);
>>     /**
>> - * dp_power_clinet_deinit() - de-initialize clock and regulator modules
>> - *
>> - * @power: instance of power module
>> - * return: 0 for success, error for failure.
>> - *
>> - * This API will de-initialize the DisplayPort's clocks and regulator
>> - * modules.
>> - */
>> -void dp_power_client_deinit(struct dp_power *power);
>> -
>> -/**
>>    * dp_power_get() - configure and get the DisplayPort power module 
>> data
>>    *
>>    * @parser: instance of parser module
>
  

On Wed, 4 Oct 2023 at 19:27, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Currently DP driver is executed independent of PM runtime framework.
> This leads msm eDP panel can not being detected by edp_panel driver during
> generic_edp_panel_probe() due to AUX DPCD read failed at edp panel driver.
> Incorporate PM runtime framework into DP driver so that host controller's
> power and clocks are enable/disable through PM runtime mechanism.
> Once PM runtime framework is incorporated into DP driver, waking up device
> from power up path is not necessary. Hence remove it.
>
> After incorporating pm_runtime framework into eDP/DP driver, 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. Therefore both dp_pm_suspend() and dp_pm_resume() are
> dropped and replace with dp_pm_runtime_suspend() and dp_pm_runtime_resume()
> respectively.
>
> Changes in v5:
> -- remove pm_runtime_put_autosuspend feature, use pm_runtime_put_sync() directly
> -- squash add pm_runtime_force_suspend()/resume() patch into this patch
>
> Changes in v4:
> -- reworded commit text to explain why pm_framework is required for edp panel
> -- reworded commit text to explain autosuspend is choiced
> -- delete EV_POWER_PM_GET and PM_EV_POWER_PUT from changes #3
> -- delete dp_display_pm_get() and dp_display_pm_Put() from changes #3
> -- return value from pm_runtime_resume_and_get() directly
> -- check return value of devm_pm_runtime_enable()
> -- delete pm_runtime_xxx from dp_display_remove()
> -- drop dp_display_host_init() from EV_HPD_INIT_SETUP
> -- 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:
> -- incorporate removing pm_runtime_xx() from dp_pwer.c to this patch
> -- use pm_runtime_resume_and_get() instead of pm_runtime_get()
> -- error checking pm_runtime_resume_and_get() return value
> -- add EV_POWER_PM_GET and PM_EV_POWER_PUT to handle HPD_GPIO case
> -- 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>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
>  drivers/gpu/drm/msm/dp/dp_display.c | 166 ++++++++++++------------------------
>  drivers/gpu/drm/msm/dp/dp_power.c   |  16 ----
>  drivers/gpu/drm/msm/dp/dp_power.h   |  11 ---
>  4 files changed, 59 insertions(+), 139 deletions(-)

[skipped]

> @@ -1702,6 +1638,12 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
>         struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
>
>         mutex_lock(&dp->event_mutex);
> +       if (pm_runtime_resume_and_get(&dp->pdev->dev)) {
> +               DRM_ERROR("failed to start power\n");

It was in the previous review comment:

"failed to resume".

> +               mutex_unlock(&dp->event_mutex);
> +               return;
> +       }
> +
  

On 06/10/2023 19:42, Kuogee Hsieh wrote:
> 
> On 10/6/2023 4:56 AM, Dmitry Baryshkov wrote:
>> On 04/10/2023 19:26, Kuogee Hsieh wrote:
>>> Currently DP driver is executed independent of PM runtime framework.
>>> This leads msm eDP panel can not being detected by edp_panel driver 
>>> during
>>> generic_edp_panel_probe() due to AUX DPCD read failed at edp panel 
>>> driver.
>>> Incorporate PM runtime framework into DP driver so that host 
>>> controller's
>>> power and clocks are enable/disable through PM runtime mechanism.
>>> Once PM runtime framework is incorporated into DP driver, waking up 
>>> device
>>> from power up path is not necessary. Hence remove it.
>>>
>>> After incorporating pm_runtime framework into eDP/DP driver, 
>>> 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. Therefore both dp_pm_suspend() and dp_pm_resume() are
>>> dropped and replace with dp_pm_runtime_suspend() and 
>>> dp_pm_runtime_resume()
>>> respectively.
>>>
>>> Changes in v5:
>>> -- remove pm_runtime_put_autosuspend feature, use 
>>> pm_runtime_put_sync() directly
>>> -- squash add pm_runtime_force_suspend()/resume() patch into this patch
>>
>> Ok, this is much better now. Thank you!
>>
>>>
>>> Changes in v4:
>>> -- reworded commit text to explain why pm_framework is required for 
>>> edp panel
>>> -- reworded commit text to explain autosuspend is choiced
>>> -- delete EV_POWER_PM_GET and PM_EV_POWER_PUT from changes #3
>>> -- delete dp_display_pm_get() and dp_display_pm_Put() from changes #3
>>> -- return value from pm_runtime_resume_and_get() directly
>>> -- check return value of devm_pm_runtime_enable()
>>> -- delete pm_runtime_xxx from dp_display_remove()
>>> -- drop dp_display_host_init() from EV_HPD_INIT_SETUP
>>> -- 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:
>>> -- incorporate removing pm_runtime_xx() from dp_pwer.c to this patch
>>> -- use pm_runtime_resume_and_get() instead of pm_runtime_get()
>>> -- error checking pm_runtime_resume_and_get() return value
>>> -- add EV_POWER_PM_GET and PM_EV_POWER_PUT to handle HPD_GPIO case
>>> -- 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>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
>>>   drivers/gpu/drm/msm/dp/dp_display.c | 166 
>>> ++++++++++++------------------------
>>>   drivers/gpu/drm/msm/dp/dp_power.c   |  16 ----
>>>   drivers/gpu/drm/msm/dp/dp_power.h   |  11 ---
>>>   4 files changed, 59 insertions(+), 139 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> index 8e3b677..10b6eeb 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> @@ -291,6 +291,10 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>>> *dp_aux,
>>>           return -EINVAL;
>>>       }
>>>   +    ret = pm_runtime_resume_and_get(dp_aux->dev);
>>> +    if (ret)
>>> +        return  ret;
>>> +
>>>       mutex_lock(&aux->mutex);
>>>       if (!aux->initted) {
>>>           ret = -EIO;
>>> @@ -364,6 +368,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>>> *dp_aux,
>>>     exit:
>>>       mutex_unlock(&aux->mutex);
>>> +    pm_runtime_put_sync(dp_aux->dev);
>>>         return ret;
>>>   }
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index e4942fc..42efe148b 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 {
>>> @@ -310,15 +309,10 @@ static void dp_display_unbind(struct device 
>>> *dev, struct device *master,
>>>       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>>       struct msm_drm_private *priv = dev_get_drvdata(master);
>>>   -    /* disable all HPD interrupts */
>>> -    if (dp->core_initialized)
>>> -        dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, 
>>> false);
>>> -
>>>       kthread_stop(dp->ev_tsk);
>>>         of_dp_aux_depopulate_bus(dp->aux);
>>>   -    dp_power_client_deinit(dp->power);
>>>       dp_unregister_audio_driver(dev, dp->audio);
>>>       dp_aux_unregister(dp->aux);
>>>       dp->drm_dev = NULL;
>>> @@ -559,7 +553,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;
>>>       }
>>> @@ -576,6 +570,14 @@ static int dp_hpd_plug_handle(struct 
>>> dp_display_private *dp, u32 data)
>>>           return 0;
>>>       }
>>>   +    if (!dp->dp_display.is_edp) {
>>
>> Why?
> 
> This is mainly for hpd_internal == false case (any possible way of 
> delivering an HPD event)
> 
> for both eDP and "hpd_internal == true" cases, 
> pm_runtime_resume_and_get() had been called already.
> 
> will it better? if it was  removed the condition (dp->dp_display.is_edp) 
> check?

Yes, this codepath seems generic.

> 
>>
>>> +        ret = pm_runtime_resume_and_get(&dp->pdev->dev);
>>> +        if (ret) {
>>> +            DRM_ERROR("failed to pm_runtime_resume\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>>       ret = dp_display_usbpd_configure_cb(&dp->pdev->dev);
>>>       if (ret) {    /* link train failed */
>>>           dp->hpd_state = ST_DISCONNECTED;
>>> @@ -658,6 +660,8 @@ static int dp_hpd_unplug_handle(struct 
>>> dp_display_private *dp, u32 data)
>>>               dp->dp_display.connector_type, state);
>>>         /* uevent will complete disconnection part */
>>> +    if (!dp->dp_display.is_edp)
>>> +        pm_runtime_put_sync(&dp->pdev->dev);
>>>       mutex_unlock(&dp->event_mutex);
>>>       return 0;
>>>   }
>>> @@ -673,7 +677,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;
>>>       }
>>> @@ -1086,7 +1090,6 @@ static int hpd_event_thread(void *data)
>>>             switch (todo->event_id) {
>>>           case EV_HPD_INIT_SETUP:
>>> -            dp_display_host_init(dp_priv);
>>>               break;
>>>           case EV_HPD_PLUG_INT:
>>>               dp_hpd_plug_handle(dp_priv, todo->data);
>>> @@ -1264,6 +1267,10 @@ static int dp_display_probe(struct 
>>> platform_device *pdev)
>>>         platform_set_drvdata(pdev, &dp->dp_display);
>>>   +    rc = devm_pm_runtime_enable(&pdev->dev);
>>> +    if (rc)
>>> +        goto err;
>>> +
>>>       rc = dp_display_request_irq(dp);
>>
>> Here we have a problem now. The device can be runtime suspended, 
>> ceasing register access. However the IRQ enablement status is no 
>> longer synchronised to the DP's runtime status. Thus, if for some 
>> reason the IRQ handler is triggered, we can get unclocked register 
>> access / sync error.
>>
>> Please correct me if I'm wrong, but I think that the request_irq 
>> should also get the IRQF_NO_AUTOEN flag and the suspend / resume paths 
>> should also have enable_irq() / disable_irq() calls.
>>
> got it.
> 
> 
>>>       if (rc)
>>>           goto err;
>>> @@ -1293,109 +1300,36 @@ static int dp_display_remove(struct 
>>> platform_device *pdev)
>>>       return 0;
>>>   }
>>>   -static int dp_pm_resume(struct device *dev)
>>> +static int dp_pm_runtime_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;
>>> -    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;
>>> +    struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>>   +    if (dp->dp_display.is_edp) {
>>
>> Why is it only for eDP case?
>>
>> My expectation is that if the DP device is suspended, then the PHY and 
>> the host parts are disabled. The driver, panel-edp / DP_AUX and the 
>> DRM framework should take care of resuming the DP device at the right 
>> time.
>>
> eDP and DP are implemented differently.
> 
> for external DP case,
> 
> 1) dp_pm_runtime_resume() only enable dp host.
> 
> 2) at dp_bridge_hpd_enable(), enable hdp block only and have hpd irq 
> enabled and start waiting for plugin interrupt.  but leave phy is not 
> initialized.
> 
> 3) DP phy is initialized at plugin interrupt handle at the time right 
> before link training.
> 
> for eDP case, hpd irq is always disable (no hpd irq required for eDP).
> 
> 1) DP host, HDP block and phy are enabled (initialized) at same time at 
> dp_pm_runtime_resume()

Ack. thanks. If you can add this to the comment somewhere in the file, 
that will be great. Also just for my understanding: The function 
dp_catalog_ctrl_hpd_enable() only enables handling of the HPD pin, it 
has no other side effects? So for the DP case if we have external HPD 
events, we don't need to call it, correct?

Also, could you please add a followup patch that drop 
dp_display->internal_hpd and always uses the same codepath for both 
cases? Would that be a logical change after this series?


>            dp_display_host_phy_exit(dp);
>>> + dp_catalog_ctrl_hpd_disable(dp->catalog);
>>>       }
>>> -
>>> -    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);
>>> +    dp_display_host_deinit(dp);
>>>         return 0;
>>>   }
>>>   -static int dp_pm_suspend(struct device *dev)
>>> +static int dp_pm_runtime_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;
>>> -
>>> -    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);
>>> +    struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>>   -    mutex_unlock(&dp->event_mutex);
>>> +    dp_display_host_init(dp);
>>> +    if (dp->dp_display.is_edp) {
>>
>> Why?
> Same as above comments.

Ack

>>
>>> + dp_catalog_ctrl_hpd_enable(dp->catalog);
>>> +        dp_display_host_phy_init(dp);
>>> +    }
>>>         return 0;
>>>   }
>>>     static const struct dev_pm_ops dp_pm_ops = {
>>> -    .suspend = dp_pm_suspend,
>>> -    .resume =  dp_pm_resume,
>>> +    SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, 
>>> NULL)
>>> +    SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>> +                                pm_runtime_force_resume)
>>>   };
>>>     static struct platform_driver dp_display_driver = {
>>> @@ -1478,10 +1412,6 @@ static int dp_display_get_next_bridge(struct 
>>> msm_dp *dp)
>>>       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>>>         if (aux_bus && dp->is_edp) {
>>> -        dp_display_host_init(dp_priv);
>>> -        dp_catalog_ctrl_hpd_enable(dp_priv->catalog);
>>> -        dp_display_host_phy_init(dp_priv);
>>> -
>>>           /*
>>>            * The code below assumes that the panel will finish probing
>>>            * by the time devm_of_dp_aux_populate_ep_devices() returns.
>>> @@ -1583,6 +1513,11 @@ void dp_bridge_atomic_enable(struct drm_bridge 
>>> *drm_bridge,
>>>           dp_hpd_plug_handle(dp_display, 0);
>>>         mutex_lock(&dp_display->event_mutex);
>>> +    if (pm_runtime_resume_and_get(&dp_display->pdev->dev)) {
>>> +        DRM_ERROR("failed to pm_runtime_resume\n");
>>> +        mutex_unlock(&dp_display->event_mutex);
>>> +        return;
>>> +    }
>>>         state = dp_display->hpd_state;
>>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>>> @@ -1647,10 +1582,9 @@ void dp_bridge_atomic_post_disable(struct 
>>> drm_bridge *drm_bridge,
>>>       mutex_lock(&dp_display->event_mutex);
>>>         state = dp_display->hpd_state;
>>> -    if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED) {
>>> -        mutex_unlock(&dp_display->event_mutex);
>>> -        return;
>>> -    }
>>> +    if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED)
>>> +        drm_dbg_dp(dp->drm_dev, "type=%d wrong hpd_state=%d\n",
>>> +               dp->connector_type, state);
>>>         dp_display_disable(dp_display);
>>>   @@ -1663,6 +1597,8 @@ void dp_bridge_atomic_post_disable(struct 
>>> drm_bridge *drm_bridge,
>>>       }
>>>         drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
>>> +
>>> +    pm_runtime_put_sync(&dp_display->pdev->dev);
>>>       mutex_unlock(&dp_display->event_mutex);
>>>   }
>>>   @@ -1702,6 +1638,12 @@ void dp_bridge_hpd_enable(struct drm_bridge 
>>> *bridge)
>>>       struct dp_display_private *dp = container_of(dp_display, struct 
>>> dp_display_private, dp_display);
>>>         mutex_lock(&dp->event_mutex);
>>> +    if (pm_runtime_resume_and_get(&dp->pdev->dev)) {
>>> +        DRM_ERROR("failed to start power\n");
>>> +        mutex_unlock(&dp->event_mutex);
>>> +        return;
>>> +    }
>>> +
>>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>         /* enable HDP interrupts */
>>> @@ -1723,6 +1665,9 @@ 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;
>>
>> As I wrote in the comment to the previous series, this is incorrect. 
>> The .hpd_disable() doesn't mean that the display gets disconnected. 
>> Neither does .hpd_enable().
> ok,
>>
>>> +
>>> +    pm_runtime_put_sync(&dp->pdev->dev);
>>>       mutex_unlock(&dp->event_mutex);
>>>   }
>>>   @@ -1737,13 +1682,10 @@ void dp_bridge_hpd_notify(struct drm_bridge 
>>> *bridge,
>>>       if (dp_display->internal_hpd)
>>>           return;
>>>   -    if (!dp->core_initialized) {
>>> -        drm_dbg_dp(dp->drm_dev, "not initialized\n");
>>> -        return;
>>> -    }
>>> -
>>> -    if (!dp_display->link_ready && status == 
>>> connector_status_connected)
>>> +    if (!dp_display->link_ready && status == 
>>> connector_status_connected) {
>>> +        dp->hpd_state = ST_DISCONNECTED;
>>>           dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
>>> -    else if (dp_display->link_ready && status == 
>>> connector_status_disconnected)
>>> +    } else if (dp_display->link_ready && status == 
>>> connector_status_disconnected) {
>>>           dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
>>> +    }
>>>   }
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c 
>>> b/drivers/gpu/drm/msm/dp/dp_power.c
>>> index 5cb84ca..863c766 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>>> @@ -152,20 +152,9 @@ int dp_power_client_init(struct dp_power *dp_power)
>>>         power = container_of(dp_power, struct dp_power_private, 
>>> dp_power);
>>>   -    pm_runtime_enable(power->dev);
>>> -
>>>       return dp_power_clk_init(power);
>>>   }
>>>   -void dp_power_client_deinit(struct dp_power *dp_power)
>>> -{
>>> -    struct dp_power_private *power;
>>> -
>>> -    power = container_of(dp_power, struct dp_power_private, dp_power);
>>> -
>>> -    pm_runtime_disable(power->dev);
>>> -}
>>> -
>>>   int dp_power_init(struct dp_power *dp_power)
>>>   {
>>>       int rc = 0;
>>> @@ -173,11 +162,7 @@ int dp_power_init(struct dp_power *dp_power)
>>>         power = container_of(dp_power, struct dp_power_private, 
>>> dp_power);
>>>   -    pm_runtime_get_sync(power->dev);
>>> -
>>>       rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
>>> -    if (rc)
>>> -        pm_runtime_put_sync(power->dev);
>>>         return rc;
>>>   }
>>> @@ -189,7 +174,6 @@ int dp_power_deinit(struct dp_power *dp_power)
>>>       power = container_of(dp_power, struct dp_power_private, dp_power);
>>>         dp_power_clk_enable(dp_power, DP_CORE_PM, false);
>>> -    pm_runtime_put_sync(power->dev);
>>>       return 0;
>>>   }
>>>   diff --git a/drivers/gpu/drm/msm/dp/dp_power.h 
>>> b/drivers/gpu/drm/msm/dp/dp_power.h
>>> index a3dec20..55ada51 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_power.h
>>> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
>>> @@ -81,17 +81,6 @@ int dp_power_clk_enable(struct dp_power *power, 
>>> enum dp_pm_type pm_type,
>>>   int dp_power_client_init(struct dp_power *power);
>>>     /**
>>> - * dp_power_clinet_deinit() - de-initialize clock and regulator modules
>>> - *
>>> - * @power: instance of power module
>>> - * return: 0 for success, error for failure.
>>> - *
>>> - * This API will de-initialize the DisplayPort's clocks and regulator
>>> - * modules.
>>> - */
>>> -void dp_power_client_deinit(struct dp_power *power);
>>> -
>>> -/**
>>>    * dp_power_get() - configure and get the DisplayPort power module 
>>> data
>>>    *
>>>    * @parser: instance of parser module
>>
  

On 10/6/2023 11:04 AM, Dmitry Baryshkov wrote:
> On 06/10/2023 19:42, Kuogee Hsieh wrote:
>>
>> On 10/6/2023 4:56 AM, Dmitry Baryshkov wrote:
>>> On 04/10/2023 19:26, Kuogee Hsieh wrote:
>>>> Currently DP driver is executed independent of PM runtime framework.
>>>> This leads msm eDP panel can not being detected by edp_panel driver 
>>>> during
>>>> generic_edp_panel_probe() due to AUX DPCD read failed at edp panel 
>>>> driver.
>>>> Incorporate PM runtime framework into DP driver so that host 
>>>> controller's
>>>> power and clocks are enable/disable through PM runtime mechanism.
>>>> Once PM runtime framework is incorporated into DP driver, waking up 
>>>> device
>>>> from power up path is not necessary. Hence remove it.
>>>>
>>>> After incorporating pm_runtime framework into eDP/DP driver, 
>>>> 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. Therefore both dp_pm_suspend() and 
>>>> dp_pm_resume() are
>>>> dropped and replace with dp_pm_runtime_suspend() and 
>>>> dp_pm_runtime_resume()
>>>> respectively.
>>>>
>>>> Changes in v5:
>>>> -- remove pm_runtime_put_autosuspend feature, use 
>>>> pm_runtime_put_sync() directly
>>>> -- squash add pm_runtime_force_suspend()/resume() patch into this 
>>>> patch
>>>
>>> Ok, this is much better now. Thank you!
>>>
>>>>
>>>> Changes in v4:
>>>> -- reworded commit text to explain why pm_framework is required for 
>>>> edp panel
>>>> -- reworded commit text to explain autosuspend is choiced
>>>> -- delete EV_POWER_PM_GET and PM_EV_POWER_PUT from changes #3
>>>> -- delete dp_display_pm_get() and dp_display_pm_Put() from changes #3
>>>> -- return value from pm_runtime_resume_and_get() directly
>>>> -- check return value of devm_pm_runtime_enable()
>>>> -- delete pm_runtime_xxx from dp_display_remove()
>>>> -- drop dp_display_host_init() from EV_HPD_INIT_SETUP
>>>> -- 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:
>>>> -- incorporate removing pm_runtime_xx() from dp_pwer.c to this patch
>>>> -- use pm_runtime_resume_and_get() instead of pm_runtime_get()
>>>> -- error checking pm_runtime_resume_and_get() return value
>>>> -- add EV_POWER_PM_GET and PM_EV_POWER_PUT to handle HPD_GPIO case
>>>> -- 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>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
>>>>   drivers/gpu/drm/msm/dp/dp_display.c | 166 
>>>> ++++++++++++------------------------
>>>>   drivers/gpu/drm/msm/dp/dp_power.c   |  16 ----
>>>>   drivers/gpu/drm/msm/dp/dp_power.h   |  11 ---
>>>>   4 files changed, 59 insertions(+), 139 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> index 8e3b677..10b6eeb 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> @@ -291,6 +291,10 @@ static ssize_t dp_aux_transfer(struct 
>>>> drm_dp_aux *dp_aux,
>>>>           return -EINVAL;
>>>>       }
>>>>   +    ret = pm_runtime_resume_and_get(dp_aux->dev);
>>>> +    if (ret)
>>>> +        return  ret;
>>>> +
>>>>       mutex_lock(&aux->mutex);
>>>>       if (!aux->initted) {
>>>>           ret = -EIO;
>>>> @@ -364,6 +368,7 @@ static ssize_t dp_aux_transfer(struct 
>>>> drm_dp_aux *dp_aux,
>>>>     exit:
>>>>       mutex_unlock(&aux->mutex);
>>>> +    pm_runtime_put_sync(dp_aux->dev);
>>>>         return ret;
>>>>   }
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index e4942fc..42efe148b 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 {
>>>> @@ -310,15 +309,10 @@ static void dp_display_unbind(struct device 
>>>> *dev, struct device *master,
>>>>       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>>>       struct msm_drm_private *priv = dev_get_drvdata(master);
>>>>   -    /* disable all HPD interrupts */
>>>> -    if (dp->core_initialized)
>>>> -        dp_catalog_hpd_config_intr(dp->catalog, 
>>>> DP_DP_HPD_INT_MASK, false);
>>>> -
>>>>       kthread_stop(dp->ev_tsk);
>>>>         of_dp_aux_depopulate_bus(dp->aux);
>>>>   -    dp_power_client_deinit(dp->power);
>>>>       dp_unregister_audio_driver(dev, dp->audio);
>>>>       dp_aux_unregister(dp->aux);
>>>>       dp->drm_dev = NULL;
>>>> @@ -559,7 +553,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;
>>>>       }
>>>> @@ -576,6 +570,14 @@ static int dp_hpd_plug_handle(struct 
>>>> dp_display_private *dp, u32 data)
>>>>           return 0;
>>>>       }
>>>>   +    if (!dp->dp_display.is_edp) {
>>>
>>> Why?
>>
>> This is mainly for hpd_internal == false case (any possible way of 
>> delivering an HPD event)
>>
>> for both eDP and "hpd_internal == true" cases, 
>> pm_runtime_resume_and_get() had been called already.
>>
>> will it better? if it was  removed the condition 
>> (dp->dp_display.is_edp) check?
>
> Yes, this codepath seems generic.
>
>>
>>>
>>>> +        ret = pm_runtime_resume_and_get(&dp->pdev->dev);
>>>> +        if (ret) {
>>>> +            DRM_ERROR("failed to pm_runtime_resume\n");
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>>> +
>>>>       ret = dp_display_usbpd_configure_cb(&dp->pdev->dev);
>>>>       if (ret) {    /* link train failed */
>>>>           dp->hpd_state = ST_DISCONNECTED;
>>>> @@ -658,6 +660,8 @@ static int dp_hpd_unplug_handle(struct 
>>>> dp_display_private *dp, u32 data)
>>>>               dp->dp_display.connector_type, state);
>>>>         /* uevent will complete disconnection part */
>>>> +    if (!dp->dp_display.is_edp)
>>>> +        pm_runtime_put_sync(&dp->pdev->dev);
>>>>       mutex_unlock(&dp->event_mutex);
>>>>       return 0;
>>>>   }
>>>> @@ -673,7 +677,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;
>>>>       }
>>>> @@ -1086,7 +1090,6 @@ static int hpd_event_thread(void *data)
>>>>             switch (todo->event_id) {
>>>>           case EV_HPD_INIT_SETUP:
>>>> -            dp_display_host_init(dp_priv);
>>>>               break;
>>>>           case EV_HPD_PLUG_INT:
>>>>               dp_hpd_plug_handle(dp_priv, todo->data);
>>>> @@ -1264,6 +1267,10 @@ static int dp_display_probe(struct 
>>>> platform_device *pdev)
>>>>         platform_set_drvdata(pdev, &dp->dp_display);
>>>>   +    rc = devm_pm_runtime_enable(&pdev->dev);
>>>> +    if (rc)
>>>> +        goto err;
>>>> +
>>>>       rc = dp_display_request_irq(dp);
>>>
>>> Here we have a problem now. The device can be runtime suspended, 
>>> ceasing register access. However the IRQ enablement status is no 
>>> longer synchronised to the DP's runtime status. Thus, if for some 
>>> reason the IRQ handler is triggered, we can get unclocked register 
>>> access / sync error.
>>>
>>> Please correct me if I'm wrong, but I think that the request_irq 
>>> should also get the IRQF_NO_AUTOEN flag and the suspend / resume 
>>> paths should also have enable_irq() / disable_irq() calls.
>>>
>> got it.
>>
>>
>>>>       if (rc)
>>>>           goto err;
>>>> @@ -1293,109 +1300,36 @@ static int dp_display_remove(struct 
>>>> platform_device *pdev)
>>>>       return 0;
>>>>   }
>>>>   -static int dp_pm_resume(struct device *dev)
>>>> +static int dp_pm_runtime_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;
>>>> -    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;
>>>> +    struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>>>   +    if (dp->dp_display.is_edp) {
>>>
>>> Why is it only for eDP case?
>>>
>>> My expectation is that if the DP device is suspended, then the PHY 
>>> and the host parts are disabled. The driver, panel-edp / DP_AUX and 
>>> the DRM framework should take care of resuming the DP device at the 
>>> right time.
>>>
>> eDP and DP are implemented differently.
>>
>> for external DP case,
>>
>> 1) dp_pm_runtime_resume() only enable dp host.
>>
>> 2) at dp_bridge_hpd_enable(), enable hdp block only and have hpd irq 
>> enabled and start waiting for plugin interrupt.  but leave phy is not 
>> initialized.
>>
>> 3) DP phy is initialized at plugin interrupt handle at the time right 
>> before link training.
>>
>> for eDP case, hpd irq is always disable (no hpd irq required for eDP).
>>
>> 1) DP host, HDP block and phy are enabled (initialized) at same time 
>> at dp_pm_runtime_resume()
>
> Ack. thanks. If you can add this to the comment somewhere in the file, 
> that will be great. Also just for my understanding: The function 
> dp_catalog_ctrl_hpd_enable() only enables handling of the HPD pin, it 
> has no other side effects? So for the DP case if we have external HPD 
> events, we don't need to call it, correct?

yes, this is correct.  dp_catalog_ctrl_hpd_enable() and 
dp_catalog_hpd_config_intr() are only for the case depend on hpd interrupts.

>
> Also, could you please add a followup patch that drop 
> dp_display->internal_hpd and always uses the same codepath for both 
> cases? Would that be a logical change after this series?
ok, i will submit a patch to remove hpd_internal.
>
>
>>            dp_display_host_phy_exit(dp);
>>>> + dp_catalog_ctrl_hpd_disable(dp->catalog);
>>>>       }
>>>> -
>>>> -    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);
>>>> +    dp_display_host_deinit(dp);
>>>>         return 0;
>>>>   }
>>>>   -static int dp_pm_suspend(struct device *dev)
>>>> +static int dp_pm_runtime_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;
>>>> -
>>>> -    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);
>>>> +    struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>>>   -    mutex_unlock(&dp->event_mutex);
>>>> +    dp_display_host_init(dp);
>>>> +    if (dp->dp_display.is_edp) {
>>>
>>> Why?
>> Same as above comments.
>
> Ack
>
>>>
>>>> + dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>> +        dp_display_host_phy_init(dp);
>>>> +    }
>>>>         return 0;
>>>>   }
>>>>     static const struct dev_pm_ops dp_pm_ops = {
>>>> -    .suspend = dp_pm_suspend,
>>>> -    .resume =  dp_pm_resume,
>>>> +    SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, 
>>>> dp_pm_runtime_resume, NULL)
>>>> +    SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>> +                                pm_runtime_force_resume)
>>>>   };
>>>>     static struct platform_driver dp_display_driver = {
>>>> @@ -1478,10 +1412,6 @@ static int dp_display_get_next_bridge(struct 
>>>> msm_dp *dp)
>>>>       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>>>>         if (aux_bus && dp->is_edp) {
>>>> -        dp_display_host_init(dp_priv);
>>>> -        dp_catalog_ctrl_hpd_enable(dp_priv->catalog);
>>>> -        dp_display_host_phy_init(dp_priv);
>>>> -
>>>>           /*
>>>>            * The code below assumes that the panel will finish probing
>>>>            * by the time devm_of_dp_aux_populate_ep_devices() returns.
>>>> @@ -1583,6 +1513,11 @@ void dp_bridge_atomic_enable(struct 
>>>> drm_bridge *drm_bridge,
>>>>           dp_hpd_plug_handle(dp_display, 0);
>>>>         mutex_lock(&dp_display->event_mutex);
>>>> +    if (pm_runtime_resume_and_get(&dp_display->pdev->dev)) {
>>>> +        DRM_ERROR("failed to pm_runtime_resume\n");
>>>> +        mutex_unlock(&dp_display->event_mutex);
>>>> +        return;
>>>> +    }
>>>>         state = dp_display->hpd_state;
>>>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>>>> @@ -1647,10 +1582,9 @@ void dp_bridge_atomic_post_disable(struct 
>>>> drm_bridge *drm_bridge,
>>>>       mutex_lock(&dp_display->event_mutex);
>>>>         state = dp_display->hpd_state;
>>>> -    if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED) {
>>>> -        mutex_unlock(&dp_display->event_mutex);
>>>> -        return;
>>>> -    }
>>>> +    if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED)
>>>> +        drm_dbg_dp(dp->drm_dev, "type=%d wrong hpd_state=%d\n",
>>>> +               dp->connector_type, state);
>>>>         dp_display_disable(dp_display);
>>>>   @@ -1663,6 +1597,8 @@ void dp_bridge_atomic_post_disable(struct 
>>>> drm_bridge *drm_bridge,
>>>>       }
>>>>         drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
>>>> +
>>>> +    pm_runtime_put_sync(&dp_display->pdev->dev);
>>>>       mutex_unlock(&dp_display->event_mutex);
>>>>   }
>>>>   @@ -1702,6 +1638,12 @@ void dp_bridge_hpd_enable(struct 
>>>> drm_bridge *bridge)
>>>>       struct dp_display_private *dp = container_of(dp_display, 
>>>> struct dp_display_private, dp_display);
>>>>         mutex_lock(&dp->event_mutex);
>>>> +    if (pm_runtime_resume_and_get(&dp->pdev->dev)) {
>>>> +        DRM_ERROR("failed to start power\n");
>>>> +        mutex_unlock(&dp->event_mutex);
>>>> +        return;
>>>> +    }
>>>> +
>>>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>>         /* enable HDP interrupts */
>>>> @@ -1723,6 +1665,9 @@ 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;
>>>
>>> As I wrote in the comment to the previous series, this is incorrect. 
>>> The .hpd_disable() doesn't mean that the display gets disconnected. 
>>> Neither does .hpd_enable().
>> ok,
>>>
>>>> +
>>>> +    pm_runtime_put_sync(&dp->pdev->dev);
>>>>       mutex_unlock(&dp->event_mutex);
>>>>   }
>>>>   @@ -1737,13 +1682,10 @@ void dp_bridge_hpd_notify(struct 
>>>> drm_bridge *bridge,
>>>>       if (dp_display->internal_hpd)
>>>>           return;
>>>>   -    if (!dp->core_initialized) {
>>>> -        drm_dbg_dp(dp->drm_dev, "not initialized\n");
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    if (!dp_display->link_ready && status == 
>>>> connector_status_connected)
>>>> +    if (!dp_display->link_ready && status == 
>>>> connector_status_connected) {
>>>> +        dp->hpd_state = ST_DISCONNECTED;
>>>>           dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
>>>> -    else if (dp_display->link_ready && status == 
>>>> connector_status_disconnected)
>>>> +    } else if (dp_display->link_ready && status == 
>>>> connector_status_disconnected) {
>>>>           dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
>>>> +    }
>>>>   }
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_power.c
>>>> index 5cb84ca..863c766 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>>>> @@ -152,20 +152,9 @@ int dp_power_client_init(struct dp_power 
>>>> *dp_power)
>>>>         power = container_of(dp_power, struct dp_power_private, 
>>>> dp_power);
>>>>   -    pm_runtime_enable(power->dev);
>>>> -
>>>>       return dp_power_clk_init(power);
>>>>   }
>>>>   -void dp_power_client_deinit(struct dp_power *dp_power)
>>>> -{
>>>> -    struct dp_power_private *power;
>>>> -
>>>> -    power = container_of(dp_power, struct dp_power_private, 
>>>> dp_power);
>>>> -
>>>> -    pm_runtime_disable(power->dev);
>>>> -}
>>>> -
>>>>   int dp_power_init(struct dp_power *dp_power)
>>>>   {
>>>>       int rc = 0;
>>>> @@ -173,11 +162,7 @@ int dp_power_init(struct dp_power *dp_power)
>>>>         power = container_of(dp_power, struct dp_power_private, 
>>>> dp_power);
>>>>   -    pm_runtime_get_sync(power->dev);
>>>> -
>>>>       rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
>>>> -    if (rc)
>>>> -        pm_runtime_put_sync(power->dev);
>>>>         return rc;
>>>>   }
>>>> @@ -189,7 +174,6 @@ int dp_power_deinit(struct dp_power *dp_power)
>>>>       power = container_of(dp_power, struct dp_power_private, 
>>>> dp_power);
>>>>         dp_power_clk_enable(dp_power, DP_CORE_PM, false);
>>>> -    pm_runtime_put_sync(power->dev);
>>>>       return 0;
>>>>   }
>>>>   diff --git a/drivers/gpu/drm/msm/dp/dp_power.h 
>>>> b/drivers/gpu/drm/msm/dp/dp_power.h
>>>> index a3dec20..55ada51 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_power.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
>>>> @@ -81,17 +81,6 @@ int dp_power_clk_enable(struct dp_power *power, 
>>>> enum dp_pm_type pm_type,
>>>>   int dp_power_client_init(struct dp_power *power);
>>>>     /**
>>>> - * dp_power_clinet_deinit() - de-initialize clock and regulator 
>>>> modules
>>>> - *
>>>> - * @power: instance of power module
>>>> - * return: 0 for success, error for failure.
>>>> - *
>>>> - * This API will de-initialize the DisplayPort's clocks and regulator
>>>> - * modules.
>>>> - */
>>>> -void dp_power_client_deinit(struct dp_power *power);
>>>> -
>>>> -/**
>>>>    * dp_power_get() - configure and get the DisplayPort power 
>>>> module data
>>>>    *
>>>>    * @parser: instance of parser module
>>>
>
  

On Fri, 6 Oct 2023 at 23:44, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 10/6/2023 11:04 AM, Dmitry Baryshkov wrote:
> > On 06/10/2023 19:42, Kuogee Hsieh wrote:
> >>
> >> On 10/6/2023 4:56 AM, Dmitry Baryshkov wrote:
> >>> On 04/10/2023 19:26, Kuogee Hsieh wrote:
> >>>> Currently DP driver is executed independent of PM runtime framework.
> >>>> This leads msm eDP panel can not being detected by edp_panel driver
> >>>> during
> >>>> generic_edp_panel_probe() due to AUX DPCD read failed at edp panel
> >>>> driver.
> >>>> Incorporate PM runtime framework into DP driver so that host
> >>>> controller's
> >>>> power and clocks are enable/disable through PM runtime mechanism.
> >>>> Once PM runtime framework is incorporated into DP driver, waking up
> >>>> device
> >>>> from power up path is not necessary. Hence remove it.
> >>>>
> >>>> After incorporating pm_runtime framework into eDP/DP driver,
> >>>> 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. Therefore both dp_pm_suspend() and
> >>>> dp_pm_resume() are
> >>>> dropped and replace with dp_pm_runtime_suspend() and
> >>>> dp_pm_runtime_resume()
> >>>> respectively.
> >>>>
> >>>> Changes in v5:
> >>>> -- remove pm_runtime_put_autosuspend feature, use
> >>>> pm_runtime_put_sync() directly
> >>>> -- squash add pm_runtime_force_suspend()/resume() patch into this
> >>>> patch
> >>>
> >>> Ok, this is much better now. Thank you!
> >>>
> >>>>
> >>>> Changes in v4:
> >>>> -- reworded commit text to explain why pm_framework is required for
> >>>> edp panel
> >>>> -- reworded commit text to explain autosuspend is choiced
> >>>> -- delete EV_POWER_PM_GET and PM_EV_POWER_PUT from changes #3
> >>>> -- delete dp_display_pm_get() and dp_display_pm_Put() from changes #3
> >>>> -- return value from pm_runtime_resume_and_get() directly
> >>>> -- check return value of devm_pm_runtime_enable()
> >>>> -- delete pm_runtime_xxx from dp_display_remove()
> >>>> -- drop dp_display_host_init() from EV_HPD_INIT_SETUP
> >>>> -- 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:
> >>>> -- incorporate removing pm_runtime_xx() from dp_pwer.c to this patch
> >>>> -- use pm_runtime_resume_and_get() instead of pm_runtime_get()
> >>>> -- error checking pm_runtime_resume_and_get() return value
> >>>> -- add EV_POWER_PM_GET and PM_EV_POWER_PUT to handle HPD_GPIO case
> >>>> -- 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>
> >>>> Reported-by: kernel test robot <lkp@intel.com>
> >>>> ---
> >>>>   drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
> >>>>   drivers/gpu/drm/msm/dp/dp_display.c | 166
> >>>> ++++++++++++------------------------
> >>>>   drivers/gpu/drm/msm/dp/dp_power.c   |  16 ----
> >>>>   drivers/gpu/drm/msm/dp/dp_power.h   |  11 ---
> >>>>   4 files changed, 59 insertions(+), 139 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> index 8e3b677..10b6eeb 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> @@ -291,6 +291,10 @@ static ssize_t dp_aux_transfer(struct
> >>>> drm_dp_aux *dp_aux,
> >>>>           return -EINVAL;
> >>>>       }
> >>>>   +    ret = pm_runtime_resume_and_get(dp_aux->dev);
> >>>> +    if (ret)
> >>>> +        return  ret;
> >>>> +
> >>>>       mutex_lock(&aux->mutex);
> >>>>       if (!aux->initted) {
> >>>>           ret = -EIO;
> >>>> @@ -364,6 +368,7 @@ static ssize_t dp_aux_transfer(struct
> >>>> drm_dp_aux *dp_aux,
> >>>>     exit:
> >>>>       mutex_unlock(&aux->mutex);
> >>>> +    pm_runtime_put_sync(dp_aux->dev);
> >>>>         return ret;
> >>>>   }
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index e4942fc..42efe148b 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 {
> >>>> @@ -310,15 +309,10 @@ static void dp_display_unbind(struct device
> >>>> *dev, struct device *master,
> >>>>       struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >>>>       struct msm_drm_private *priv = dev_get_drvdata(master);
> >>>>   -    /* disable all HPD interrupts */
> >>>> -    if (dp->core_initialized)
> >>>> -        dp_catalog_hpd_config_intr(dp->catalog,
> >>>> DP_DP_HPD_INT_MASK, false);
> >>>> -
> >>>>       kthread_stop(dp->ev_tsk);
> >>>>         of_dp_aux_depopulate_bus(dp->aux);
> >>>>   -    dp_power_client_deinit(dp->power);
> >>>>       dp_unregister_audio_driver(dev, dp->audio);
> >>>>       dp_aux_unregister(dp->aux);
> >>>>       dp->drm_dev = NULL;
> >>>> @@ -559,7 +553,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;
> >>>>       }
> >>>> @@ -576,6 +570,14 @@ static int dp_hpd_plug_handle(struct
> >>>> dp_display_private *dp, u32 data)
> >>>>           return 0;
> >>>>       }
> >>>>   +    if (!dp->dp_display.is_edp) {
> >>>
> >>> Why?
> >>
> >> This is mainly for hpd_internal == false case (any possible way of
> >> delivering an HPD event)
> >>
> >> for both eDP and "hpd_internal == true" cases,
> >> pm_runtime_resume_and_get() had been called already.
> >>
> >> will it better? if it was  removed the condition
> >> (dp->dp_display.is_edp) check?
> >
> > Yes, this codepath seems generic.
> >
> >>
> >>>
> >>>> +        ret = pm_runtime_resume_and_get(&dp->pdev->dev);
> >>>> +        if (ret) {
> >>>> +            DRM_ERROR("failed to pm_runtime_resume\n");
> >>>> +            return ret;
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>>       ret = dp_display_usbpd_configure_cb(&dp->pdev->dev);
> >>>>       if (ret) {    /* link train failed */
> >>>>           dp->hpd_state = ST_DISCONNECTED;
> >>>> @@ -658,6 +660,8 @@ static int dp_hpd_unplug_handle(struct
> >>>> dp_display_private *dp, u32 data)
> >>>>               dp->dp_display.connector_type, state);
> >>>>         /* uevent will complete disconnection part */
> >>>> +    if (!dp->dp_display.is_edp)
> >>>> +        pm_runtime_put_sync(&dp->pdev->dev);
> >>>>       mutex_unlock(&dp->event_mutex);
> >>>>       return 0;
> >>>>   }
> >>>> @@ -673,7 +677,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;
> >>>>       }
> >>>> @@ -1086,7 +1090,6 @@ static int hpd_event_thread(void *data)
> >>>>             switch (todo->event_id) {
> >>>>           case EV_HPD_INIT_SETUP:
> >>>> -            dp_display_host_init(dp_priv);
> >>>>               break;
> >>>>           case EV_HPD_PLUG_INT:
> >>>>               dp_hpd_plug_handle(dp_priv, todo->data);
> >>>> @@ -1264,6 +1267,10 @@ static int dp_display_probe(struct
> >>>> platform_device *pdev)
> >>>>         platform_set_drvdata(pdev, &dp->dp_display);
> >>>>   +    rc = devm_pm_runtime_enable(&pdev->dev);
> >>>> +    if (rc)
> >>>> +        goto err;
> >>>> +
> >>>>       rc = dp_display_request_irq(dp);
> >>>
> >>> Here we have a problem now. The device can be runtime suspended,
> >>> ceasing register access. However the IRQ enablement status is no
> >>> longer synchronised to the DP's runtime status. Thus, if for some
> >>> reason the IRQ handler is triggered, we can get unclocked register
> >>> access / sync error.
> >>>
> >>> Please correct me if I'm wrong, but I think that the request_irq
> >>> should also get the IRQF_NO_AUTOEN flag and the suspend / resume
> >>> paths should also have enable_irq() / disable_irq() calls.
> >>>
> >> got it.
> >>
> >>
> >>>>       if (rc)
> >>>>           goto err;
> >>>> @@ -1293,109 +1300,36 @@ static int dp_display_remove(struct
> >>>> platform_device *pdev)
> >>>>       return 0;
> >>>>   }
> >>>>   -static int dp_pm_resume(struct device *dev)
> >>>> +static int dp_pm_runtime_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;
> >>>> -    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;
> >>>> +    struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >>>>   +    if (dp->dp_display.is_edp) {
> >>>
> >>> Why is it only for eDP case?
> >>>
> >>> My expectation is that if the DP device is suspended, then the PHY
> >>> and the host parts are disabled. The driver, panel-edp / DP_AUX and
> >>> the DRM framework should take care of resuming the DP device at the
> >>> right time.
> >>>
> >> eDP and DP are implemented differently.
> >>
> >> for external DP case,
> >>
> >> 1) dp_pm_runtime_resume() only enable dp host.
> >>
> >> 2) at dp_bridge_hpd_enable(), enable hdp block only and have hpd irq
> >> enabled and start waiting for plugin interrupt.  but leave phy is not
> >> initialized.
> >>
> >> 3) DP phy is initialized at plugin interrupt handle at the time right
> >> before link training.
> >>
> >> for eDP case, hpd irq is always disable (no hpd irq required for eDP).
> >>
> >> 1) DP host, HDP block and phy are enabled (initialized) at same time
> >> at dp_pm_runtime_resume()
> >
> > Ack. thanks. If you can add this to the comment somewhere in the file,
> > that will be great. Also just for my understanding: The function
> > dp_catalog_ctrl_hpd_enable() only enables handling of the HPD pin, it
> > has no other side effects? So for the DP case if we have external HPD
> > events, we don't need to call it, correct?
>
> yes, this is correct.  dp_catalog_ctrl_hpd_enable() and
> dp_catalog_hpd_config_intr() are only for the case depend on hpd interrupts.

Thanks for the confirmation. Could you please add these details to a comment?

>
> >
> > Also, could you please add a followup patch that drop
> > dp_display->internal_hpd and always uses the same codepath for both
> > cases? Would that be a logical change after this series?
> ok, i will submit a patch to remove hpd_internal.

Thank you!

> >
> >
> >>            dp_display_host_phy_exit(dp);
> >>>> + dp_catalog_ctrl_hpd_disable(dp->catalog);
> >>>>       }
> >>>> -
> >>>> -    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);
> >>>> +    dp_display_host_deinit(dp);
> >>>>         return 0;
> >>>>   }
> >>>>   -static int dp_pm_suspend(struct device *dev)
> >>>> +static int dp_pm_runtime_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;
> >>>> -
> >>>> -    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);
> >>>> +    struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >>>>   -    mutex_unlock(&dp->event_mutex);
> >>>> +    dp_display_host_init(dp);
> >>>> +    if (dp->dp_display.is_edp) {
> >>>
> >>> Why?
> >> Same as above comments.
> >
> > Ack
> >
> >>>
> >>>> + dp_catalog_ctrl_hpd_enable(dp->catalog);
> >>>> +        dp_display_host_phy_init(dp);
> >>>> +    }
> >>>>         return 0;
> >>>>   }
> >>>>     static const struct dev_pm_ops dp_pm_ops = {
> >>>> -    .suspend = dp_pm_suspend,
> >>>> -    .resume =  dp_pm_resume,
> >>>> +    SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend,
> >>>> dp_pm_runtime_resume, NULL)
> >>>> +    SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >>>> +                                pm_runtime_force_resume)
> >>>>   };
> >>>>     static struct platform_driver dp_display_driver = {
> >>>> @@ -1478,10 +1412,6 @@ static int dp_display_get_next_bridge(struct
> >>>> msm_dp *dp)
> >>>>       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> >>>>         if (aux_bus && dp->is_edp) {
> >>>> -        dp_display_host_init(dp_priv);
> >>>> -        dp_catalog_ctrl_hpd_enable(dp_priv->catalog);
> >>>> -        dp_display_host_phy_init(dp_priv);
> >>>> -
> >>>>           /*
> >>>>            * The code below assumes that the panel will finish probing
> >>>>            * by the time devm_of_dp_aux_populate_ep_devices() returns.
> >>>> @@ -1583,6 +1513,11 @@ void dp_bridge_atomic_enable(struct
> >>>> drm_bridge *drm_bridge,
> >>>>           dp_hpd_plug_handle(dp_display, 0);
> >>>>         mutex_lock(&dp_display->event_mutex);
> >>>> +    if (pm_runtime_resume_and_get(&dp_display->pdev->dev)) {
> >>>> +        DRM_ERROR("failed to pm_runtime_resume\n");
> >>>> +        mutex_unlock(&dp_display->event_mutex);
> >>>> +        return;
> >>>> +    }
> >>>>         state = dp_display->hpd_state;
> >>>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
> >>>> @@ -1647,10 +1582,9 @@ void dp_bridge_atomic_post_disable(struct
> >>>> drm_bridge *drm_bridge,
> >>>>       mutex_lock(&dp_display->event_mutex);
> >>>>         state = dp_display->hpd_state;
> >>>> -    if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED) {
> >>>> -        mutex_unlock(&dp_display->event_mutex);
> >>>> -        return;
> >>>> -    }
> >>>> +    if (state != ST_DISCONNECT_PENDING && state != ST_CONNECTED)
> >>>> +        drm_dbg_dp(dp->drm_dev, "type=%d wrong hpd_state=%d\n",
> >>>> +               dp->connector_type, state);
> >>>>         dp_display_disable(dp_display);
> >>>>   @@ -1663,6 +1597,8 @@ void dp_bridge_atomic_post_disable(struct
> >>>> drm_bridge *drm_bridge,
> >>>>       }
> >>>>         drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
> >>>> +
> >>>> +    pm_runtime_put_sync(&dp_display->pdev->dev);
> >>>>       mutex_unlock(&dp_display->event_mutex);
> >>>>   }
> >>>>   @@ -1702,6 +1638,12 @@ void dp_bridge_hpd_enable(struct
> >>>> drm_bridge *bridge)
> >>>>       struct dp_display_private *dp = container_of(dp_display,
> >>>> struct dp_display_private, dp_display);
> >>>>         mutex_lock(&dp->event_mutex);
> >>>> +    if (pm_runtime_resume_and_get(&dp->pdev->dev)) {
> >>>> +        DRM_ERROR("failed to start power\n");
> >>>> +        mutex_unlock(&dp->event_mutex);
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
> >>>>         /* enable HDP interrupts */
> >>>> @@ -1723,6 +1665,9 @@ 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;
> >>>
> >>> As I wrote in the comment to the previous series, this is incorrect.
> >>> The .hpd_disable() doesn't mean that the display gets disconnected.
> >>> Neither does .hpd_enable().
> >> ok,
> >>>
> >>>> +
> >>>> +    pm_runtime_put_sync(&dp->pdev->dev);
> >>>>       mutex_unlock(&dp->event_mutex);
> >>>>   }
> >>>>   @@ -1737,13 +1682,10 @@ void dp_bridge_hpd_notify(struct
> >>>> drm_bridge *bridge,
> >>>>       if (dp_display->internal_hpd)
> >>>>           return;
> >>>>   -    if (!dp->core_initialized) {
> >>>> -        drm_dbg_dp(dp->drm_dev, "not initialized\n");
> >>>> -        return;
> >>>> -    }
> >>>> -
> >>>> -    if (!dp_display->link_ready && status ==
> >>>> connector_status_connected)
> >>>> +    if (!dp_display->link_ready && status ==
> >>>> connector_status_connected) {
> >>>> +        dp->hpd_state = ST_DISCONNECTED;
> >>>>           dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> >>>> -    else if (dp_display->link_ready && status ==
> >>>> connector_status_disconnected)
> >>>> +    } else if (dp_display->link_ready && status ==
> >>>> connector_status_disconnected) {
> >>>>           dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> >>>> +    }
> >>>>   }
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_power.c
> >>>> index 5cb84ca..863c766 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> >>>> @@ -152,20 +152,9 @@ int dp_power_client_init(struct dp_power
> >>>> *dp_power)
> >>>>         power = container_of(dp_power, struct dp_power_private,
> >>>> dp_power);
> >>>>   -    pm_runtime_enable(power->dev);
> >>>> -
> >>>>       return dp_power_clk_init(power);
> >>>>   }
> >>>>   -void dp_power_client_deinit(struct dp_power *dp_power)
> >>>> -{
> >>>> -    struct dp_power_private *power;
> >>>> -
> >>>> -    power = container_of(dp_power, struct dp_power_private,
> >>>> dp_power);
> >>>> -
> >>>> -    pm_runtime_disable(power->dev);
> >>>> -}
> >>>> -
> >>>>   int dp_power_init(struct dp_power *dp_power)
> >>>>   {
> >>>>       int rc = 0;
> >>>> @@ -173,11 +162,7 @@ int dp_power_init(struct dp_power *dp_power)
> >>>>         power = container_of(dp_power, struct dp_power_private,
> >>>> dp_power);
> >>>>   -    pm_runtime_get_sync(power->dev);
> >>>> -
> >>>>       rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> >>>> -    if (rc)
> >>>> -        pm_runtime_put_sync(power->dev);
> >>>>         return rc;
> >>>>   }
> >>>> @@ -189,7 +174,6 @@ int dp_power_deinit(struct dp_power *dp_power)
> >>>>       power = container_of(dp_power, struct dp_power_private,
> >>>> dp_power);
> >>>>         dp_power_clk_enable(dp_power, DP_CORE_PM, false);
> >>>> -    pm_runtime_put_sync(power->dev);
> >>>>       return 0;
> >>>>   }
> >>>>   diff --git a/drivers/gpu/drm/msm/dp/dp_power.h
> >>>> b/drivers/gpu/drm/msm/dp/dp_power.h
> >>>> index a3dec20..55ada51 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_power.h
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
> >>>> @@ -81,17 +81,6 @@ int dp_power_clk_enable(struct dp_power *power,
> >>>> enum dp_pm_type pm_type,
> >>>>   int dp_power_client_init(struct dp_power *power);
> >>>>     /**
> >>>> - * dp_power_clinet_deinit() - de-initialize clock and regulator
> >>>> modules
> >>>> - *
> >>>> - * @power: instance of power module
> >>>> - * return: 0 for success, error for failure.
> >>>> - *
> >>>> - * This API will de-initialize the DisplayPort's clocks and regulator
> >>>> - * modules.
> >>>> - */
> >>>> -void dp_power_client_deinit(struct dp_power *power);
> >>>> -
> >>>> -/**
> >>>>    * dp_power_get() - configure and get the DisplayPort power
> >>>> module data
> >>>>    *
> >>>>    * @parser: instance of parser module
> >>>
> >