[PATCH v5 0/7] incorporate pm runtime framework and eDP clean up

[PATCH v5 0/7] incorporate pm runtime framework and eDP clean up

[Kuogee Hsieh]

The purpose of this patch series is to incorporate pm runtime framework
into MSM eDP/DP driver so that eDP panel can be detected by DRM eDP panel
driver during system probe time. During incorporating procedure, original
customized pm realted fucntions, such as dp_pm_prepare(), dp_pm_suspend(),
dp_pm_resume() and dp_pm_prepare(), are removed and replaced with functions
provided by pm runtiem framework such as pm_runtime_force_suspend() and
pm_runtime_force_resume(). In addition, both eDP aux-bus and irq handler
are bound at system probe time too.

Kuogee Hsieh (7):
  drm/msm/dp: tie dp_display_irq_handler() with dp driver
  drm/msm/dp: rename is_connected with link_ready
  drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes
  drm/msm/dp: move parser->parse() and dp_power_client_init() to probe
  drm/msm/dp: incorporate pm_runtime framework into DP driver
  drm/msm/dp: delete EV_HPD_INIT_SETUP
  drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |   4 -
 drivers/gpu/drm/msm/dp/dp_aux.c         |  39 +++-
 drivers/gpu/drm/msm/dp/dp_display.c     | 318 +++++++++++---------------------
 drivers/gpu/drm/msm/dp/dp_display.h     |   3 +-
 drivers/gpu/drm/msm/dp/dp_drm.c         |  14 +-
 drivers/gpu/drm/msm/dp/dp_power.c       |  16 --
 drivers/gpu/drm/msm/dp/dp_power.h       |  11 --
 drivers/gpu/drm/msm/msm_drv.h           |   5 -
 8 files changed, 146 insertions(+), 264 deletions(-)

-- 
2.7.4

[PATCH v5 1/7] drm/msm/dp: tie dp_display_irq_handler() with dp driver

[Kuogee Hsieh]

Currently the dp_display_request_irq() is executed at msm_dp_modeset_init()
which ties irq registering to the DPU device's life cycle, while depending on
resources that are released as the DP device is torn down. Move register DP
driver irq handler to dp_display_probe() to have dp_display_irq_handler()
IRQ tied with DP device. In addition, use platform_get_irq() to retrieve irq
number from platform device directly.

Changes in v5:
-- reworded commit text as review comments at change #4
-- tear down component if failed at dp_display_request_irq()

Changes in v4:
-- delete dp->irq check at dp_display_request_irq()

Changes in v3:
-- move calling dp_display_irq_handler() to probe

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 32 +++++++++++++-------------------
 drivers/gpu/drm/msm/dp/dp_display.h |  1 -
 2 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 76f1395..940da48 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1193,26 +1193,18 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
 	return ret;
 }
 
-int dp_display_request_irq(struct msm_dp *dp_display)
+static int dp_display_request_irq(struct dp_display_private *dp)
 {
 	int rc = 0;
-	struct dp_display_private *dp;
-
-	if (!dp_display) {
-		DRM_ERROR("invalid input\n");
-		return -EINVAL;
-	}
-
-	dp = container_of(dp_display, struct dp_display_private, dp_display);
+	struct device *dev = &dp->pdev->dev;
 
-	dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
+	dp->irq = platform_get_irq(dp->pdev, 0);
 	if (!dp->irq) {
 		DRM_ERROR("failed to get irq\n");
 		return -EINVAL;
 	}
 
-	rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
-			dp_display_irq_handler,
+	rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
 			IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
 	if (rc < 0) {
 		DRM_ERROR("failed to request IRQ%u: %d\n",
@@ -1287,13 +1279,21 @@ static int dp_display_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dp->dp_display);
 
+	rc = dp_display_request_irq(dp);
+	if (rc)
+		goto err;
+
 	rc = component_add(&pdev->dev, &dp_display_comp_ops);
 	if (rc) {
 		DRM_ERROR("component add failed, rc=%d\n", rc);
-		dp_display_deinit_sub_modules(dp);
+		goto err;
 	}
 
 	return rc;
+
+err:
+	dp_display_deinit_sub_modules(dp);
+	return rc;
 }
 
 static int dp_display_remove(struct platform_device *pdev)
@@ -1549,12 +1549,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 
 	dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
 
-	ret = dp_display_request_irq(dp_display);
-	if (ret) {
-		DRM_ERROR("request_irq failed, ret=%d\n", ret);
-		return ret;
-	}
-
 	ret = dp_display_get_next_bridge(dp_display);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 1e9415a..b3c08de 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -35,7 +35,6 @@ struct msm_dp {
 int dp_display_set_plugged_cb(struct msm_dp *dp_display,
 		hdmi_codec_plugged_cb fn, struct device *codec_dev);
 int dp_display_get_modes(struct msm_dp *dp_display);
-int dp_display_request_irq(struct msm_dp *dp_display);
 bool dp_display_check_video_test(struct msm_dp *dp_display);
 int dp_display_get_test_bpp(struct msm_dp *dp_display);
 void dp_display_signal_audio_start(struct msm_dp *dp_display);
-- 
2.7.4

Re: [PATCH v5 1/7] drm/msm/dp: tie dp_display_irq_handler() with dp driver

[Dmitry Baryshkov]

On 04/10/2023 19:26, Kuogee Hsieh wrote:
> Currently the dp_display_request_irq() is executed at msm_dp_modeset_init()
> which ties irq registering to the DPU device's life cycle, while depending on
> resources that are released as the DP device is torn down. Move register DP
> driver irq handler to dp_display_probe() to have dp_display_irq_handler()
> IRQ tied with DP device. In addition, use platform_get_irq() to retrieve irq
> number from platform device directly.
> 
> Changes in v5:
> -- reworded commit text as review comments at change #4
> -- tear down component if failed at dp_display_request_irq()
> 
> Changes in v4:
> -- delete dp->irq check at dp_display_request_irq()
> 
> Changes in v3:
> -- move calling dp_display_irq_handler() to probe
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 32 +++++++++++++-------------------
>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>   2 files changed, 13 insertions(+), 20 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

[PATCH v5 2/7] drm/msm/dp: rename is_connected with link_ready

[Kuogee Hsieh]

The is_connected flag is set to true after DP mainlink successfully
finishes link training to enter into ST_MAINLINK_READY state rather
than being set after the DP dongle is connected. Rename the
is_connected flag with link_ready flag to match the state of DP
driver's state machine.

Changes in v5:
-- reworded commit text according to review comments from change #4

Changes in v4:
-- reworded commit text

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 19 +++++++++----------
 drivers/gpu/drm/msm/dp/dp_display.h |  2 +-
 drivers/gpu/drm/msm/dp/dp_drm.c     | 14 +++++++-------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 940da48..9e51876 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -367,12 +367,11 @@ static void dp_display_send_hpd_event(struct msm_dp *dp_display)
 	drm_helper_hpd_irq_event(connector->dev);
 }
 
-
 static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 					    bool hpd)
 {
-	if ((hpd && dp->dp_display.is_connected) ||
-			(!hpd && !dp->dp_display.is_connected)) {
+	if ((hpd && dp->dp_display.link_ready) ||
+			(!hpd && !dp->dp_display.link_ready)) {
 		drm_dbg_dp(dp->drm_dev, "HPD already %s\n",
 				(hpd ? "on" : "off"));
 		return 0;
@@ -382,7 +381,7 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 	if (!hpd)
 		dp->panel->video_test = false;
 
-	dp->dp_display.is_connected = hpd;
+	dp->dp_display.link_ready = hpd;
 
 	drm_dbg_dp(dp->drm_dev, "type=%d hpd=%d\n",
 			dp->dp_display.connector_type, hpd);
@@ -922,7 +921,7 @@ int dp_display_set_plugged_cb(struct msm_dp *dp_display,
 
 	dp_display->plugged_cb = fn;
 	dp_display->codec_dev = codec_dev;
-	plugged = dp_display->is_connected;
+	plugged = dp_display->link_ready;
 	dp_display_handle_plugged_change(dp_display, plugged);
 
 	return 0;
@@ -1355,16 +1354,16 @@ static int dp_pm_resume(struct device *dev)
 	 * also only signal audio when disconnected
 	 */
 	if (dp->link->sink_count) {
-		dp->dp_display.is_connected = true;
+		dp->dp_display.link_ready = true;
 	} else {
-		dp->dp_display.is_connected = false;
+		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.is_connected, dp->core_initialized,
+		dp->dp_display.link_ready, dp->core_initialized,
 		dp->phy_initialized, dp_display->power_on);
 
 	mutex_unlock(&dp->event_mutex);
@@ -1757,8 +1756,8 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
 		return;
 	}
 
-	if (!dp_display->is_connected && status == connector_status_connected)
+	if (!dp_display->link_ready && status == connector_status_connected)
 		dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
-	else if (dp_display->is_connected && 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_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index b3c08de..d65693e 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -16,7 +16,7 @@ struct msm_dp {
 	struct drm_bridge *bridge;
 	struct drm_connector *connector;
 	struct drm_bridge *next_bridge;
-	bool is_connected;
+	bool link_ready;
 	bool audio_enabled;
 	bool power_on;
 	unsigned int connector_type;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 785d766..ee945ca 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -24,10 +24,10 @@ static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge)
 
 	dp = to_dp_bridge(bridge)->dp_display;
 
-	drm_dbg_dp(dp->drm_dev, "is_connected = %s\n",
-		(dp->is_connected) ? "true" : "false");
+	drm_dbg_dp(dp->drm_dev, "link_ready = %s\n",
+		(dp->link_ready) ? "true" : "false");
 
-	return (dp->is_connected) ? connector_status_connected :
+	return (dp->link_ready) ? connector_status_connected :
 					connector_status_disconnected;
 }
 
@@ -40,8 +40,8 @@ static int dp_bridge_atomic_check(struct drm_bridge *bridge,
 
 	dp = to_dp_bridge(bridge)->dp_display;
 
-	drm_dbg_dp(dp->drm_dev, "is_connected = %s\n",
-		(dp->is_connected) ? "true" : "false");
+	drm_dbg_dp(dp->drm_dev, "link_ready = %s\n",
+		(dp->link_ready) ? "true" : "false");
 
 	/*
 	 * There is no protection in the DRM framework to check if the display
@@ -55,7 +55,7 @@ static int dp_bridge_atomic_check(struct drm_bridge *bridge,
 	 * After that this piece of code can be removed.
 	 */
 	if (bridge->ops & DRM_BRIDGE_OP_HPD)
-		return (dp->is_connected) ? 0 : -ENOTCONN;
+		return (dp->link_ready) ? 0 : -ENOTCONN;
 
 	return 0;
 }
@@ -78,7 +78,7 @@ static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *
 	dp = to_dp_bridge(bridge)->dp_display;
 
 	/* pluggable case assumes EDID is read when HPD */
-	if (dp->is_connected) {
+	if (dp->link_ready) {
 		rc = dp_display_get_modes(dp);
 		if (rc <= 0) {
 			DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
-- 
2.7.4

Re: [PATCH v5 2/7] drm/msm/dp: rename is_connected with link_ready

[Dmitry Baryshkov]

On 04/10/2023 19:26, Kuogee Hsieh wrote:
> The is_connected flag is set to true after DP mainlink successfully
> finishes link training to enter into ST_MAINLINK_READY state rather
> than being set after the DP dongle is connected. Rename the
> is_connected flag with link_ready flag to match the state of DP
> driver's state machine.
> 
> Changes in v5:
> -- reworded commit text according to review comments from change #4
> 
> Changes in v4:
> -- reworded commit text
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 19 +++++++++----------
>   drivers/gpu/drm/msm/dp/dp_display.h |  2 +-
>   drivers/gpu/drm/msm/dp/dp_drm.c     | 14 +++++++-------
>   3 files changed, 17 insertions(+), 18 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

[PATCH v5 3/7] drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes

[Kuogee Hsieh]

Currently DP driver use drm_helper_hpd_irq_event(), bypassing drm bridge
framework, to report HPD status changes to user space frame work.
Replace it with drm_bridge_hpd_notify() since DP driver is part of drm
bridge.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 9e51876..32663ea 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -356,26 +356,10 @@ static bool dp_display_is_sink_count_zero(struct dp_display_private *dp)
 		(dp->link->sink_count == 0);
 }
 
-static void dp_display_send_hpd_event(struct msm_dp *dp_display)
-{
-	struct dp_display_private *dp;
-	struct drm_connector *connector;
-
-	dp = container_of(dp_display, struct dp_display_private, dp_display);
-
-	connector = dp->dp_display.connector;
-	drm_helper_hpd_irq_event(connector->dev);
-}
-
 static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 					    bool hpd)
 {
-	if ((hpd && dp->dp_display.link_ready) ||
-			(!hpd && !dp->dp_display.link_ready)) {
-		drm_dbg_dp(dp->drm_dev, "HPD already %s\n",
-				(hpd ? "on" : "off"));
-		return 0;
-	}
+	struct drm_bridge *bridge = dp->dp_display.bridge;
 
 	/* reset video pattern flag on disconnect */
 	if (!hpd)
@@ -385,7 +369,7 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 
 	drm_dbg_dp(dp->drm_dev, "type=%d hpd=%d\n",
 			dp->dp_display.connector_type, hpd);
-	dp_display_send_hpd_event(&dp->dp_display);
+	drm_bridge_hpd_notify(bridge, dp->dp_display.link_ready);
 
 	return 0;
 }
-- 
2.7.4

[PATCH v5 4/7] drm/msm/dp: move parser->parse() and dp_power_client_init() to probe

[Kuogee Hsieh]

Original both parser->parse() and dp_power_client_init() are done at
dp_display_bind() since eDP population is done at binding time.
In the preparation of having eDP population done at probe() time,
move both function from dp_display_bind() to dp_display_probe().

Changes in v4:
-- explain why parser->parse() and dp_power_client_init() are moved to probe time
-- tear down sub modules if failed

Changes in v4:
-- split this patch out of "incorporate pm_runtime framework into DP driver" patch

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 32663ea..e4942fc 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -276,11 +276,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	dp->dp_display.drm_dev = drm;
 	priv->dp[dp->id] = &dp->dp_display;
 
-	rc = dp->parser->parse(dp->parser);
-	if (rc) {
-		DRM_ERROR("device tree parsing failed\n");
-		goto end;
-	}
 
 
 	dp->drm_dev = drm;
@@ -291,11 +286,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
 		goto end;
 	}
 
-	rc = dp_power_client_init(dp->power);
-	if (rc) {
-		DRM_ERROR("Power client create failed\n");
-		goto end;
-	}
 
 	rc = dp_register_audio_driver(dev, dp->audio);
 	if (rc) {
@@ -1250,6 +1240,18 @@ static int dp_display_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
+	rc = dp->parser->parse(dp->parser);
+	if (rc) {
+		DRM_ERROR("device tree parsing failed\n");
+		goto err;
+	}
+
+	rc = dp_power_client_init(dp->power);
+	if (rc) {
+		DRM_ERROR("Power client create failed\n");
+		goto err;
+	}
+
 	/* setup event q */
 	mutex_init(&dp->event_mutex);
 	init_waitqueue_head(&dp->event_q);
-- 
2.7.4

Re: [PATCH v5 4/7] drm/msm/dp: move parser->parse() and dp_power_client_init() to probe

[Dmitry Baryshkov]

On 04/10/2023 19:26, Kuogee Hsieh wrote:
> Original both parser->parse() and dp_power_client_init() are done at
> dp_display_bind() since eDP population is done at binding time.
> In the preparation of having eDP population done at probe() time,
> move both function from dp_display_bind() to dp_display_probe().
> 
> Changes in v4:
> -- explain why parser->parse() and dp_power_client_init() are moved to probe time
> -- tear down sub modules if failed
> 
> Changes in v4:
> -- split this patch out of "incorporate pm_runtime framework into DP driver" patch
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 32663ea..e4942fc 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -276,11 +276,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>   	dp->dp_display.drm_dev = drm;
>   	priv->dp[dp->id] = &dp->dp_display;
>   
> -	rc = dp->parser->parse(dp->parser);
> -	if (rc) {
> -		DRM_ERROR("device tree parsing failed\n");
> -		goto end;
> -	}
>   
>   
>   	dp->drm_dev = drm;
> @@ -291,11 +286,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>   		goto end;
>   	}
>   
> -	rc = dp_power_client_init(dp->power);

As we have moved dp_power_client_init() from bind() to probe(), we 
should also move dp_power_client_deinit() to remove(). Otherwise after 
several bind/unbind attempts the pm enablement count can become negative.

> -	if (rc) {
> -		DRM_ERROR("Power client create failed\n");
> -		goto end;
> -	}
>   
>   	rc = dp_register_audio_driver(dev, dp->audio);
>   	if (rc) {
> @@ -1250,6 +1240,18 @@ static int dp_display_probe(struct platform_device *pdev)
>   		return -EPROBE_DEFER;
>   	}
>   
> +	rc = dp->parser->parse(dp->parser);
> +	if (rc) {
> +		DRM_ERROR("device tree parsing failed\n");
> +		goto err;
> +	}
> +
> +	rc = dp_power_client_init(dp->power);
> +	if (rc) {
> +		DRM_ERROR("Power client create failed\n");
> +		goto err;
> +	}
> +
>   	/* setup event q */
>   	mutex_init(&dp->event_mutex);
>   	init_waitqueue_head(&dp->event_q);

-- 
With best wishes
Dmitry

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

[Kuogee Hsieh]

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

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) {
+		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);
 	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) {
 		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) {
+		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;
+
+	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
-- 
2.7.4

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Kuogee Hsieh]

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
>

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Kuogee Hsieh]

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

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

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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;
> +       }
> +

-- 
With best wishes
Dmitry

[PATCH v5 6/7] drm/msm/dp: delete EV_HPD_INIT_SETUP

[Kuogee Hsieh]

EV_HPD_INIT_SETUP flag is used to trigger the initialization of external
DP host controller. Since external DP host controller initialization had
been incorporated into pm_runtime_resume(), this flag became obsolete.
msm_dp_irq_postinstall() which triggers EV_HPD_INIT_SETUP event is
obsoleted accordingly.

Changes in v4:
-- reworded commit text
-- drop EV_HPD_INIT_SETUP
-- drop msm_dp_irq_postinstall()

Changes in v3:
-- drop EV_HPD_INIT_SETUP and msm_dp_irq_postinstall()

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |  4 ----
 drivers/gpu/drm/msm/dp/dp_display.c     | 16 ----------------
 drivers/gpu/drm/msm/msm_drv.h           |  5 -----
 3 files changed, 25 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa6ba2c..146f263 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -869,7 +869,6 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
 {
 	struct msm_drm_private *priv;
 	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
-	int i;
 
 	if (!dpu_kms || !dpu_kms->dev)
 		return -EINVAL;
@@ -878,9 +877,6 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
 	if (!priv)
 		return -EINVAL;
 
-	for (i = 0; i < ARRAY_SIZE(priv->dp); i++)
-		msm_dp_irq_postinstall(priv->dp[i]);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 42efe148b..79e56d9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -54,7 +54,6 @@ enum {
 enum {
 	EV_NO_EVENT,
 	/* hpd events */
-	EV_HPD_INIT_SETUP,
 	EV_HPD_PLUG_INT,
 	EV_IRQ_HPD_INT,
 	EV_HPD_UNPLUG_INT,
@@ -1089,8 +1088,6 @@ static int hpd_event_thread(void *data)
 		spin_unlock_irqrestore(&dp_priv->event_lock, flag);
 
 		switch (todo->event_id) {
-		case EV_HPD_INIT_SETUP:
-			break;
 		case EV_HPD_PLUG_INT:
 			dp_hpd_plug_handle(dp_priv, todo->data);
 			break;
@@ -1359,19 +1356,6 @@ void __exit msm_dp_unregister(void)
 	platform_driver_unregister(&dp_display_driver);
 }
 
-void msm_dp_irq_postinstall(struct msm_dp *dp_display)
-{
-	struct dp_display_private *dp;
-
-	if (!dp_display)
-		return;
-
-	dp = container_of(dp_display, struct dp_display_private, dp_display);
-
-	if (!dp_display->is_edp)
-		dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0);
-}
-
 bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
 {
 	struct dp_display_private *dp;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 02fd6c7..30723da 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -384,7 +384,6 @@ int __init msm_dp_register(void);
 void __exit msm_dp_unregister(void);
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 			 struct drm_encoder *encoder);
-void msm_dp_irq_postinstall(struct msm_dp *dp_display);
 void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
 
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
@@ -405,10 +404,6 @@ static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
 	return -EINVAL;
 }
 
-static inline void msm_dp_irq_postinstall(struct msm_dp *dp_display)
-{
-}
-
 static inline void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display)
 {
 }
-- 
2.7.4

[PATCH v5 7/7] drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()

[Kuogee Hsieh]

Currently eDP population is done at msm_dp_modeset_init() which happen
at binding time. Move eDP population to be done at display probe time
so that probe deferral cases can be handled effectively.
wait_for_hpd_asserted callback is added during drm_dp_aux_init()
to ensure eDP's HPD is up before proceeding eDP population.

Changes in v5:
-- inline dp_display_auxbus_population() and delete it

Changes in v4:
-- delete duplicate initialize code to dp_aux before drm_dp_aux_register()
-- delete of_get_child_by_name(dev->of_node, "aux-bus") and inline the function
-- not initialize rc = 0

Changes in v3:
-- add done_probing callback into devm_of_dp_aux_populate_bus()

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     | 34 ++++++++++++++++-----
 drivers/gpu/drm/msm/dp/dp_display.c | 59 +++++++++++++++----------------------
 2 files changed, 51 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 10b6eeb..03f4951 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -479,7 +479,6 @@ void dp_aux_deinit(struct drm_dp_aux *dp_aux)
 
 int dp_aux_register(struct drm_dp_aux *dp_aux)
 {
-	struct dp_aux_private *aux;
 	int ret;
 
 	if (!dp_aux) {
@@ -487,12 +486,7 @@ int dp_aux_register(struct drm_dp_aux *dp_aux)
 		return -EINVAL;
 	}
 
-	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
-
-	aux->dp_aux.name = "dpu_dp_aux";
-	aux->dp_aux.dev = aux->dev;
-	aux->dp_aux.transfer = dp_aux_transfer;
-	ret = drm_dp_aux_register(&aux->dp_aux);
+	ret = drm_dp_aux_register(dp_aux);
 	if (ret) {
 		DRM_ERROR("%s: failed to register drm aux: %d\n", __func__,
 				ret);
@@ -507,6 +501,21 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
 	drm_dp_aux_unregister(dp_aux);
 }
 
+static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux,
+				 unsigned long wait_us)
+{
+	int ret;
+	struct dp_aux_private *aux;
+
+	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
+
+	pm_runtime_get_sync(aux->dev);
+	ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+	pm_runtime_put_sync(aux->dev);
+
+	return ret;
+}
+
 struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
 			      bool is_edp)
 {
@@ -530,6 +539,17 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
 	aux->catalog = catalog;
 	aux->retry_cnt = 0;
 
+	/*
+	 * Use the drm_dp_aux_init() to use the aux adapter
+	 * before registering AUX with the DRM device so that
+	 * msm eDP panel can be detected by generic_dep_panel_probe().
+	 */
+	aux->dp_aux.name = "dpu_dp_aux";
+	aux->dp_aux.dev = dev;
+	aux->dp_aux.transfer = dp_aux_transfer;
+	aux->dp_aux.wait_hpd_asserted = dp_wait_hpd_asserted;
+	drm_dp_aux_init(&aux->dp_aux);
+
 	return &aux->dp_aux;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 79e56d9..df15145 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1207,6 +1207,17 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde
 	return NULL;
 }
 
+static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
+{
+	int rc;
+
+	rc = component_add(aux->dev, &dp_display_comp_ops);
+	if (rc)
+		DRM_ERROR("eDP component add failed, rc=%d\n", rc);
+
+	return rc;
+}
+
 static int dp_display_probe(struct platform_device *pdev)
 {
 	int rc = 0;
@@ -1272,10 +1283,18 @@ static int dp_display_probe(struct platform_device *pdev)
 	if (rc)
 		goto err;
 
-	rc = component_add(&pdev->dev, &dp_display_comp_ops);
-	if (rc) {
-		DRM_ERROR("component add failed, rc=%d\n", rc);
-		goto err;
+	if (dp->dp_display.is_edp) {
+		rc = devm_of_dp_aux_populate_bus(dp->aux, dp_auxbus_done_probe);
+		if (rc) {
+			DRM_ERROR("eDP auxbus population failed, rc=%d\n", rc);
+			goto err;
+		}
+	} else {
+		rc = component_add(&pdev->dev, &dp_display_comp_ops);
+		if (rc) {
+			DRM_ERROR("component add failed, rc=%d\n", rc);
+			goto err;
+		}
 	}
 
 	return rc;
@@ -1291,7 +1310,6 @@ static int dp_display_remove(struct platform_device *pdev)
 
 	component_del(&pdev->dev, &dp_display_comp_ops);
 	dp_display_deinit_sub_modules(dp);
-
 	platform_set_drvdata(pdev, NULL);
 
 	return 0;
@@ -1388,29 +1406,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 {
 	int rc;
 	struct dp_display_private *dp_priv;
-	struct device_node *aux_bus;
-	struct device *dev;
 
 	dp_priv = container_of(dp, struct dp_display_private, dp_display);
-	dev = &dp_priv->pdev->dev;
-	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
-
-	if (aux_bus && dp->is_edp) {
-		/*
-		 * The code below assumes that the panel will finish probing
-		 * by the time devm_of_dp_aux_populate_ep_devices() returns.
-		 * This isn't a great assumption since it will fail if the
-		 * panel driver is probed asynchronously but is the best we
-		 * can do without a bigger driver reorganization.
-		 */
-		rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
-		of_node_put(aux_bus);
-		if (rc)
-			goto error;
-	} else if (dp->is_edp) {
-		DRM_ERROR("eDP aux_bus not found\n");
-		return -ENODEV;
-	}
 
 	/*
 	 * External bridges are mandatory for eDP interfaces: one has to
@@ -1423,17 +1420,9 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 	if (!dp->is_edp && rc == -ENODEV)
 		return 0;
 
-	if (!rc) {
+	if (!rc)
 		dp->next_bridge = dp_priv->parser->next_bridge;
-		return 0;
-	}
 
-error:
-	if (dp->is_edp) {
-		of_dp_aux_depopulate_bus(dp_priv->aux);
-		dp_display_host_phy_exit(dp_priv);
-		dp_display_host_deinit(dp_priv);
-	}
 	return rc;
 }
 
-- 
2.7.4

Re: [PATCH v5 7/7] drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()

[Dmitry Baryshkov]

On 04/10/2023 19:27, Kuogee Hsieh wrote:
> Currently eDP population is done at msm_dp_modeset_init() which happen
> at binding time. Move eDP population to be done at display probe time
> so that probe deferral cases can be handled effectively.
> wait_for_hpd_asserted callback is added during drm_dp_aux_init()
> to ensure eDP's HPD is up before proceeding eDP population.
> 
> Changes in v5:
> -- inline dp_display_auxbus_population() and delete it
> 
> Changes in v4:
> -- delete duplicate initialize code to dp_aux before drm_dp_aux_register()
> -- delete of_get_child_by_name(dev->of_node, "aux-bus") and inline the function
> -- not initialize rc = 0
> 
> Changes in v3:
> -- add done_probing callback into devm_of_dp_aux_populate_bus()
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_aux.c     | 34 ++++++++++++++++-----
>   drivers/gpu/drm/msm/dp/dp_display.c | 59 +++++++++++++++----------------------
>   2 files changed, 51 insertions(+), 42 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 10b6eeb..03f4951 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -479,7 +479,6 @@ void dp_aux_deinit(struct drm_dp_aux *dp_aux)
>   
>   int dp_aux_register(struct drm_dp_aux *dp_aux)
>   {
> -	struct dp_aux_private *aux;
>   	int ret;
>   
>   	if (!dp_aux) {
> @@ -487,12 +486,7 @@ int dp_aux_register(struct drm_dp_aux *dp_aux)
>   		return -EINVAL;
>   	}
>   
> -	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> -
> -	aux->dp_aux.name = "dpu_dp_aux";
> -	aux->dp_aux.dev = aux->dev;
> -	aux->dp_aux.transfer = dp_aux_transfer;
> -	ret = drm_dp_aux_register(&aux->dp_aux);
> +	ret = drm_dp_aux_register(dp_aux);
>   	if (ret) {
>   		DRM_ERROR("%s: failed to register drm aux: %d\n", __func__,
>   				ret);
> @@ -507,6 +501,21 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
>   	drm_dp_aux_unregister(dp_aux);
>   }
>   
> +static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux,
> +				 unsigned long wait_us)
> +{
> +	int ret;
> +	struct dp_aux_private *aux;
> +
> +	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> +
> +	pm_runtime_get_sync(aux->dev);
> +	ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> +	pm_runtime_put_sync(aux->dev);
> +
> +	return ret;
> +}
> +
>   struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>   			      bool is_edp)
>   {
> @@ -530,6 +539,17 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>   	aux->catalog = catalog;
>   	aux->retry_cnt = 0;
>   
> +	/*
> +	 * Use the drm_dp_aux_init() to use the aux adapter
> +	 * before registering AUX with the DRM device so that
> +	 * msm eDP panel can be detected by generic_dep_panel_probe().
> +	 */
> +	aux->dp_aux.name = "dpu_dp_aux";
> +	aux->dp_aux.dev = dev;
> +	aux->dp_aux.transfer = dp_aux_transfer;
> +	aux->dp_aux.wait_hpd_asserted = dp_wait_hpd_asserted;
> +	drm_dp_aux_init(&aux->dp_aux);
> +
>   	return &aux->dp_aux;
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 79e56d9..df15145 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1207,6 +1207,17 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde
>   	return NULL;
>   }
>   
> +static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
> +{
> +	int rc;
> +
> +	rc = component_add(aux->dev, &dp_display_comp_ops);
> +	if (rc)
> +		DRM_ERROR("eDP component add failed, rc=%d\n", rc);
> +
> +	return rc;
> +}
> +
>   static int dp_display_probe(struct platform_device *pdev)
>   {
>   	int rc = 0;
> @@ -1272,10 +1283,18 @@ static int dp_display_probe(struct platform_device *pdev)
>   	if (rc)
>   		goto err;
>   
> -	rc = component_add(&pdev->dev, &dp_display_comp_ops);
> -	if (rc) {
> -		DRM_ERROR("component add failed, rc=%d\n", rc);
> -		goto err;
> +	if (dp->dp_display.is_edp) {
> +		rc = devm_of_dp_aux_populate_bus(dp->aux, dp_auxbus_done_probe);
> +		if (rc) {
> +			DRM_ERROR("eDP auxbus population failed, rc=%d\n", rc);
> +			goto err;
> +		}
> +	} else {
> +		rc = component_add(&pdev->dev, &dp_display_comp_ops);
> +		if (rc) {
> +			DRM_ERROR("component add failed, rc=%d\n", rc);
> +			goto err;
> +		}
>   	}
>   
>   	return rc;
> @@ -1291,7 +1310,6 @@ static int dp_display_remove(struct platform_device *pdev)
>   
>   	component_del(&pdev->dev, &dp_display_comp_ops);
>   	dp_display_deinit_sub_modules(dp);
> -

Nit: irrelevant to this patch.

>   	platform_set_drvdata(pdev, NULL);
>   
>   	return 0;
> @@ -1388,29 +1406,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>   {
>   	int rc;
>   	struct dp_display_private *dp_priv;
> -	struct device_node *aux_bus;
> -	struct device *dev;
>   
>   	dp_priv = container_of(dp, struct dp_display_private, dp_display);
> -	dev = &dp_priv->pdev->dev;
> -	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> -
> -	if (aux_bus && dp->is_edp) {
> -		/*
> -		 * The code below assumes that the panel will finish probing
> -		 * by the time devm_of_dp_aux_populate_ep_devices() returns.
> -		 * This isn't a great assumption since it will fail if the
> -		 * panel driver is probed asynchronously but is the best we
> -		 * can do without a bigger driver reorganization.
> -		 */
> -		rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
> -		of_node_put(aux_bus);
> -		if (rc)
> -			goto error;
> -	} else if (dp->is_edp) {
> -		DRM_ERROR("eDP aux_bus not found\n");
> -		return -ENODEV;
> -	}
>   
>   	/*
>   	 * External bridges are mandatory for eDP interfaces: one has to
> @@ -1423,17 +1420,9 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>   	if (!dp->is_edp && rc == -ENODEV)
>   		return 0;
>   
> -	if (!rc) {
> +	if (!rc)
>   		dp->next_bridge = dp_priv->parser->next_bridge;
> -		return 0;
> -	}
>   
> -error:
> -	if (dp->is_edp) {
> -		of_dp_aux_depopulate_bus(dp_priv->aux);
> -		dp_display_host_phy_exit(dp_priv);
> -		dp_display_host_deinit(dp_priv);
> -	}
>   	return rc;
>   }
>   

-- 
With best wishes
Dmitry