[PATCH v4 0/8] incorporate pm runtime framework and eDP clean up

[PATCH v4 0/8] incorporate pm runtime framework and eDP clean up

[Kuogee Hsieh]

Incorporate pm runtime framework into DP driver and clean up eDP
by moving of_dp_aux_populate_bus() to probe().

Kuogee Hsieh (8):
  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: add pm_runtime_force_suspend()/resume()
  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         |  40 +++-
 drivers/gpu/drm/msm/dp/dp_display.c     | 341 +++++++++++---------------------
 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, 161 insertions(+), 273 deletions(-)

-- 
2.7.4

[PATCH v4 1/8] drm/msm/dp: tie dp_display_irq_handler() with dp driver

[Kuogee Hsieh]

Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
which ties irq registration 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 at dp_display_probe() to have dp_display_irq_handler()
is tied with DP device.

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 | 29 +++++++++--------------------
 drivers/gpu/drm/msm/dp/dp_display.h |  1 -
 2 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 76f1395..5645178 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1193,30 +1193,21 @@ 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",
-				dp->irq, rc);
+		DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
 		return rc;
 	}
 
@@ -1287,6 +1278,10 @@ static int dp_display_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dp->dp_display);
 
+	rc = dp_display_request_irq(dp);
+	if (rc)
+		return rc;
+
 	rc = component_add(&pdev->dev, &dp_display_comp_ops);
 	if (rc) {
 		DRM_ERROR("component add failed, rc=%d\n", rc);
@@ -1549,12 +1544,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

[PATCH v4 2/8] 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. Rename
the is_connected flag with link_ready flag to match the state of DP
driver's state machine.

Changes in v4:
-- reworded commit etxt

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 5645178..9cb5a5b 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;
@@ -1350,16 +1349,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);
@@ -1752,8 +1751,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

[PATCH v4 3/8] 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 9cb5a5b..7ae3b8b 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 v4 4/8] drm/msm/dp: move parser->parse() and dp_power_client_init() to probe

[Kuogee Hsieh]

Move parser->parse() and dp_power_client_init() from dp_display_bind()
to dp_display_probe() in preparation of adding pm_runtime framework
at next patch.

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 7ae3b8b..3ef141c 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) {
@@ -1249,6 +1239,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");
+		return -EPROBE_DEFER;
+	}
+
+	rc = dp_power_client_init(dp->power);
+	if (rc) {
+		DRM_ERROR("Power client create failed\n");
+		return -EPROBE_DEFER;
+	}
+
 	/* setup event q */
 	mutex_init(&dp->event_mutex);
 	init_waitqueue_head(&dp->event_q);
-- 
2.7.4

[PATCH v4 5/8] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Kuogee Hsieh]

Currently DP driver is executed independent of PM runtime framework.
This lead to msm edp panel can not be detected by edp_panel driver at
generic_edp_panel_probe() due to aux dpcd read failed at msm edp driver.
Incorporating pm runtime framework into DP driver so that both power and
clocks to enable/disable host controller fits with PM runtime mechanism.
Once pm runtime framework is incorporated into DP driver, wake up device
from power up path is not necessary. Hence remove it.
Since DP is part of user interface, we choice to use autosuspend feature
with timer of one second. pm runtime suspends is prevented from happening
until timer expired.

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

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

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     |  6 +++
 drivers/gpu/drm/msm/dp/dp_display.c | 95 +++++++++++++++++++++++++++----------
 drivers/gpu/drm/msm/dp/dp_power.c   | 16 -------
 drivers/gpu/drm/msm/dp/dp_power.h   | 11 -----
 4 files changed, 77 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 8e3b677..22eb774 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,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 
 exit:
 	mutex_unlock(&aux->mutex);
+	pm_runtime_mark_last_busy(dp_aux->dev);
+	pm_runtime_put_autosuspend(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 3ef141c..bfb4692 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -276,8 +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;
 
-
-
 	dp->drm_dev = drm;
 	dp->aux->drm_dev = drm;
 	rc = dp_aux_register(dp->aux);
@@ -286,7 +284,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
 		goto end;
 	}
 
-
 	rc = dp_register_audio_driver(dev, dp->audio);
 	if (rc) {
 		DRM_ERROR("Audio registration Dp failed\n");
@@ -310,15 +307,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;
@@ -460,6 +452,16 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
 static int dp_display_usbpd_configure_cb(struct device *dev)
 {
 	struct dp_display_private *dp = dev_get_dp_display_private(dev);
+	int ret;
+
+	if (!dp->dp_display.internal_hpd) {
+		/* hpd through gpio */
+		ret = pm_runtime_resume_and_get(&dp->pdev->dev);
+		if (ret) {
+			DRM_ERROR("failed to start power\n");
+			return ret;
+		}
+	}
 
 	dp_display_host_phy_init(dp);
 
@@ -1086,7 +1088,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);
@@ -1263,6 +1264,13 @@ 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)
+		return rc;
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+
 	rc = dp_display_request_irq(dp);
 	if (rc)
 		return rc;
@@ -1285,6 +1293,34 @@ static int dp_display_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
+	dp_display_deinit_sub_modules(dp);
+
+	return 0;
+}
+
+static int dp_pm_runtime_suspend(struct device *dev)
+{
+	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_display_host_deinit(dp);
+
+	return 0;
+}
+
+static int dp_pm_runtime_resume(struct device *dev)
+{
+	struct dp_display_private *dp = dev_get_dp_display_private(dev);
+
+	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;
 }
 
@@ -1389,6 +1425,7 @@ static int dp_pm_suspend(struct device *dev)
 }
 
 static const struct dev_pm_ops dp_pm_ops = {
+	SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
 	.suspend = dp_pm_suspend,
 	.resume =  dp_pm_resume,
 };
@@ -1473,10 +1510,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.
@@ -1578,6 +1611,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 start power\n");
+		mutex_unlock(&dp_display->event_mutex);
+		return;
+	}
 
 	state = dp_display->hpd_state;
 	if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
@@ -1642,10 +1680,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);
 
@@ -1658,6 +1695,9 @@ 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_mark_last_busy(&dp_display->pdev->dev);
+	pm_runtime_put_autosuspend(&dp_display->pdev->dev);
 	mutex_unlock(&dp_display->event_mutex);
 }
 
@@ -1697,6 +1737,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 */
@@ -1718,6 +1764,9 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
 	dp_catalog_ctrl_hpd_disable(dp->catalog);
 
 	dp_display->internal_hpd = false;
+
+	pm_runtime_mark_last_busy(&dp->pdev->dev);
+	pm_runtime_put_autosuspend(&dp->pdev->dev);
 	mutex_unlock(&dp->event_mutex);
 }
 
@@ -1732,13 +1781,11 @@ 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)
+	/* hpd through gpio */
+	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

[PATCH v4 6/8] 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 bfb4692..9158a2c 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -55,7 +55,6 @@ enum {
 enum {
 	EV_NO_EVENT,
 	/* hpd events */
-	EV_HPD_INIT_SETUP,
 	EV_HPD_PLUG_INT,
 	EV_IRQ_HPD_INT,
 	EV_HPD_UNPLUG_INT,
@@ -1087,8 +1086,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;
@@ -1457,19 +1454,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 v4 7/8] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Kuogee Hsieh]

After incorporated pm_runtime framework into eDP/DP driver, the
original dp_pm_suspend() to handle power off both DP phy and
controller during suspend and dp_pm_resume() to handle power on
both DP phy and controller during resume are not necessary since
those function are replaced by dp_pm_runtime_suspend() and
dp_pm_runtime_resume() through pm runtime framework.
Therefore add pm framework provides functions,
pm_runtime_force_suspend()/resume() to complete incorporating pm
runtime framework into DP driver.

Changes in v4:
-- drop both dp_pm_prepare() and dp_pm_compete() from this change
-- delete ST_SUSPENDED state
-- rewording commit text to add more details regrading the purpose
   of this change

Changes in v3:
-- replace dp_pm_suspend() with pm_runtime_force_suspend()
-- replace dp_pm_resume() with pm_runtime_force_resume()

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

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 9158a2c..711d262 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -49,7 +49,6 @@ enum {
 	ST_CONNECTED,
 	ST_DISCONNECT_PENDING,
 	ST_DISPLAY_OFF,
-	ST_SUSPENDED,
 };
 
 enum {
@@ -560,7 +559,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
 	drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
 			dp->dp_display.connector_type, state);
 
-	if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
+	if (state == ST_DISPLAY_OFF) {
 		mutex_unlock(&dp->event_mutex);
 		return 0;
 	}
@@ -674,7 +673,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
 	drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
 			dp->dp_display.connector_type, state);
 
-	if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
+	if (state == ST_DISPLAY_OFF) {
 		mutex_unlock(&dp->event_mutex);
 		return 0;
 	}
@@ -1321,110 +1320,10 @@ static int dp_pm_runtime_resume(struct device *dev)
 	return 0;
 }
 
-static int dp_pm_resume(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct msm_dp *dp_display = platform_get_drvdata(pdev);
-	struct dp_display_private *dp;
-	int sink_count = 0;
-
-	dp = container_of(dp_display, struct dp_display_private, dp_display);
-
-	mutex_lock(&dp->event_mutex);
-
-	drm_dbg_dp(dp->drm_dev,
-		"Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
-		dp->dp_display.connector_type, dp->core_initialized,
-		dp->phy_initialized, dp_display->power_on);
-
-	/* start from disconnected state */
-	dp->hpd_state = ST_DISCONNECTED;
-
-	/* turn on dp ctrl/phy */
-	dp_display_host_init(dp);
-
-	if (dp_display->is_edp)
-		dp_catalog_ctrl_hpd_enable(dp->catalog);
-
-	if (dp_catalog_link_is_connected(dp->catalog)) {
-		/*
-		 * set sink to normal operation mode -- D0
-		 * before dpcd read
-		 */
-		dp_display_host_phy_init(dp);
-		dp_link_psm_config(dp->link, &dp->panel->link_info, false);
-		sink_count = drm_dp_read_sink_count(dp->aux);
-		if (sink_count < 0)
-			sink_count = 0;
-
-		dp_display_host_phy_exit(dp);
-	}
-
-	dp->link->sink_count = sink_count;
-	/*
-	 * can not declared display is connected unless
-	 * HDMI cable is plugged in and sink_count of
-	 * dongle become 1
-	 * also only signal audio when disconnected
-	 */
-	if (dp->link->sink_count) {
-		dp->dp_display.link_ready = true;
-	} else {
-		dp->dp_display.link_ready = false;
-		dp_display_handle_plugged_change(dp_display, false);
-	}
-
-	drm_dbg_dp(dp->drm_dev,
-		"After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
-		dp->dp_display.connector_type, dp->link->sink_count,
-		dp->dp_display.link_ready, dp->core_initialized,
-		dp->phy_initialized, dp_display->power_on);
-
-	mutex_unlock(&dp->event_mutex);
-
-	return 0;
-}
-
-static int dp_pm_suspend(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct msm_dp *dp_display = platform_get_drvdata(pdev);
-	struct dp_display_private *dp;
-
-	dp = container_of(dp_display, struct dp_display_private, dp_display);
-
-	mutex_lock(&dp->event_mutex);
-
-	drm_dbg_dp(dp->drm_dev,
-		"Before, type=%d core_inited=%d  phy_inited=%d power_on=%d\n",
-		dp->dp_display.connector_type, dp->core_initialized,
-		dp->phy_initialized, dp_display->power_on);
-
-	/* mainlink enabled */
-	if (dp_power_clk_status(dp->power, DP_CTRL_PM))
-		dp_ctrl_off_link_stream(dp->ctrl);
-
-	dp_display_host_phy_exit(dp);
-
-	/* host_init will be called at pm_resume */
-	dp_display_host_deinit(dp);
-
-	dp->hpd_state = ST_SUSPENDED;
-
-	drm_dbg_dp(dp->drm_dev,
-		"After, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
-		dp->dp_display.connector_type, dp->core_initialized,
-		dp->phy_initialized, dp_display->power_on);
-
-	mutex_unlock(&dp->event_mutex);
-
-	return 0;
-}
-
 static const struct dev_pm_ops dp_pm_ops = {
 	SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
-	.suspend = dp_pm_suspend,
-	.resume =  dp_pm_resume,
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+                                pm_runtime_force_resume)
 };
 
 static struct platform_driver dp_display_driver = {
@@ -1658,9 +1557,6 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 
-	if (dp->is_edp)
-		dp_hpd_unplug_handle(dp_display, 0);
-
 	mutex_lock(&dp_display->event_mutex);
 
 	state = dp_display->hpd_state;
@@ -1748,6 +1644,7 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
 	dp_catalog_ctrl_hpd_disable(dp->catalog);
 
 	dp_display->internal_hpd = false;
+	dp->hpd_state = ST_DISCONNECTED;
 
 	pm_runtime_mark_last_busy(&dp->pdev->dev);
 	pm_runtime_put_autosuspend(&dp->pdev->dev);
-- 
2.7.4

[PATCH v4 8/8] 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 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 | 69 ++++++++++++++++++-------------------
 2 files changed, 60 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 22eb774..425b5c5 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -480,7 +480,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) {
@@ -488,12 +487,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);
@@ -508,6 +502,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)
 {
@@ -531,6 +540,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 711d262..9a2b403 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1203,6 +1203,28 @@ 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 inline int dp_display_auxbus_population(struct dp_display_private *dp)
+{
+	int ret;
+
+	ret = devm_of_dp_aux_populate_bus(dp->aux, dp_auxbus_done_probe);
+	if (ret == -ENODEV)
+		DRM_ERROR("aux-bus not found\n");
+
+	return ret;
+}
+
 static int dp_display_probe(struct platform_device *pdev)
 {
 	int rc = 0;
@@ -1271,10 +1293,16 @@ static int dp_display_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
-	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);
+	if (dp->dp_display.is_edp) {
+		rc = dp_display_auxbus_population(dp);
+		if (rc)
+			DRM_ERROR("eDP auxbus population failed, rc=%d\n", rc);
+	} else {
+		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);
+		}
 	}
 
 	return rc;
@@ -1285,8 +1313,6 @@ static int dp_display_remove(struct platform_device *pdev)
 	struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
 
 	component_del(&pdev->dev, &dp_display_comp_ops);
-	dp_display_deinit_sub_modules(dp);
-
 	platform_set_drvdata(pdev, NULL);
 
 	dp_display_deinit_sub_modules(dp);
@@ -1385,29 +1411,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
@@ -1420,17 +1425,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 v4 1/8] drm/msm/dp: tie dp_display_irq_handler() with dp driver

[Dmitry Baryshkov]

On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()

dp_display_request_irq()

> which ties irq registration to the DPU device's life cycle, while depending on
> resources that are released as the DP device is torn down. Move register DP

`registering` or `registration of`

> driver irq handler at dp_display_probe() to have dp_display_irq_handler()

IRQ, s/at/to/

> is tied with DP device.

s/is //

Moreover, your commit does more that you have described in the commit
message. It also e.g. switches to platform_get_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 | 29 +++++++++--------------------
>  drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>  2 files changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 76f1395..5645178 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1193,30 +1193,21 @@ 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",
> -                               dp->irq, rc);
> +               DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);

Please don't mix functional changes with code reformatting.

>                 return rc;
>         }
>
> @@ -1287,6 +1278,10 @@ static int dp_display_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, &dp->dp_display);
>
> +       rc = dp_display_request_irq(dp);
> +       if (rc)
> +               return rc;

Who will perform component teardown for you if the driver just returns
an error here?

> +
>         rc = component_add(&pdev->dev, &dp_display_comp_ops);
>         if (rc) {
>                 DRM_ERROR("component add failed, rc=%d\n", rc);
> @@ -1549,12 +1544,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
>


-- 
With best wishes
Dmitry

Re: [PATCH v4 0/8] incorporate pm runtime framework and eDP clean up

[Dmitry Baryshkov]

On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Incorporate pm runtime framework into DP driver and clean up eDP
> by moving of_dp_aux_populate_bus() to probe().

Dear Kuogee. Let me quote my response to v1 of your series:

Please use sensible prefix for cover letters too. It helps people
understand, which driver/area is touched by the patchset.

This is v4 already and the cover letter still has the same subject line.
If you are ignoring the review comments, should I start ignoring your patches?

>
> Kuogee Hsieh (8):
>   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: add pm_runtime_force_suspend()/resume()
>   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         |  40 +++-
>  drivers/gpu/drm/msm/dp/dp_display.c     | 341 +++++++++++---------------------
>  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, 161 insertions(+), 273 deletions(-)
>
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH v4 2/8] drm/msm/dp: rename is_connected with link_ready

[Dmitry Baryshkov]

On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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 v4:
> -- reworded commit etxt

s/etxt/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 5645178..9cb5a5b 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;
> @@ -1350,16 +1349,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);
> @@ -1752,8 +1751,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
>


--
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Move parser->parse() and dp_power_client_init() from dp_display_bind()
> to dp_display_probe() in preparation of adding pm_runtime framework
> at next patch.

This describes what the patch does, not why it is done. Could you
please rewrite it to describe the reason for the change?

>
> 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 7ae3b8b..3ef141c 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) {
> @@ -1249,6 +1239,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");
> +               return -EPROBE_DEFER;
> +       }
> +
> +       rc = dp_power_client_init(dp->power);
> +       if (rc) {
> +               DRM_ERROR("Power client create failed\n");
> +               return -EPROBE_DEFER;
> +       }
> +
>         /* setup event q */
>         mutex_init(&dp->event_mutex);
>         init_waitqueue_head(&dp->event_q);
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Move parser->parse() and dp_power_client_init() from dp_display_bind()
> to dp_display_probe() in preparation of adding pm_runtime framework
> at next patch.
>
> 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 7ae3b8b..3ef141c 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) {
> @@ -1249,6 +1239,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");
> +               return -EPROBE_DEFER;
> +       }
> +
> +       rc = dp_power_client_init(dp->power);
> +       if (rc) {
> +               DRM_ERROR("Power client create failed\n");
> +               return -EPROBE_DEFER;
> +       }

Hit enter too soon. No submodules teardown, so NAK.

Also please propagate returned error codes instead of inventing
EPROBE_DEFER on your own.

> +
>         /* setup event q */
>         mutex_init(&dp->event_mutex);
>         init_waitqueue_head(&dp->event_q);
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Currently DP driver is executed independent of PM runtime framework.
> This lead to msm edp panel can not be detected by edp_panel driver at
> generic_edp_panel_probe() due to aux dpcd read failed at msm edp driver.

eDP, AUX, DPCD. leads. not being detected, s/at/during.

Also there is no msm eDP driver.

> Incorporating pm runtime framework into DP driver so that both power and

Incorporate. PM

> clocks to enable/disable host controller fits with PM runtime mechanism.

Ugh?

> Once pm runtime framework is incorporated into DP driver, wake up device

waking

> from power up path is not necessary. Hence remove it.
> Since DP is part of user interface, we choice to use autosuspend feature
> with timer of one second. pm runtime suspends is prevented from happening
> until timer expired.

No, this is not the right reason to use autosuspend. Please use normal
suspend unless there is a performance regression caused by the
suspend/resume paths.

>
> 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
>
> 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
>
> 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     |  6 +++
>  drivers/gpu/drm/msm/dp/dp_display.c | 95 +++++++++++++++++++++++++++----------
>  drivers/gpu/drm/msm/dp/dp_power.c   | 16 -------
>  drivers/gpu/drm/msm/dp/dp_power.h   | 11 -----
>  4 files changed, 77 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 8e3b677..22eb774 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,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>
>  exit:
>         mutex_unlock(&aux->mutex);
> +       pm_runtime_mark_last_busy(dp_aux->dev);
> +       pm_runtime_put_autosuspend(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 3ef141c..bfb4692 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -276,8 +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;
>
> -
> -

Is this also a part of pm_runtime support? No, it is not.

>         dp->drm_dev = drm;
>         dp->aux->drm_dev = drm;
>         rc = dp_aux_register(dp->aux);
> @@ -286,7 +284,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>                 goto end;
>         }
>
> -

And this. If you want to clean up stray empty lines, please split that
to a separate patch.

>         rc = dp_register_audio_driver(dev, dp->audio);
>         if (rc) {
>                 DRM_ERROR("Audio registration Dp failed\n");
> @@ -310,15 +307,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;
> @@ -460,6 +452,16 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>  static int dp_display_usbpd_configure_cb(struct device *dev)
>  {
>         struct dp_display_private *dp = dev_get_dp_display_private(dev);
> +       int ret;
> +
> +       if (!dp->dp_display.internal_hpd) {

I'd say, drop the condition here.

> +               /* hpd through gpio */

Why? !internal_hpd means any possible way of delivering an HPD event.
For example, via the USB-C TCPM.

> +               ret = pm_runtime_resume_and_get(&dp->pdev->dev);

Which function contains corresponding pm_runtime_put()?

> +               if (ret) {
> +                       DRM_ERROR("failed to start power\n");

failed to resume

> +                       return ret;
> +               }
> +       }
>
>         dp_display_host_phy_init(dp);
>
> @@ -1086,7 +1088,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);
> @@ -1263,6 +1264,13 @@ 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)
> +               return rc;

Missing submodules deinit.

> +
> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> +       pm_runtime_use_autosuspend(&pdev->dev);
> +
>         rc = dp_display_request_irq(dp);
>         if (rc)
>                 return rc;
> @@ -1285,6 +1293,34 @@ static int dp_display_remove(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, NULL);
>
> +       dp_display_deinit_sub_modules(dp);

There is already a call to dp_display_deinit_sub_modules() in
dp_display_remove().
> +
> +       return 0;
> +}
> +
> +static int dp_pm_runtime_suspend(struct device *dev)
> +{
> +       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);
> +       }

I don't see where this code was removed.

> +       dp_display_host_deinit(dp);
> +
> +       return 0;
> +}
> +
> +static int dp_pm_runtime_resume(struct device *dev)
> +{
> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
> +
> +       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;
>  }
>
> @@ -1389,6 +1425,7 @@ static int dp_pm_suspend(struct device *dev)
>  }
>
>  static const struct dev_pm_ops dp_pm_ops = {
> +       SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
>         .suspend = dp_pm_suspend,
>         .resume =  dp_pm_resume,
>  };
> @@ -1473,10 +1510,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.
> @@ -1578,6 +1611,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 start power\n");

failed to resume.

> +               mutex_unlock(&dp_display->event_mutex);
> +               return;
> +       }
>
>         state = dp_display->hpd_state;
>         if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
> @@ -1642,10 +1680,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);
>
> @@ -1658,6 +1695,9 @@ 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_mark_last_busy(&dp_display->pdev->dev);
> +       pm_runtime_put_autosuspend(&dp_display->pdev->dev);
>         mutex_unlock(&dp_display->event_mutex);
>  }
>
> @@ -1697,6 +1737,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 */
> @@ -1718,6 +1764,9 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>         dp_catalog_ctrl_hpd_disable(dp->catalog);
>
>         dp_display->internal_hpd = false;
> +
> +       pm_runtime_mark_last_busy(&dp->pdev->dev);
> +       pm_runtime_put_autosuspend(&dp->pdev->dev);
>         mutex_unlock(&dp->event_mutex);
>  }
>
> @@ -1732,13 +1781,11 @@ 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)
> +       /* hpd through gpio */

Lack of the comment might be better than the incorrect comment.

> +       if (!dp_display->link_ready && status == connector_status_connected) {
> +               dp->hpd_state = ST_DISCONNECTED;

Is this also a part of pm_runtime support?

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


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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 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 | 69 ++++++++++++++++++-------------------
>  2 files changed, 60 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 22eb774..425b5c5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -480,7 +480,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) {
> @@ -488,12 +487,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);
> @@ -508,6 +502,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);

Ok, so here you have used put_sync instead of autosuspend. Can we have
some uniformity? (I'd prefer to see put_sync or just put everywhere)

> +
> +       return ret;
> +}
> +
>  struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>                               bool is_edp)
>  {
> @@ -531,6 +540,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().

eDP, AUX, generic_edp_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 711d262..9a2b403 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1203,6 +1203,28 @@ 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);

drop.

> +
> +       return rc;
> +}
> +
> +static inline int dp_display_auxbus_population(struct dp_display_private *dp)

It's not `population`. It is just `populate`.

Also please inline this function.


> +{
> +       int ret;
> +
> +       ret = devm_of_dp_aux_populate_bus(dp->aux, dp_auxbus_done_probe);
> +       if (ret == -ENODEV)
> +               DRM_ERROR("aux-bus not found\n");
> +
> +       return ret;
> +}
> +
>  static int dp_display_probe(struct platform_device *pdev)
>  {
>         int rc = 0;
> @@ -1271,10 +1293,16 @@ static int dp_display_probe(struct platform_device *pdev)
>         if (rc)
>                 return rc;
>
> -       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);
> +       if (dp->dp_display.is_edp) {
> +               rc = dp_display_auxbus_population(dp);
> +               if (rc)
> +                       DRM_ERROR("eDP auxbus population failed, rc=%d\n", rc);
> +       } else {
> +               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);
> +               }
>         }
>
>         return rc;
> @@ -1285,8 +1313,6 @@ static int dp_display_remove(struct platform_device *pdev)
>         struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
>
>         component_del(&pdev->dev, &dp_display_comp_ops);
> -       dp_display_deinit_sub_modules(dp);
> -
>         platform_set_drvdata(pdev, NULL);
>
>         dp_display_deinit_sub_modules(dp);
> @@ -1385,29 +1411,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
> @@ -1420,17 +1425,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
>


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> After incorporated pm_runtime framework into eDP/DP driver, the

incorporating


> original dp_pm_suspend() to handle power off both DP phy and
> controller during suspend and dp_pm_resume() to handle power on
> both DP phy and controller during resume are not necessary since
> those function are replaced by dp_pm_runtime_suspend() and
> dp_pm_runtime_resume() through pm runtime framework.
> Therefore add pm framework provides functions,
> pm_runtime_force_suspend()/resume() to complete incorporating pm
> runtime framework into DP driver.
>
> Changes in v4:
> -- drop both dp_pm_prepare() and dp_pm_compete() from this change
> -- delete ST_SUSPENDED state
> -- rewording commit text to add more details regrading the purpose
>    of this change
>
> Changes in v3:
> -- replace dp_pm_suspend() with pm_runtime_force_suspend()
> -- replace dp_pm_resume() with pm_runtime_force_resume()
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 113 ++----------------------------------
>  1 file changed, 5 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 9158a2c..711d262 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -49,7 +49,6 @@ enum {
>         ST_CONNECTED,
>         ST_DISCONNECT_PENDING,
>         ST_DISPLAY_OFF,
> -       ST_SUSPENDED,
>  };
>
>  enum {
> @@ -560,7 +559,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>         drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
>                         dp->dp_display.connector_type, state);
>
> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
> +       if (state == ST_DISPLAY_OFF) {
>                 mutex_unlock(&dp->event_mutex);
>                 return 0;
>         }
> @@ -674,7 +673,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
>         drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
>                         dp->dp_display.connector_type, state);
>
> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
> +       if (state == ST_DISPLAY_OFF) {
>                 mutex_unlock(&dp->event_mutex);
>                 return 0;
>         }
> @@ -1321,110 +1320,10 @@ static int dp_pm_runtime_resume(struct device *dev)
>         return 0;
>  }
>
> -static int dp_pm_resume(struct device *dev)
> -{
> -       struct platform_device *pdev = to_platform_device(dev);
> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
> -       struct dp_display_private *dp;
> -       int sink_count = 0;
> -
> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
> -
> -       mutex_lock(&dp->event_mutex);
> -
> -       drm_dbg_dp(dp->drm_dev,
> -               "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
> -               dp->dp_display.connector_type, dp->core_initialized,
> -               dp->phy_initialized, dp_display->power_on);
> -
> -       /* start from disconnected state */
> -       dp->hpd_state = ST_DISCONNECTED;
> -
> -       /* turn on dp ctrl/phy */
> -       dp_display_host_init(dp);
> -
> -       if (dp_display->is_edp)
> -               dp_catalog_ctrl_hpd_enable(dp->catalog);
> -
> -       if (dp_catalog_link_is_connected(dp->catalog)) {
> -               /*
> -                * set sink to normal operation mode -- D0
> -                * before dpcd read
> -                */
> -               dp_display_host_phy_init(dp);
> -               dp_link_psm_config(dp->link, &dp->panel->link_info, false);
> -               sink_count = drm_dp_read_sink_count(dp->aux);
> -               if (sink_count < 0)
> -                       sink_count = 0;
> -
> -               dp_display_host_phy_exit(dp);
> -       }
> -
> -       dp->link->sink_count = sink_count;
> -       /*
> -        * can not declared display is connected unless
> -        * HDMI cable is plugged in and sink_count of
> -        * dongle become 1
> -        * also only signal audio when disconnected
> -        */
> -       if (dp->link->sink_count) {
> -               dp->dp_display.link_ready = true;
> -       } else {
> -               dp->dp_display.link_ready = false;
> -               dp_display_handle_plugged_change(dp_display, false);
> -       }
> -
> -       drm_dbg_dp(dp->drm_dev,
> -               "After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
> -               dp->dp_display.connector_type, dp->link->sink_count,
> -               dp->dp_display.link_ready, dp->core_initialized,
> -               dp->phy_initialized, dp_display->power_on);
> -
> -       mutex_unlock(&dp->event_mutex);
> -
> -       return 0;
> -}
> -
> -static int dp_pm_suspend(struct device *dev)
> -{
> -       struct platform_device *pdev = to_platform_device(dev);
> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
> -       struct dp_display_private *dp;
> -
> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
> -
> -       mutex_lock(&dp->event_mutex);
> -
> -       drm_dbg_dp(dp->drm_dev,
> -               "Before, type=%d core_inited=%d  phy_inited=%d power_on=%d\n",
> -               dp->dp_display.connector_type, dp->core_initialized,
> -               dp->phy_initialized, dp_display->power_on);
> -
> -       /* mainlink enabled */
> -       if (dp_power_clk_status(dp->power, DP_CTRL_PM))
> -               dp_ctrl_off_link_stream(dp->ctrl);
> -
> -       dp_display_host_phy_exit(dp);

I was under the impression that dp_pm_runtime_suspend / _resume
functions perform phy init/exit only in eDP cases. Can we really drop
the main suspend/resume functions?

> -
> -       /* host_init will be called at pm_resume */
> -       dp_display_host_deinit(dp);
> -
> -       dp->hpd_state = ST_SUSPENDED;
> -
> -       drm_dbg_dp(dp->drm_dev,
> -               "After, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
> -               dp->dp_display.connector_type, dp->core_initialized,
> -               dp->phy_initialized, dp_display->power_on);
> -
> -       mutex_unlock(&dp->event_mutex);
> -
> -       return 0;
> -}
> -
>  static const struct dev_pm_ops dp_pm_ops = {
>         SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
> -       .suspend = dp_pm_suspend,
> -       .resume =  dp_pm_resume,
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                                pm_runtime_force_resume)
>  };
>
>  static struct platform_driver dp_display_driver = {
> @@ -1658,9 +1557,6 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
>
>         dp_display = container_of(dp, struct dp_display_private, dp_display);
>
> -       if (dp->is_edp)
> -               dp_hpd_unplug_handle(dp_display, 0);

Why?

> -
>         mutex_lock(&dp_display->event_mutex);
>
>         state = dp_display->hpd_state;
> @@ -1748,6 +1644,7 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>         dp_catalog_ctrl_hpd_disable(dp->catalog);
>
>         dp_display->internal_hpd = false;
> +       dp->hpd_state = ST_DISCONNECTED;

Why? We have only disabled sending of the HPD events. The dongle might
still be connected.

>
>         pm_runtime_mark_last_busy(&dp->pdev->dev);
>         pm_runtime_put_autosuspend(&dp->pdev->dev);
> --
> 2.7.4
>


--
With best wishes

Dmitry

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

[Kuogee Hsieh]

On 9/27/2023 2:41 PM, Dmitry Baryshkov wrote:
> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> Currently DP driver is executed independent of PM runtime framework.
>> This lead to msm edp panel can not be detected by edp_panel driver at
>> generic_edp_panel_probe() due to aux dpcd read failed at msm edp driver.
> eDP, AUX, DPCD. leads. not being detected, s/at/during.
>
> Also there is no msm eDP driver.
>
>> Incorporating pm runtime framework into DP driver so that both power and
> Incorporate. PM
>
>> clocks to enable/disable host controller fits with PM runtime mechanism.
> Ugh?
>
>> Once pm runtime framework is incorporated into DP driver, wake up device
> waking
>
>> from power up path is not necessary. Hence remove it.
>> Since DP is part of user interface, we choice to use autosuspend feature
>> with timer of one second. pm runtime suspends is prevented from happening
>> until timer expired.
> No, this is not the right reason to use autosuspend. Please use normal
> suspend unless there is a performance regression caused by the
> suspend/resume paths.
>
>> 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
>>
>> 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
>>
>> 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     |  6 +++
>>   drivers/gpu/drm/msm/dp/dp_display.c | 95 +++++++++++++++++++++++++++----------
>>   drivers/gpu/drm/msm/dp/dp_power.c   | 16 -------
>>   drivers/gpu/drm/msm/dp/dp_power.h   | 11 -----
>>   4 files changed, 77 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 8e3b677..22eb774 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,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>>
>>   exit:
>>          mutex_unlock(&aux->mutex);
>> +       pm_runtime_mark_last_busy(dp_aux->dev);
>> +       pm_runtime_put_autosuspend(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 3ef141c..bfb4692 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -276,8 +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;
>>
>> -
>> -
> Is this also a part of pm_runtime support? No, it is not.
>
>>          dp->drm_dev = drm;
>>          dp->aux->drm_dev = drm;
>>          rc = dp_aux_register(dp->aux);
>> @@ -286,7 +284,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>>                  goto end;
>>          }
>>
>> -
> And this. If you want to clean up stray empty lines, please split that
> to a separate patch.
>
>>          rc = dp_register_audio_driver(dev, dp->audio);
>>          if (rc) {
>>                  DRM_ERROR("Audio registration Dp failed\n");
>> @@ -310,15 +307,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;
>> @@ -460,6 +452,16 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>>   static int dp_display_usbpd_configure_cb(struct device *dev)
>>   {
>>          struct dp_display_private *dp = dev_get_dp_display_private(dev);
>> +       int ret;
>> +
>> +       if (!dp->dp_display.internal_hpd) {
> I'd say, drop the condition here.

ok,

to match p,_runtime_get and pm_runtime_put, I will move 
pm_runtime_resume_and_get(&dp->pdev->dev) from here to 
dp_hpd_plug_handle() and

add pm_runtime_put() at dp_hpd_unpluh_handle().

>
>> +               /* hpd through gpio */
> Why? !internal_hpd means any possible way of delivering an HPD event.
> For example, via the USB-C TCPM.
>
>> +               ret = pm_runtime_resume_and_get(&dp->pdev->dev);
> Which function contains corresponding pm_runtime_put()?

should be at dp_hpd_unplug_handle().

I had missed that, will add it at next patch.

>
>> +               if (ret) {
>> +                       DRM_ERROR("failed to start power\n");
> failed to resume
>
>> +                       return ret;
>> +               }
>> +       }
>>
>>          dp_display_host_phy_init(dp);
>>
>> @@ -1086,7 +1088,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);
>> @@ -1263,6 +1264,13 @@ 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)
>> +               return rc;
> Missing submodules deinit.
>
>> +
>> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>> +       pm_runtime_use_autosuspend(&pdev->dev);
>> +
>>          rc = dp_display_request_irq(dp);
>>          if (rc)
>>                  return rc;
>> @@ -1285,6 +1293,34 @@ static int dp_display_remove(struct platform_device *pdev)
>>
>>          platform_set_drvdata(pdev, NULL);
>>
>> +       dp_display_deinit_sub_modules(dp);
> There is already a call to dp_display_deinit_sub_modules() in
> dp_display_remove().
>> +
>> +       return 0;
>> +}
>> +
>> +static int dp_pm_runtime_suspend(struct device *dev)
>> +{
>> +       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);
>> +       }
> I don't see where this code was removed.
>
>> +       dp_display_host_deinit(dp);
>> +
>> +       return 0;
>> +}
>> +
>> +static int dp_pm_runtime_resume(struct device *dev)
>> +{
>> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>> +
>> +       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;
>>   }
>>
>> @@ -1389,6 +1425,7 @@ static int dp_pm_suspend(struct device *dev)
>>   }
>>
>>   static const struct dev_pm_ops dp_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
>>          .suspend = dp_pm_suspend,
>>          .resume =  dp_pm_resume,
>>   };
>> @@ -1473,10 +1510,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.
>> @@ -1578,6 +1611,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 start power\n");
> failed to resume.
>
>> +               mutex_unlock(&dp_display->event_mutex);
>> +               return;
>> +       }
>>
>>          state = dp_display->hpd_state;
>>          if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>> @@ -1642,10 +1680,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);
>>
>> @@ -1658,6 +1695,9 @@ 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_mark_last_busy(&dp_display->pdev->dev);
>> +       pm_runtime_put_autosuspend(&dp_display->pdev->dev);
>>          mutex_unlock(&dp_display->event_mutex);
>>   }
>>
>> @@ -1697,6 +1737,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 */
>> @@ -1718,6 +1764,9 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>>          dp_catalog_ctrl_hpd_disable(dp->catalog);
>>
>>          dp_display->internal_hpd = false;
>> +
>> +       pm_runtime_mark_last_busy(&dp->pdev->dev);
>> +       pm_runtime_put_autosuspend(&dp->pdev->dev);
>>          mutex_unlock(&dp->event_mutex);
>>   }
>>
>> @@ -1732,13 +1781,11 @@ 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)
>> +       /* hpd through gpio */
> Lack of the comment might be better than the incorrect comment.
>
>> +       if (!dp_display->link_ready && status == connector_status_connected) {
>> +               dp->hpd_state = ST_DISCONNECTED;
> Is this also a part of pm_runtime support?
>
>>                  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 v4 5/8] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Kuogee Hsieh]

On 9/27/2023 2:41 PM, Dmitry Baryshkov wrote:
> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> Currently DP driver is executed independent of PM runtime framework.
>> This lead to msm edp panel can not be detected by edp_panel driver at
>> generic_edp_panel_probe() due to aux dpcd read failed at msm edp driver.
> eDP, AUX, DPCD. leads. not being detected, s/at/during.
>
> Also there is no msm eDP driver.
>
>> Incorporating pm runtime framework into DP driver so that both power and
> Incorporate. PM
>
>> clocks to enable/disable host controller fits with PM runtime mechanism.
> Ugh?
>
>> Once pm runtime framework is incorporated into DP driver, wake up device
> waking
>
>> from power up path is not necessary. Hence remove it.
>> Since DP is part of user interface, we choice to use autosuspend feature
>> with timer of one second. pm runtime suspends is prevented from happening
>> until timer expired.
> No, this is not the right reason to use autosuspend. Please use normal
> suspend unless there is a performance regression caused by the
> suspend/resume paths.
>
>> 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
>>
>> 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
>>
>> 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     |  6 +++
>>   drivers/gpu/drm/msm/dp/dp_display.c | 95 +++++++++++++++++++++++++++----------
>>   drivers/gpu/drm/msm/dp/dp_power.c   | 16 -------
>>   drivers/gpu/drm/msm/dp/dp_power.h   | 11 -----
>>   4 files changed, 77 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 8e3b677..22eb774 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,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>>
>>   exit:
>>          mutex_unlock(&aux->mutex);
>> +       pm_runtime_mark_last_busy(dp_aux->dev);
>> +       pm_runtime_put_autosuspend(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 3ef141c..bfb4692 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -276,8 +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;
>>
>> -
>> -
> Is this also a part of pm_runtime support? No, it is not.
>
>>          dp->drm_dev = drm;
>>          dp->aux->drm_dev = drm;
>>          rc = dp_aux_register(dp->aux);
>> @@ -286,7 +284,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>>                  goto end;
>>          }
>>
>> -
> And this. If you want to clean up stray empty lines, please split that
> to a separate patch.
>
>>          rc = dp_register_audio_driver(dev, dp->audio);
>>          if (rc) {
>>                  DRM_ERROR("Audio registration Dp failed\n");
>> @@ -310,15 +307,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;
>> @@ -460,6 +452,16 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>>   static int dp_display_usbpd_configure_cb(struct device *dev)
>>   {
>>          struct dp_display_private *dp = dev_get_dp_display_private(dev);
>> +       int ret;
>> +
>> +       if (!dp->dp_display.internal_hpd) {
> I'd say, drop the condition here.
>
>> +               /* hpd through gpio */
> Why? !internal_hpd means any possible way of delivering an HPD event.
> For example, via the USB-C TCPM.
>
>> +               ret = pm_runtime_resume_and_get(&dp->pdev->dev);
> Which function contains corresponding pm_runtime_put()?
>
>> +               if (ret) {
>> +                       DRM_ERROR("failed to start power\n");
> failed to resume
>
>> +                       return ret;
>> +               }
>> +       }
>>
>>          dp_display_host_phy_init(dp);
>>
>> @@ -1086,7 +1088,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);
>> @@ -1263,6 +1264,13 @@ 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)
>> +               return rc;
> Missing submodules deinit.
>
>> +
>> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>> +       pm_runtime_use_autosuspend(&pdev->dev);
>> +
>>          rc = dp_display_request_irq(dp);
>>          if (rc)
>>                  return rc;
>> @@ -1285,6 +1293,34 @@ static int dp_display_remove(struct platform_device *pdev)
>>
>>          platform_set_drvdata(pdev, NULL);
>>
>> +       dp_display_deinit_sub_modules(dp);
> There is already a call to dp_display_deinit_sub_modules() in
> dp_display_remove().
>> +
>> +       return 0;
>> +}
>> +
>> +static int dp_pm_runtime_suspend(struct device *dev)
>> +{
>> +       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);
>> +       }
> I don't see where this code was removed.

this code block is adapted from dp_pm_suspend().

dp_pm_suspend() will be removed at "add 
pm_runtime_force_suspend()/resume()" patch.

>
>> +       dp_display_host_deinit(dp);
>> +
>> +       return 0;
>> +}
>> +
>> +static int dp_pm_runtime_resume(struct device *dev)
>> +{
>> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>> +
>> +       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;
>>   }
>>
>> @@ -1389,6 +1425,7 @@ static int dp_pm_suspend(struct device *dev)
>>   }
>>
>>   static const struct dev_pm_ops dp_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
>>          .suspend = dp_pm_suspend,
>>          .resume =  dp_pm_resume,
>>   };
>> @@ -1473,10 +1510,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.
>> @@ -1578,6 +1611,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 start power\n");
> failed to resume.
>
>> +               mutex_unlock(&dp_display->event_mutex);
>> +               return;
>> +       }
>>
>>          state = dp_display->hpd_state;
>>          if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>> @@ -1642,10 +1680,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);
>>
>> @@ -1658,6 +1695,9 @@ 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_mark_last_busy(&dp_display->pdev->dev);
>> +       pm_runtime_put_autosuspend(&dp_display->pdev->dev);
>>          mutex_unlock(&dp_display->event_mutex);
>>   }
>>
>> @@ -1697,6 +1737,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 */
>> @@ -1718,6 +1764,9 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>>          dp_catalog_ctrl_hpd_disable(dp->catalog);
>>
>>          dp_display->internal_hpd = false;
>> +
>> +       pm_runtime_mark_last_busy(&dp->pdev->dev);
>> +       pm_runtime_put_autosuspend(&dp->pdev->dev);
>>          mutex_unlock(&dp->event_mutex);
>>   }
>>
>> @@ -1732,13 +1781,11 @@ 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)
>> +       /* hpd through gpio */
> Lack of the comment might be better than the incorrect comment.
>
>> +       if (!dp_display->link_ready && status == connector_status_connected) {
>> +               dp->hpd_state = ST_DISCONNECTED;
> Is this also a part of pm_runtime support?

yes, this is for hpd_internal == false case.

The hpd_state need to be at init state.

>
>>                  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 v4 5/8] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Dmitry Baryshkov]

On Tue, 3 Oct 2023 at 01:48, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 9/27/2023 2:41 PM, Dmitry Baryshkov wrote:
> > On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> Currently DP driver is executed independent of PM runtime framework.
> >> This lead to msm edp panel can not be detected by edp_panel driver at
> >> generic_edp_panel_probe() due to aux dpcd read failed at msm edp driver.
> > eDP, AUX, DPCD. leads. not being detected, s/at/during.
> >
> > Also there is no msm eDP driver.
> >
> >> Incorporating pm runtime framework into DP driver so that both power and
> > Incorporate. PM
> >
> >> clocks to enable/disable host controller fits with PM runtime mechanism.
> > Ugh?
> >
> >> Once pm runtime framework is incorporated into DP driver, wake up device
> > waking
> >
> >> from power up path is not necessary. Hence remove it.
> >> Since DP is part of user interface, we choice to use autosuspend feature
> >> with timer of one second. pm runtime suspends is prevented from happening
> >> until timer expired.
> > No, this is not the right reason to use autosuspend. Please use normal
> > suspend unless there is a performance regression caused by the
> > suspend/resume paths.
> >
> >> 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
> >>
> >> 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
> >>
> >> 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     |  6 +++
> >>   drivers/gpu/drm/msm/dp/dp_display.c | 95 +++++++++++++++++++++++++++----------
> >>   drivers/gpu/drm/msm/dp/dp_power.c   | 16 -------
> >>   drivers/gpu/drm/msm/dp/dp_power.h   | 11 -----
> >>   4 files changed, 77 insertions(+), 51 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> index 8e3b677..22eb774 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,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> >>
> >>   exit:
> >>          mutex_unlock(&aux->mutex);
> >> +       pm_runtime_mark_last_busy(dp_aux->dev);
> >> +       pm_runtime_put_autosuspend(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 3ef141c..bfb4692 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -276,8 +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;
> >>
> >> -
> >> -
> > Is this also a part of pm_runtime support? No, it is not.
> >
> >>          dp->drm_dev = drm;
> >>          dp->aux->drm_dev = drm;
> >>          rc = dp_aux_register(dp->aux);
> >> @@ -286,7 +284,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
> >>                  goto end;
> >>          }
> >>
> >> -
> > And this. If you want to clean up stray empty lines, please split that
> > to a separate patch.
> >
> >>          rc = dp_register_audio_driver(dev, dp->audio);
> >>          if (rc) {
> >>                  DRM_ERROR("Audio registration Dp failed\n");
> >> @@ -310,15 +307,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;
> >> @@ -460,6 +452,16 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
> >>   static int dp_display_usbpd_configure_cb(struct device *dev)
> >>   {
> >>          struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >> +       int ret;
> >> +
> >> +       if (!dp->dp_display.internal_hpd) {
> > I'd say, drop the condition here.
> >
> >> +               /* hpd through gpio */
> > Why? !internal_hpd means any possible way of delivering an HPD event.
> > For example, via the USB-C TCPM.
> >
> >> +               ret = pm_runtime_resume_and_get(&dp->pdev->dev);
> > Which function contains corresponding pm_runtime_put()?
> >
> >> +               if (ret) {
> >> +                       DRM_ERROR("failed to start power\n");
> > failed to resume
> >
> >> +                       return ret;
> >> +               }
> >> +       }
> >>
> >>          dp_display_host_phy_init(dp);
> >>
> >> @@ -1086,7 +1088,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);
> >> @@ -1263,6 +1264,13 @@ 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)
> >> +               return rc;
> > Missing submodules deinit.
> >
> >> +
> >> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >> +       pm_runtime_use_autosuspend(&pdev->dev);
> >> +
> >>          rc = dp_display_request_irq(dp);
> >>          if (rc)
> >>                  return rc;
> >> @@ -1285,6 +1293,34 @@ static int dp_display_remove(struct platform_device *pdev)
> >>
> >>          platform_set_drvdata(pdev, NULL);
> >>
> >> +       dp_display_deinit_sub_modules(dp);
> > There is already a call to dp_display_deinit_sub_modules() in
> > dp_display_remove().
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int dp_pm_runtime_suspend(struct device *dev)
> >> +{
> >> +       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);
> >> +       }
> > I don't see where this code was removed.
>
> this code block is adapted from dp_pm_suspend().
>
> dp_pm_suspend() will be removed at "add
> pm_runtime_force_suspend()/resume()" patch.

Each patch should be atomic. Adding code here, while removing it
later, breaks that assumption.

>
> >
> >> +       dp_display_host_deinit(dp);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int dp_pm_runtime_resume(struct device *dev)
> >> +{
> >> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >> +
> >> +       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;
> >>   }
> >>
> >> @@ -1389,6 +1425,7 @@ static int dp_pm_suspend(struct device *dev)
> >>   }
> >>
> >>   static const struct dev_pm_ops dp_pm_ops = {
> >> +       SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
> >>          .suspend = dp_pm_suspend,
> >>          .resume =  dp_pm_resume,
> >>   };
> >> @@ -1473,10 +1510,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.
> >> @@ -1578,6 +1611,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 start power\n");
> > failed to resume.
> >
> >> +               mutex_unlock(&dp_display->event_mutex);
> >> +               return;
> >> +       }
> >>
> >>          state = dp_display->hpd_state;
> >>          if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
> >> @@ -1642,10 +1680,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);
> >>
> >> @@ -1658,6 +1695,9 @@ 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_mark_last_busy(&dp_display->pdev->dev);
> >> +       pm_runtime_put_autosuspend(&dp_display->pdev->dev);
> >>          mutex_unlock(&dp_display->event_mutex);
> >>   }
> >>
> >> @@ -1697,6 +1737,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 */
> >> @@ -1718,6 +1764,9 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
> >>          dp_catalog_ctrl_hpd_disable(dp->catalog);
> >>
> >>          dp_display->internal_hpd = false;
> >> +
> >> +       pm_runtime_mark_last_busy(&dp->pdev->dev);
> >> +       pm_runtime_put_autosuspend(&dp->pdev->dev);
> >>          mutex_unlock(&dp->event_mutex);
> >>   }
> >>
> >> @@ -1732,13 +1781,11 @@ 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)
> >> +       /* hpd through gpio */
> > Lack of the comment might be better than the incorrect comment.
> >
> >> +       if (!dp_display->link_ready && status == connector_status_connected) {
> >> +               dp->hpd_state = ST_DISCONNECTED;
> > Is this also a part of pm_runtime support?
>
> yes, this is for hpd_internal == false case.
>
> The hpd_state need to be at init state.

Why do you have to add this line now, in this patch? What has changed
compared to the previous patch, which just added an event?

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



-- 
With best wishes
Dmitry

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

[Kuogee Hsieh]

On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote:
> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> After incorporated pm_runtime framework into eDP/DP driver, the
> incorporating
>
>
>> original dp_pm_suspend() to handle power off both DP phy and
>> controller during suspend and dp_pm_resume() to handle power on
>> both DP phy and controller during resume are not necessary since
>> those function are replaced by dp_pm_runtime_suspend() and
>> dp_pm_runtime_resume() through pm runtime framework.
>> Therefore add pm framework provides functions,
>> pm_runtime_force_suspend()/resume() to complete incorporating pm
>> runtime framework into DP driver.
>>
>> Changes in v4:
>> -- drop both dp_pm_prepare() and dp_pm_compete() from this change
>> -- delete ST_SUSPENDED state
>> -- rewording commit text to add more details regrading the purpose
>>     of this change
>>
>> Changes in v3:
>> -- replace dp_pm_suspend() with pm_runtime_force_suspend()
>> -- replace dp_pm_resume() with pm_runtime_force_resume()
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 113 ++----------------------------------
>>   1 file changed, 5 insertions(+), 108 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 9158a2c..711d262 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -49,7 +49,6 @@ enum {
>>          ST_CONNECTED,
>>          ST_DISCONNECT_PENDING,
>>          ST_DISPLAY_OFF,
>> -       ST_SUSPENDED,
>>   };
>>
>>   enum {
>> @@ -560,7 +559,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>>          drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
>>                          dp->dp_display.connector_type, state);
>>
>> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
>> +       if (state == ST_DISPLAY_OFF) {
>>                  mutex_unlock(&dp->event_mutex);
>>                  return 0;
>>          }
>> @@ -674,7 +673,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
>>          drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
>>                          dp->dp_display.connector_type, state);
>>
>> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
>> +       if (state == ST_DISPLAY_OFF) {
>>                  mutex_unlock(&dp->event_mutex);
>>                  return 0;
>>          }
>> @@ -1321,110 +1320,10 @@ static int dp_pm_runtime_resume(struct device *dev)
>>          return 0;
>>   }
>>
>> -static int dp_pm_resume(struct device *dev)
>> -{
>> -       struct platform_device *pdev = to_platform_device(dev);
>> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> -       struct dp_display_private *dp;
>> -       int sink_count = 0;
>> -
>> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
>> -
>> -       mutex_lock(&dp->event_mutex);
>> -
>> -       drm_dbg_dp(dp->drm_dev,
>> -               "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
>> -               dp->dp_display.connector_type, dp->core_initialized,
>> -               dp->phy_initialized, dp_display->power_on);
>> -
>> -       /* start from disconnected state */
>> -       dp->hpd_state = ST_DISCONNECTED;
>> -
>> -       /* turn on dp ctrl/phy */
>> -       dp_display_host_init(dp);
>> -
>> -       if (dp_display->is_edp)
>> -               dp_catalog_ctrl_hpd_enable(dp->catalog);
>> -
>> -       if (dp_catalog_link_is_connected(dp->catalog)) {
>> -               /*
>> -                * set sink to normal operation mode -- D0
>> -                * before dpcd read
>> -                */
>> -               dp_display_host_phy_init(dp);
>> -               dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>> -               sink_count = drm_dp_read_sink_count(dp->aux);
>> -               if (sink_count < 0)
>> -                       sink_count = 0;
>> -
>> -               dp_display_host_phy_exit(dp);
>> -       }
>> -
>> -       dp->link->sink_count = sink_count;
>> -       /*
>> -        * can not declared display is connected unless
>> -        * HDMI cable is plugged in and sink_count of
>> -        * dongle become 1
>> -        * also only signal audio when disconnected
>> -        */
>> -       if (dp->link->sink_count) {
>> -               dp->dp_display.link_ready = true;
>> -       } else {
>> -               dp->dp_display.link_ready = false;
>> -               dp_display_handle_plugged_change(dp_display, false);
>> -       }
>> -
>> -       drm_dbg_dp(dp->drm_dev,
>> -               "After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
>> -               dp->dp_display.connector_type, dp->link->sink_count,
>> -               dp->dp_display.link_ready, dp->core_initialized,
>> -               dp->phy_initialized, dp_display->power_on);
>> -
>> -       mutex_unlock(&dp->event_mutex);
>> -
>> -       return 0;
>> -}
>> -
>> -static int dp_pm_suspend(struct device *dev)
>> -{
>> -       struct platform_device *pdev = to_platform_device(dev);
>> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> -       struct dp_display_private *dp;
>> -
>> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
>> -
>> -       mutex_lock(&dp->event_mutex);
>> -
>> -       drm_dbg_dp(dp->drm_dev,
>> -               "Before, type=%d core_inited=%d  phy_inited=%d power_on=%d\n",
>> -               dp->dp_display.connector_type, dp->core_initialized,
>> -               dp->phy_initialized, dp_display->power_on);
>> -
>> -       /* mainlink enabled */
>> -       if (dp_power_clk_status(dp->power, DP_CTRL_PM))
>> -               dp_ctrl_off_link_stream(dp->ctrl);
>> -
>> -       dp_display_host_phy_exit(dp);
> I was under the impression that dp_pm_runtime_suspend / _resume
> functions perform phy init/exit only in eDP cases. Can we really drop
> the main suspend/resume functions?

yes on eDP case since it is embedded.

for external DP case, there are two steps

step 1: enable DP controller's  hpd block and start waiting for hpd 
interrupts at dp_display_hpd_enable()

step 2:  at plugin interrupts,  dp_display_host_phy_init()

step 3: at unplug interrupt: dp_bridge_atomic_post_disable() 
dp_display_host_phy_exi()

at runtime, there is loop between step 2 and step 3

step  4: disable DP controller's  hpd block

>
>> -
>> -       /* host_init will be called at pm_resume */
>> -       dp_display_host_deinit(dp);
>> -
>> -       dp->hpd_state = ST_SUSPENDED;
>> -
>> -       drm_dbg_dp(dp->drm_dev,
>> -               "After, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
>> -               dp->dp_display.connector_type, dp->core_initialized,
>> -               dp->phy_initialized, dp_display->power_on);
>> -
>> -       mutex_unlock(&dp->event_mutex);
>> -
>> -       return 0;
>> -}
>> -
>>   static const struct dev_pm_ops dp_pm_ops = {
>>          SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
>> -       .suspend = dp_pm_suspend,
>> -       .resume =  dp_pm_resume,
>> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +                                pm_runtime_force_resume)
>>   };
>>
>>   static struct platform_driver dp_display_driver = {
>> @@ -1658,9 +1557,6 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
>>
>>          dp_display = container_of(dp, struct dp_display_private, dp_display);
>>
>> -       if (dp->is_edp)
>> -               dp_hpd_unplug_handle(dp_display, 0);
> Why?

dp_hpd_unplug_handle() does not tear down phy.

Therefore eDP does not need to call unplug handle.



>> -
>>          mutex_lock(&dp_display->event_mutex);
>>
>>          state = dp_display->hpd_state;
>> @@ -1748,6 +1644,7 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>>          dp_catalog_ctrl_hpd_disable(dp->catalog);
>>
>>          dp_display->internal_hpd = false;
>> +       dp->hpd_state = ST_DISCONNECTED;
> Why? We have only disabled sending of the HPD events. The dongle might
> still be connected.

dp_bridge_hpd_disable() disable dp controller hpd block (no more hpd 
interrupt will be received).

dp_bridge_hpd_disable() should happen after DP main link had been teared 
down already.

Therefore hpd_state need to be in default state so that next plugin 
handle will be start with correct state.


>
>>          pm_runtime_mark_last_busy(&dp->pdev->dev);
>>          pm_runtime_put_autosuspend(&dp->pdev->dev);
>> --
>> 2.7.4
>>
>
> --
> With best wishes
>
> Dmitry

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

[Kuogee Hsieh]

On 9/27/2023 2:57 PM, Dmitry Baryshkov wrote:
> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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 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 | 69 ++++++++++++++++++-------------------
>>   2 files changed, 60 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 22eb774..425b5c5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -480,7 +480,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) {
>> @@ -488,12 +487,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);
>> @@ -508,6 +502,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);
> Ok, so here you have used put_sync instead of autosuspend. Can we have
> some uniformity? (I'd prefer to see put_sync or just put everywhere)
>
>> +
>> +       return ret;
>> +}
>> +
>>   struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>>                                bool is_edp)
>>   {
>> @@ -531,6 +540,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().
> eDP, AUX, generic_edp_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 711d262..9a2b403 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1203,6 +1203,28 @@ 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);
> drop.
are you mean dropping this line?
>> +
>> +       return rc;
>> +}
>> +
>> +static inline int dp_display_auxbus_population(struct dp_display_private *dp)
> It's not `population`. It is just `populate`.
>
> Also please inline this function.
Are you means moving this function to header file?
>
>
>> +{
>> +       int ret;
>> +
>> +       ret = devm_of_dp_aux_populate_bus(dp->aux, dp_auxbus_done_probe);
>> +       if (ret == -ENODEV)
>> +               DRM_ERROR("aux-bus not found\n");
>> +
>> +       return ret;
>> +}
>> +
>>   static int dp_display_probe(struct platform_device *pdev)
>>   {
>>          int rc = 0;
>> @@ -1271,10 +1293,16 @@ static int dp_display_probe(struct platform_device *pdev)
>>          if (rc)
>>                  return rc;
>>
>> -       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);
>> +       if (dp->dp_display.is_edp) {
>> +               rc = dp_display_auxbus_population(dp);
>> +               if (rc)
>> +                       DRM_ERROR("eDP auxbus population failed, rc=%d\n", rc);
>> +       } else {
>> +               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);
>> +               }
>>          }
>>
>>          return rc;
>> @@ -1285,8 +1313,6 @@ static int dp_display_remove(struct platform_device *pdev)
>>          struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
>>
>>          component_del(&pdev->dev, &dp_display_comp_ops);
>> -       dp_display_deinit_sub_modules(dp);
>> -
>>          platform_set_drvdata(pdev, NULL);
>>
>>          dp_display_deinit_sub_modules(dp);
>> @@ -1385,29 +1411,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
>> @@ -1420,17 +1425,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 v4 8/8] drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()

[Dmitry Baryshkov]

On Tue, 3 Oct 2023 at 20:16, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 9/27/2023 2:57 PM, Dmitry Baryshkov wrote:
> > On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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 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 | 69 ++++++++++++++++++-------------------
> >>   2 files changed, 60 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> index 22eb774..425b5c5 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> @@ -480,7 +480,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) {
> >> @@ -488,12 +487,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);
> >> @@ -508,6 +502,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);
> > Ok, so here you have used put_sync instead of autosuspend. Can we have
> > some uniformity? (I'd prefer to see put_sync or just put everywhere)
> >
> >> +
> >> +       return ret;
> >> +}
> >> +
> >>   struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
> >>                                bool is_edp)
> >>   {
> >> @@ -531,6 +540,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().
> > eDP, AUX, generic_edp_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 711d262..9a2b403 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1203,6 +1203,28 @@ 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);
> > drop.
> are you mean dropping this line?
> >> +
> >> +       return rc;
> >> +}
> >> +
> >> +static inline int dp_display_auxbus_population(struct dp_display_private *dp)
> > It's not `population`. It is just `populate`.
> >
> > Also please inline this function.
> Are you means moving this function to header file?

No. I mean inlining this function.

-- 
With best wishes
Dmitry

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

[Kuogee Hsieh]

On 9/27/2023 2:57 PM, Dmitry Baryshkov wrote:
> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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 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 | 69 ++++++++++++++++++-------------------
>>   2 files changed, 60 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 22eb774..425b5c5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -480,7 +480,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) {
>> @@ -488,12 +487,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);
>> @@ -508,6 +502,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);
> Ok, so here you have used put_sync instead of autosuspend. Can we have
> some uniformity? (I'd prefer to see put_sync or just put everywhere)


my point is,

since display is user interface,

if there has any inputs before timer expire then there is no reason to 
execute  pm_runtime_suspend().

otherwise pm_runtime_suspend() should be executed.

Therefore I used autosuspend  at aux_transfer() an 
ddp_bridge_atomic_post_disable().

here is not related to user interface so that i use put_sysn() directly.

is my point make sense?

  or should I drop all autosuspend and replace them with put_sync()?


>
>> +
>> +       return ret;
>> +}
>> +
>>   struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>>                                bool is_edp)
>>   {
>> @@ -531,6 +540,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().
> eDP, AUX, generic_edp_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 711d262..9a2b403 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1203,6 +1203,28 @@ 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);
> drop.
>
>> +
>> +       return rc;
>> +}
>> +
>> +static inline int dp_display_auxbus_population(struct dp_display_private *dp)
> It's not `population`. It is just `populate`.
>
> Also please inline this function.
>
>
>> +{
>> +       int ret;
>> +
>> +       ret = devm_of_dp_aux_populate_bus(dp->aux, dp_auxbus_done_probe);
>> +       if (ret == -ENODEV)
>> +               DRM_ERROR("aux-bus not found\n");
>> +
>> +       return ret;
>> +}
>> +
>>   static int dp_display_probe(struct platform_device *pdev)
>>   {
>>          int rc = 0;
>> @@ -1271,10 +1293,16 @@ static int dp_display_probe(struct platform_device *pdev)
>>          if (rc)
>>                  return rc;
>>
>> -       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);
>> +       if (dp->dp_display.is_edp) {
>> +               rc = dp_display_auxbus_population(dp);
>> +               if (rc)
>> +                       DRM_ERROR("eDP auxbus population failed, rc=%d\n", rc);
>> +       } else {
>> +               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);
>> +               }
>>          }
>>
>>          return rc;
>> @@ -1285,8 +1313,6 @@ static int dp_display_remove(struct platform_device *pdev)
>>          struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
>>
>>          component_del(&pdev->dev, &dp_display_comp_ops);
>> -       dp_display_deinit_sub_modules(dp);
>> -
>>          platform_set_drvdata(pdev, NULL);
>>
>>          dp_display_deinit_sub_modules(dp);
>> @@ -1385,29 +1411,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
>> @@ -1420,17 +1425,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 v4 7/8] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Dmitry Baryshkov]

On Tue, 3 Oct 2023 at 19:44, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote:
> > On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> After incorporated pm_runtime framework into eDP/DP driver, the
> > incorporating
> >
> >
> >> original dp_pm_suspend() to handle power off both DP phy and
> >> controller during suspend and dp_pm_resume() to handle power on
> >> both DP phy and controller during resume are not necessary since
> >> those function are replaced by dp_pm_runtime_suspend() and
> >> dp_pm_runtime_resume() through pm runtime framework.
> >> Therefore add pm framework provides functions,
> >> pm_runtime_force_suspend()/resume() to complete incorporating pm
> >> runtime framework into DP driver.
> >>
> >> Changes in v4:
> >> -- drop both dp_pm_prepare() and dp_pm_compete() from this change
> >> -- delete ST_SUSPENDED state
> >> -- rewording commit text to add more details regrading the purpose
> >>     of this change
> >>
> >> Changes in v3:
> >> -- replace dp_pm_suspend() with pm_runtime_force_suspend()
> >> -- replace dp_pm_resume() with pm_runtime_force_resume()
> >>
> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_display.c | 113 ++----------------------------------
> >>   1 file changed, 5 insertions(+), 108 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 9158a2c..711d262 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -49,7 +49,6 @@ enum {
> >>          ST_CONNECTED,
> >>          ST_DISCONNECT_PENDING,
> >>          ST_DISPLAY_OFF,
> >> -       ST_SUSPENDED,
> >>   };
> >>
> >>   enum {
> >> @@ -560,7 +559,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
> >>          drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
> >>                          dp->dp_display.connector_type, state);
> >>
> >> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
> >> +       if (state == ST_DISPLAY_OFF) {
> >>                  mutex_unlock(&dp->event_mutex);
> >>                  return 0;
> >>          }
> >> @@ -674,7 +673,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
> >>          drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
> >>                          dp->dp_display.connector_type, state);
> >>
> >> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
> >> +       if (state == ST_DISPLAY_OFF) {
> >>                  mutex_unlock(&dp->event_mutex);
> >>                  return 0;
> >>          }
> >> @@ -1321,110 +1320,10 @@ static int dp_pm_runtime_resume(struct device *dev)
> >>          return 0;
> >>   }
> >>
> >> -static int dp_pm_resume(struct device *dev)
> >> -{
> >> -       struct platform_device *pdev = to_platform_device(dev);
> >> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
> >> -       struct dp_display_private *dp;
> >> -       int sink_count = 0;
> >> -
> >> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
> >> -
> >> -       mutex_lock(&dp->event_mutex);
> >> -
> >> -       drm_dbg_dp(dp->drm_dev,
> >> -               "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
> >> -               dp->dp_display.connector_type, dp->core_initialized,
> >> -               dp->phy_initialized, dp_display->power_on);
> >> -
> >> -       /* start from disconnected state */
> >> -       dp->hpd_state = ST_DISCONNECTED;
> >> -
> >> -       /* turn on dp ctrl/phy */
> >> -       dp_display_host_init(dp);
> >> -
> >> -       if (dp_display->is_edp)
> >> -               dp_catalog_ctrl_hpd_enable(dp->catalog);
> >> -
> >> -       if (dp_catalog_link_is_connected(dp->catalog)) {
> >> -               /*
> >> -                * set sink to normal operation mode -- D0
> >> -                * before dpcd read
> >> -                */
> >> -               dp_display_host_phy_init(dp);
> >> -               dp_link_psm_config(dp->link, &dp->panel->link_info, false);
> >> -               sink_count = drm_dp_read_sink_count(dp->aux);
> >> -               if (sink_count < 0)
> >> -                       sink_count = 0;
> >> -
> >> -               dp_display_host_phy_exit(dp);
> >> -       }
> >> -
> >> -       dp->link->sink_count = sink_count;
> >> -       /*
> >> -        * can not declared display is connected unless
> >> -        * HDMI cable is plugged in and sink_count of
> >> -        * dongle become 1
> >> -        * also only signal audio when disconnected
> >> -        */
> >> -       if (dp->link->sink_count) {
> >> -               dp->dp_display.link_ready = true;
> >> -       } else {
> >> -               dp->dp_display.link_ready = false;
> >> -               dp_display_handle_plugged_change(dp_display, false);
> >> -       }
> >> -
> >> -       drm_dbg_dp(dp->drm_dev,
> >> -               "After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
> >> -               dp->dp_display.connector_type, dp->link->sink_count,
> >> -               dp->dp_display.link_ready, dp->core_initialized,
> >> -               dp->phy_initialized, dp_display->power_on);
> >> -
> >> -       mutex_unlock(&dp->event_mutex);
> >> -
> >> -       return 0;
> >> -}
> >> -
> >> -static int dp_pm_suspend(struct device *dev)
> >> -{
> >> -       struct platform_device *pdev = to_platform_device(dev);
> >> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
> >> -       struct dp_display_private *dp;
> >> -
> >> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
> >> -
> >> -       mutex_lock(&dp->event_mutex);
> >> -
> >> -       drm_dbg_dp(dp->drm_dev,
> >> -               "Before, type=%d core_inited=%d  phy_inited=%d power_on=%d\n",
> >> -               dp->dp_display.connector_type, dp->core_initialized,
> >> -               dp->phy_initialized, dp_display->power_on);
> >> -
> >> -       /* mainlink enabled */
> >> -       if (dp_power_clk_status(dp->power, DP_CTRL_PM))
> >> -               dp_ctrl_off_link_stream(dp->ctrl);
> >> -
> >> -       dp_display_host_phy_exit(dp);
> > I was under the impression that dp_pm_runtime_suspend / _resume
> > functions perform phy init/exit only in eDP cases. Can we really drop
> > the main suspend/resume functions?
>
> yes on eDP case since it is embedded.

Let me ask the same question in a different way:

dp_pm_suspend() / dp_pm_resume() functions contain several calls to DP
functions. Why can we drop them now? Maybe they had to be dropped in
one of the previous patches, when you have added proper runtime PM
support?

Could you please confirm that after each patch the DP driver is
working, that there are no hidden dependencies between patches?

> for external DP case, there are two steps
>
> step 1: enable DP controller's  hpd block and start waiting for hpd
> interrupts at dp_display_hpd_enable()

Step 1 should be optional. DP should be functional even if the
.hpd_enable was not called. Have you tested this usecase?

>
> step 2:  at plugin interrupts,  dp_display_host_phy_init()
>
> step 3: at unplug interrupt: dp_bridge_atomic_post_disable()
> dp_display_host_phy_exi()
>
> at runtime, there is loop between step 2 and step 3
>
> step  4: disable DP controller's  hpd block
>
> >
> >> -
> >> -       /* host_init will be called at pm_resume */
> >> -       dp_display_host_deinit(dp);
> >> -
> >> -       dp->hpd_state = ST_SUSPENDED;
> >> -
> >> -       drm_dbg_dp(dp->drm_dev,
> >> -               "After, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
> >> -               dp->dp_display.connector_type, dp->core_initialized,
> >> -               dp->phy_initialized, dp_display->power_on);
> >> -
> >> -       mutex_unlock(&dp->event_mutex);
> >> -
> >> -       return 0;
> >> -}
> >> -
> >>   static const struct dev_pm_ops dp_pm_ops = {
> >>          SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
> >> -       .suspend = dp_pm_suspend,
> >> -       .resume =  dp_pm_resume,
> >> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >> +                                pm_runtime_force_resume)
> >>   };
> >>
> >>   static struct platform_driver dp_display_driver = {
> >> @@ -1658,9 +1557,6 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
> >>
> >>          dp_display = container_of(dp, struct dp_display_private, dp_display);
> >>
> >> -       if (dp->is_edp)
> >> -               dp_hpd_unplug_handle(dp_display, 0);
> > Why?
>
> dp_hpd_unplug_handle() does not tear down phy.
>
> Therefore eDP does not need to call unplug handle.

I don't fully understand your argument here. Could you please
describe, why this function call was necessary beforehand and what is
being changed now, so that it becomes unnecessary?

>
>
>
> >> -
> >>          mutex_lock(&dp_display->event_mutex);
> >>
> >>          state = dp_display->hpd_state;
> >> @@ -1748,6 +1644,7 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
> >>          dp_catalog_ctrl_hpd_disable(dp->catalog);
> >>
> >>          dp_display->internal_hpd = false;
> >> +       dp->hpd_state = ST_DISCONNECTED;
> > Why? We have only disabled sending of the HPD events. The dongle might
> > still be connected.
>
> dp_bridge_hpd_disable() disable dp controller hpd block (no more hpd
> interrupt will be received).
>
> dp_bridge_hpd_disable() should happen after DP main link had been teared
> down already.

No, this assumption is incorrect. hpd_disable can happen at any point
during runtime.
It merely disables HPD interrupt generation, it has nothing to do with
the DP block being enabled or not.

> Therefore hpd_state need to be in default state so that next plugin
> handle will be start with correct state.
>
>
> >
> >>          pm_runtime_mark_last_busy(&dp->pdev->dev);
> >>          pm_runtime_put_autosuspend(&dp->pdev->dev);
> >> --
> >> 2.7.4
> >>
> >
> > --
> > With best wishes
> >
> > Dmitry



-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 03/10/2023 20:25, Kuogee Hsieh wrote:
> 
> On 9/27/2023 2:57 PM, Dmitry Baryshkov wrote:
>> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 
>> 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 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 | 69 
>>> ++++++++++++++++++-------------------
>>>   2 files changed, 60 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> index 22eb774..425b5c5 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> @@ -480,7 +480,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) {
>>> @@ -488,12 +487,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);
>>> @@ -508,6 +502,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);
>> Ok, so here you have used put_sync instead of autosuspend. Can we have
>> some uniformity? (I'd prefer to see put_sync or just put everywhere)
> 
> 
> my point is,
> 
> since display is user interface,
> 
> if there has any inputs before timer expire then there is no reason to 
> execute  pm_runtime_suspend().
> 
> otherwise pm_runtime_suspend() should be executed.
> 
> Therefore I used autosuspend  at aux_transfer() an 
> ddp_bridge_atomic_post_disable().
> 
> here is not related to user interface so that i use put_sysn() directly.
> 
> is my point make sense?
> 
>   or should I drop all autosuspend and replace them with put_sync()?

This was my question from the beginning: what was the reason for using 
autosuspend? Did it bring any sensible improvement in the disable & 
reenable path?

> 
> 
>>
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog 
>>> *catalog,
>>>                                bool is_edp)
>>>   {
>>> @@ -531,6 +540,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().
>> eDP, AUX, generic_edp_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 711d262..9a2b403 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -1203,6 +1203,28 @@ 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);
>> drop.
>>
>>> +
>>> +       return rc;
>>> +}
>>> +
>>> +static inline int dp_display_auxbus_population(struct 
>>> dp_display_private *dp)
>> It's not `population`. It is just `populate`.
>>
>> Also please inline this function.
>>
>>
>>> +{
>>> +       int ret;
>>> +
>>> +       ret = devm_of_dp_aux_populate_bus(dp->aux, 
>>> dp_auxbus_done_probe);
>>> +       if (ret == -ENODEV)
>>> +               DRM_ERROR("aux-bus not found\n");
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   static int dp_display_probe(struct platform_device *pdev)
>>>   {
>>>          int rc = 0;
>>> @@ -1271,10 +1293,16 @@ static int dp_display_probe(struct 
>>> platform_device *pdev)
>>>          if (rc)
>>>                  return rc;
>>>
>>> -       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);
>>> +       if (dp->dp_display.is_edp) {
>>> +               rc = dp_display_auxbus_population(dp);
>>> +               if (rc)
>>> +                       DRM_ERROR("eDP auxbus population failed, 
>>> rc=%d\n", rc);
>>> +       } else {
>>> +               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);
>>> +               }
>>>          }
>>>
>>>          return rc;
>>> @@ -1285,8 +1313,6 @@ static int dp_display_remove(struct 
>>> platform_device *pdev)
>>>          struct dp_display_private *dp = 
>>> dev_get_dp_display_private(&pdev->dev);
>>>
>>>          component_del(&pdev->dev, &dp_display_comp_ops);
>>> -       dp_display_deinit_sub_modules(dp);
>>> -
>>>          platform_set_drvdata(pdev, NULL);
>>>
>>>          dp_display_deinit_sub_modules(dp);
>>> @@ -1385,29 +1411,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
>>> @@ -1420,17 +1425,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
>>>
>>

-- 
With best wishes
Dmitry

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

[Kuogee Hsieh]

On 10/3/2023 10:56 AM, Dmitry Baryshkov wrote:
> On 03/10/2023 20:25, Kuogee Hsieh wrote:
>>
>> On 9/27/2023 2:57 PM, Dmitry Baryshkov wrote:
>>> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh 
>>> <quic_khsieh@quicinc.com> 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 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 | 69 
>>>> ++++++++++++++++++-------------------
>>>>   2 files changed, 60 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> index 22eb774..425b5c5 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> @@ -480,7 +480,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) {
>>>> @@ -488,12 +487,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);
>>>> @@ -508,6 +502,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);
>>> Ok, so here you have used put_sync instead of autosuspend. Can we have
>>> some uniformity? (I'd prefer to see put_sync or just put everywhere)
>>
>>
>> my point is,
>>
>> since display is user interface,
>>
>> if there has any inputs before timer expire then there is no reason 
>> to execute  pm_runtime_suspend().
>>
>> otherwise pm_runtime_suspend() should be executed.
>>
>> Therefore I used autosuspend  at aux_transfer() an 
>> ddp_bridge_atomic_post_disable().
>>
>> here is not related to user interface so that i use put_sysn() directly.
>>
>> is my point make sense?
>>
>>   or should I drop all autosuspend and replace them with put_sync()?
>
> This was my question from the beginning: what was the reason for using 
> autosuspend? Did it bring any sensible improvement in the disable & 
> reenable path?

ok, i got your point.

1) I will use put_sync() at dp_bridge_atomic_dsiable() and 
dp_bridge_hpd_disable() instead of put_autosuspend().

2) keep pm_runtime_put_autosuspend() at dp_aux_transfer().

Is this good?

>
>>
>>
>>>
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>>   struct drm_dp_aux *dp_aux_get(struct device *dev, struct 
>>>> dp_catalog *catalog,
>>>>                                bool is_edp)
>>>>   {
>>>> @@ -531,6 +540,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().
>>> eDP, AUX, generic_edp_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 711d262..9a2b403 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -1203,6 +1203,28 @@ 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);
>>> drop.
>>>
>>>> +
>>>> +       return rc;
>>>> +}
>>>> +
>>>> +static inline int dp_display_auxbus_population(struct 
>>>> dp_display_private *dp)
>>> It's not `population`. It is just `populate`.
>>>
>>> Also please inline this function.
>>>
>>>
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       ret = devm_of_dp_aux_populate_bus(dp->aux, 
>>>> dp_auxbus_done_probe);
>>>> +       if (ret == -ENODEV)
>>>> +               DRM_ERROR("aux-bus not found\n");
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>>   static int dp_display_probe(struct platform_device *pdev)
>>>>   {
>>>>          int rc = 0;
>>>> @@ -1271,10 +1293,16 @@ static int dp_display_probe(struct 
>>>> platform_device *pdev)
>>>>          if (rc)
>>>>                  return rc;
>>>>
>>>> -       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);
>>>> +       if (dp->dp_display.is_edp) {
>>>> +               rc = dp_display_auxbus_population(dp);
>>>> +               if (rc)
>>>> +                       DRM_ERROR("eDP auxbus population failed, 
>>>> rc=%d\n", rc);
>>>> +       } else {
>>>> +               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);
>>>> +               }
>>>>          }
>>>>
>>>>          return rc;
>>>> @@ -1285,8 +1313,6 @@ static int dp_display_remove(struct 
>>>> platform_device *pdev)
>>>>          struct dp_display_private *dp = 
>>>> dev_get_dp_display_private(&pdev->dev);
>>>>
>>>>          component_del(&pdev->dev, &dp_display_comp_ops);
>>>> -       dp_display_deinit_sub_modules(dp);
>>>> -
>>>>          platform_set_drvdata(pdev, NULL);
>>>>
>>>>          dp_display_deinit_sub_modules(dp);
>>>> @@ -1385,29 +1411,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
>>>> @@ -1420,17 +1425,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 v4 8/8] drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()

[Dmitry Baryshkov]

On Tue, 3 Oct 2023 at 23:18, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 10/3/2023 10:56 AM, Dmitry Baryshkov wrote:
> > On 03/10/2023 20:25, Kuogee Hsieh wrote:
> >>
> >> On 9/27/2023 2:57 PM, Dmitry Baryshkov wrote:
> >>> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh
> >>> <quic_khsieh@quicinc.com> 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 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 | 69
> >>>> ++++++++++++++++++-------------------
> >>>>   2 files changed, 60 insertions(+), 43 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> index 22eb774..425b5c5 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> @@ -480,7 +480,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) {
> >>>> @@ -488,12 +487,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);
> >>>> @@ -508,6 +502,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);
> >>> Ok, so here you have used put_sync instead of autosuspend. Can we have
> >>> some uniformity? (I'd prefer to see put_sync or just put everywhere)
> >>
> >>
> >> my point is,
> >>
> >> since display is user interface,
> >>
> >> if there has any inputs before timer expire then there is no reason
> >> to execute  pm_runtime_suspend().
> >>
> >> otherwise pm_runtime_suspend() should be executed.
> >>
> >> Therefore I used autosuspend  at aux_transfer() an
> >> ddp_bridge_atomic_post_disable().
> >>
> >> here is not related to user interface so that i use put_sysn() directly.
> >>
> >> is my point make sense?
> >>
> >>   or should I drop all autosuspend and replace them with put_sync()?
> >
> > This was my question from the beginning: what was the reason for using
> > autosuspend? Did it bring any sensible improvement in the disable &
> > reenable path?
>
> ok, i got your point.
>
> 1) I will use put_sync() at dp_bridge_atomic_dsiable() and
> dp_bridge_hpd_disable() instead of put_autosuspend().
>
> 2) keep pm_runtime_put_autosuspend() at dp_aux_transfer().

Why? The panel driver should take care about keeping DP on between transfers.

>
> Is this good?
>
> >
> >>
> >>
> >>>
> >>>> +
> >>>> +       return ret;
> >>>> +}
> >>>> +
> >>>>   struct drm_dp_aux *dp_aux_get(struct device *dev, struct
> >>>> dp_catalog *catalog,
> >>>>                                bool is_edp)
> >>>>   {
> >>>> @@ -531,6 +540,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().
> >>> eDP, AUX, generic_edp_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 711d262..9a2b403 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -1203,6 +1203,28 @@ 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);
> >>> drop.
> >>>
> >>>> +
> >>>> +       return rc;
> >>>> +}
> >>>> +
> >>>> +static inline int dp_display_auxbus_population(struct
> >>>> dp_display_private *dp)
> >>> It's not `population`. It is just `populate`.
> >>>
> >>> Also please inline this function.
> >>>
> >>>
> >>>> +{
> >>>> +       int ret;
> >>>> +
> >>>> +       ret = devm_of_dp_aux_populate_bus(dp->aux,
> >>>> dp_auxbus_done_probe);
> >>>> +       if (ret == -ENODEV)
> >>>> +               DRM_ERROR("aux-bus not found\n");
> >>>> +
> >>>> +       return ret;
> >>>> +}
> >>>> +
> >>>>   static int dp_display_probe(struct platform_device *pdev)
> >>>>   {
> >>>>          int rc = 0;
> >>>> @@ -1271,10 +1293,16 @@ static int dp_display_probe(struct
> >>>> platform_device *pdev)
> >>>>          if (rc)
> >>>>                  return rc;
> >>>>
> >>>> -       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);
> >>>> +       if (dp->dp_display.is_edp) {
> >>>> +               rc = dp_display_auxbus_population(dp);
> >>>> +               if (rc)
> >>>> +                       DRM_ERROR("eDP auxbus population failed,
> >>>> rc=%d\n", rc);
> >>>> +       } else {
> >>>> +               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);
> >>>> +               }
> >>>>          }
> >>>>
> >>>>          return rc;
> >>>> @@ -1285,8 +1313,6 @@ static int dp_display_remove(struct
> >>>> platform_device *pdev)
> >>>>          struct dp_display_private *dp =
> >>>> dev_get_dp_display_private(&pdev->dev);
> >>>>
> >>>>          component_del(&pdev->dev, &dp_display_comp_ops);
> >>>> -       dp_display_deinit_sub_modules(dp);
> >>>> -
> >>>>          platform_set_drvdata(pdev, NULL);
> >>>>
> >>>>          dp_display_deinit_sub_modules(dp);
> >>>> @@ -1385,29 +1411,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
> >>>> @@ -1420,17 +1425,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
> >>>>
> >>>
> >



-- 
With best wishes
Dmitry

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

[Kuogee Hsieh]

On 10/3/2023 10:53 AM, Dmitry Baryshkov wrote:
> On Tue, 3 Oct 2023 at 19:44, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote:
>>> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>> After incorporated pm_runtime framework into eDP/DP driver, the
>>> incorporating
>>>
>>>
>>>> original dp_pm_suspend() to handle power off both DP phy and
>>>> controller during suspend and dp_pm_resume() to handle power on
>>>> both DP phy and controller during resume are not necessary since
>>>> those function are replaced by dp_pm_runtime_suspend() and
>>>> dp_pm_runtime_resume() through pm runtime framework.
>>>> Therefore add pm framework provides functions,
>>>> pm_runtime_force_suspend()/resume() to complete incorporating pm
>>>> runtime framework into DP driver.
>>>>
>>>> Changes in v4:
>>>> -- drop both dp_pm_prepare() and dp_pm_compete() from this change
>>>> -- delete ST_SUSPENDED state
>>>> -- rewording commit text to add more details regrading the purpose
>>>>      of this change
>>>>
>>>> Changes in v3:
>>>> -- replace dp_pm_suspend() with pm_runtime_force_suspend()
>>>> -- replace dp_pm_resume() with pm_runtime_force_resume()
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/dp/dp_display.c | 113 ++----------------------------------
>>>>    1 file changed, 5 insertions(+), 108 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index 9158a2c..711d262 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -49,7 +49,6 @@ enum {
>>>>           ST_CONNECTED,
>>>>           ST_DISCONNECT_PENDING,
>>>>           ST_DISPLAY_OFF,
>>>> -       ST_SUSPENDED,
>>>>    };
>>>>
>>>>    enum {
>>>> @@ -560,7 +559,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>>>>           drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
>>>>                           dp->dp_display.connector_type, state);
>>>>
>>>> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
>>>> +       if (state == ST_DISPLAY_OFF) {
>>>>                   mutex_unlock(&dp->event_mutex);
>>>>                   return 0;
>>>>           }
>>>> @@ -674,7 +673,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
>>>>           drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
>>>>                           dp->dp_display.connector_type, state);
>>>>
>>>> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
>>>> +       if (state == ST_DISPLAY_OFF) {
>>>>                   mutex_unlock(&dp->event_mutex);
>>>>                   return 0;
>>>>           }
>>>> @@ -1321,110 +1320,10 @@ static int dp_pm_runtime_resume(struct device *dev)
>>>>           return 0;
>>>>    }
>>>>
>>>> -static int dp_pm_resume(struct device *dev)
>>>> -{
>>>> -       struct platform_device *pdev = to_platform_device(dev);
>>>> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>>> -       struct dp_display_private *dp;
>>>> -       int sink_count = 0;
>>>> -
>>>> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
>>>> -
>>>> -       mutex_lock(&dp->event_mutex);
>>>> -
>>>> -       drm_dbg_dp(dp->drm_dev,
>>>> -               "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
>>>> -               dp->dp_display.connector_type, dp->core_initialized,
>>>> -               dp->phy_initialized, dp_display->power_on);
>>>> -
>>>> -       /* start from disconnected state */
>>>> -       dp->hpd_state = ST_DISCONNECTED;
>>>> -
>>>> -       /* turn on dp ctrl/phy */
>>>> -       dp_display_host_init(dp);
>>>> -
>>>> -       if (dp_display->is_edp)
>>>> -               dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>> -
>>>> -       if (dp_catalog_link_is_connected(dp->catalog)) {
>>>> -               /*
>>>> -                * set sink to normal operation mode -- D0
>>>> -                * before dpcd read
>>>> -                */
>>>> -               dp_display_host_phy_init(dp);
>>>> -               dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>>>> -               sink_count = drm_dp_read_sink_count(dp->aux);
>>>> -               if (sink_count < 0)
>>>> -                       sink_count = 0;
>>>> -
>>>> -               dp_display_host_phy_exit(dp);
>>>> -       }
>>>> -
>>>> -       dp->link->sink_count = sink_count;
>>>> -       /*
>>>> -        * can not declared display is connected unless
>>>> -        * HDMI cable is plugged in and sink_count of
>>>> -        * dongle become 1
>>>> -        * also only signal audio when disconnected
>>>> -        */
>>>> -       if (dp->link->sink_count) {
>>>> -               dp->dp_display.link_ready = true;
>>>> -       } else {
>>>> -               dp->dp_display.link_ready = false;
>>>> -               dp_display_handle_plugged_change(dp_display, false);
>>>> -       }
>>>> -
>>>> -       drm_dbg_dp(dp->drm_dev,
>>>> -               "After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
>>>> -               dp->dp_display.connector_type, dp->link->sink_count,
>>>> -               dp->dp_display.link_ready, dp->core_initialized,
>>>> -               dp->phy_initialized, dp_display->power_on);
>>>> -
>>>> -       mutex_unlock(&dp->event_mutex);
>>>> -
>>>> -       return 0;
>>>> -}
>>>> -
>>>> -static int dp_pm_suspend(struct device *dev)
>>>> -{
>>>> -       struct platform_device *pdev = to_platform_device(dev);
>>>> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>>> -       struct dp_display_private *dp;
>>>> -
>>>> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
>>>> -
>>>> -       mutex_lock(&dp->event_mutex);
>>>> -
>>>> -       drm_dbg_dp(dp->drm_dev,
>>>> -               "Before, type=%d core_inited=%d  phy_inited=%d power_on=%d\n",
>>>> -               dp->dp_display.connector_type, dp->core_initialized,
>>>> -               dp->phy_initialized, dp_display->power_on);
>>>> -
>>>> -       /* mainlink enabled */
>>>> -       if (dp_power_clk_status(dp->power, DP_CTRL_PM))
>>>> -               dp_ctrl_off_link_stream(dp->ctrl);
>>>> -
>>>> -       dp_display_host_phy_exit(dp);
>>> I was under the impression that dp_pm_runtime_suspend / _resume
>>> functions perform phy init/exit only in eDP cases. Can we really drop
>>> the main suspend/resume functions?
>> yes on eDP case since it is embedded.
> Let me ask the same question in a different way:
>
> dp_pm_suspend() / dp_pm_resume() functions contain several calls to DP
> functions. Why can we drop them now? Maybe they had to be dropped in
> one of the previous patches, when you have added proper runtime PM
> support?
>
> Could you please confirm that after each patch the DP driver is
> working, that there are no hidden dependencies between patches?

patch #5 ==>  drm/msm/dp: incorporate pm_runtime framework into DP driver

patch #6 ==> drm/msm/dp: delete EV_HPD_INIT_SETUP

patch #7 ==>  drm/msm/dp: add pm_runtime_force_suspend()/resume() <== 
both dp_pm_suspend() and dp_pm_resume() are dropped here


Patch #5 is this patch and dp_pm_suspend() and dp_pm_resume() still kept.

patch #7 drop both dp_pm_suspend() and dp_pm_resume().

In order to keep every patch work for  suspend/resume test, I drop 
dp_pm_susend() and dp_pm_resuem() at patch #7.

yes, i confirm each patch DP driver is working.


>
>> for external DP case, there are two steps
>>
>> step 1: enable DP controller's  hpd block and start waiting for hpd
>> interrupts at dp_display_hpd_enable()
The step number I mentioned here is for hpd_internal == true case.
> Step 1 should be optional. DP should be functional even if the
> .hpd_enable was not called. Have you tested this usecase?

yes, for hpd_internal == false, step #1 is not required.

however I do not have device to test it.

But i think it should work since  pm_runtime_resume_and_get() and 
pm_runtime_put_sync() to dp_hpd_plug_handle() and dp_hpd_unplug_handle() 
respectively.




>> step 2:  at plugin interrupts,  dp_display_host_phy_init()
>>
>> step 3: at unplug interrupt: dp_bridge_atomic_post_disable()
>> dp_display_host_phy_exi()
>>
>> at runtime, there is loop between step 2 and step 3
>>
>> step  4: disable DP controller's  hpd block
>>
>>>> -
>>>> -       /* host_init will be called at pm_resume */
>>>> -       dp_display_host_deinit(dp);
>>>> -
>>>> -       dp->hpd_state = ST_SUSPENDED;
>>>> -
>>>> -       drm_dbg_dp(dp->drm_dev,
>>>> -               "After, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
>>>> -               dp->dp_display.connector_type, dp->core_initialized,
>>>> -               dp->phy_initialized, dp_display->power_on);
>>>> -
>>>> -       mutex_unlock(&dp->event_mutex);
>>>> -
>>>> -       return 0;
>>>> -}
>>>> -
>>>>    static const struct dev_pm_ops dp_pm_ops = {
>>>>           SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
>>>> -       .suspend = dp_pm_suspend,
>>>> -       .resume =  dp_pm_resume,
>>>> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>> +                                pm_runtime_force_resume)
>>>>    };
>>>>
>>>>    static struct platform_driver dp_display_driver = {
>>>> @@ -1658,9 +1557,6 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
>>>>
>>>>           dp_display = container_of(dp, struct dp_display_private, dp_display);
>>>>
>>>> -       if (dp->is_edp)
>>>> -               dp_hpd_unplug_handle(dp_display, 0);
>>> Why?
>> dp_hpd_unplug_handle() does not tear down phy.
>>
>> Therefore eDP does not need to call unplug handle.
> I don't fully understand your argument here. Could you please
> describe, why this function call was necessary beforehand and what is
> being changed now, so that it becomes unnecessary?

dp_hpd_unplug_handle() is not necessary for eDP from very beginning 
since dp_bridge_atomic_enable() do it all (tear down link and phy).

I think it was added long time ago mistakenly  just like to be 
compatible with external DP since external DP always have 
dp_hpd_unplug_handle() called from irq_handle().

i can restore it back if you insist it.


>>
>>
>>>> -
>>>>           mutex_lock(&dp_display->event_mutex);
>>>>
>>>>           state = dp_display->hpd_state;
>>>> @@ -1748,6 +1644,7 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>>>>           dp_catalog_ctrl_hpd_disable(dp->catalog);
>>>>
>>>>           dp_display->internal_hpd = false;
>>>> +       dp->hpd_state = ST_DISCONNECTED;
>>> Why? We have only disabled sending of the HPD events. The dongle might
>>> still be connected.
>> dp_bridge_hpd_disable() disable dp controller hpd block (no more hpd
>> interrupt will be received).
>>
>> dp_bridge_hpd_disable() should happen after DP main link had been teared
>> down already.
> No, this assumption is incorrect. hpd_disable can happen at any point
> during runtime.
> It merely disables HPD interrupt generation, it has nothing to do with
> the DP block being enabled or not.
>
>> Therefore hpd_state need to be in default state so that next plugin
>> handle will be start with correct state.
>>
>>
>>>>           pm_runtime_mark_last_busy(&dp->pdev->dev);
>>>>           pm_runtime_put_autosuspend(&dp->pdev->dev);
>>>> --
>>>> 2.7.4
>>>>
>>> --
>>> With best wishes
>>>
>>> Dmitry
>
>

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

[Dmitry Baryshkov]

On Wed, 4 Oct 2023 at 01:12, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 10/3/2023 10:53 AM, Dmitry Baryshkov wrote:
> > On Tue, 3 Oct 2023 at 19:44, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>
> >> On 9/27/2023 3:00 PM, Dmitry Baryshkov wrote:
> >>> On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>>> After incorporated pm_runtime framework into eDP/DP driver, the
> >>> incorporating
> >>>
> >>>
> >>>> original dp_pm_suspend() to handle power off both DP phy and
> >>>> controller during suspend and dp_pm_resume() to handle power on
> >>>> both DP phy and controller during resume are not necessary since
> >>>> those function are replaced by dp_pm_runtime_suspend() and
> >>>> dp_pm_runtime_resume() through pm runtime framework.
> >>>> Therefore add pm framework provides functions,
> >>>> pm_runtime_force_suspend()/resume() to complete incorporating pm
> >>>> runtime framework into DP driver.
> >>>>
> >>>> Changes in v4:
> >>>> -- drop both dp_pm_prepare() and dp_pm_compete() from this change
> >>>> -- delete ST_SUSPENDED state
> >>>> -- rewording commit text to add more details regrading the purpose
> >>>>      of this change
> >>>>
> >>>> Changes in v3:
> >>>> -- replace dp_pm_suspend() with pm_runtime_force_suspend()
> >>>> -- replace dp_pm_resume() with pm_runtime_force_resume()
> >>>>
> >>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/dp/dp_display.c | 113 ++----------------------------------
> >>>>    1 file changed, 5 insertions(+), 108 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index 9158a2c..711d262 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -49,7 +49,6 @@ enum {
> >>>>           ST_CONNECTED,
> >>>>           ST_DISCONNECT_PENDING,
> >>>>           ST_DISPLAY_OFF,
> >>>> -       ST_SUSPENDED,
> >>>>    };
> >>>>
> >>>>    enum {
> >>>> @@ -560,7 +559,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
> >>>>           drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
> >>>>                           dp->dp_display.connector_type, state);
> >>>>
> >>>> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
> >>>> +       if (state == ST_DISPLAY_OFF) {
> >>>>                   mutex_unlock(&dp->event_mutex);
> >>>>                   return 0;
> >>>>           }
> >>>> @@ -674,7 +673,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
> >>>>           drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
> >>>>                           dp->dp_display.connector_type, state);
> >>>>
> >>>> -       if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
> >>>> +       if (state == ST_DISPLAY_OFF) {
> >>>>                   mutex_unlock(&dp->event_mutex);
> >>>>                   return 0;
> >>>>           }
> >>>> @@ -1321,110 +1320,10 @@ static int dp_pm_runtime_resume(struct device *dev)
> >>>>           return 0;
> >>>>    }
> >>>>
> >>>> -static int dp_pm_resume(struct device *dev)
> >>>> -{
> >>>> -       struct platform_device *pdev = to_platform_device(dev);
> >>>> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
> >>>> -       struct dp_display_private *dp;
> >>>> -       int sink_count = 0;
> >>>> -
> >>>> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
> >>>> -
> >>>> -       mutex_lock(&dp->event_mutex);
> >>>> -
> >>>> -       drm_dbg_dp(dp->drm_dev,
> >>>> -               "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
> >>>> -               dp->dp_display.connector_type, dp->core_initialized,
> >>>> -               dp->phy_initialized, dp_display->power_on);
> >>>> -
> >>>> -       /* start from disconnected state */
> >>>> -       dp->hpd_state = ST_DISCONNECTED;
> >>>> -
> >>>> -       /* turn on dp ctrl/phy */
> >>>> -       dp_display_host_init(dp);
> >>>> -
> >>>> -       if (dp_display->is_edp)
> >>>> -               dp_catalog_ctrl_hpd_enable(dp->catalog);
> >>>> -
> >>>> -       if (dp_catalog_link_is_connected(dp->catalog)) {
> >>>> -               /*
> >>>> -                * set sink to normal operation mode -- D0
> >>>> -                * before dpcd read
> >>>> -                */
> >>>> -               dp_display_host_phy_init(dp);
> >>>> -               dp_link_psm_config(dp->link, &dp->panel->link_info, false);
> >>>> -               sink_count = drm_dp_read_sink_count(dp->aux);
> >>>> -               if (sink_count < 0)
> >>>> -                       sink_count = 0;
> >>>> -
> >>>> -               dp_display_host_phy_exit(dp);
> >>>> -       }
> >>>> -
> >>>> -       dp->link->sink_count = sink_count;
> >>>> -       /*
> >>>> -        * can not declared display is connected unless
> >>>> -        * HDMI cable is plugged in and sink_count of
> >>>> -        * dongle become 1
> >>>> -        * also only signal audio when disconnected
> >>>> -        */
> >>>> -       if (dp->link->sink_count) {
> >>>> -               dp->dp_display.link_ready = true;
> >>>> -       } else {
> >>>> -               dp->dp_display.link_ready = false;
> >>>> -               dp_display_handle_plugged_change(dp_display, false);
> >>>> -       }
> >>>> -
> >>>> -       drm_dbg_dp(dp->drm_dev,
> >>>> -               "After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
> >>>> -               dp->dp_display.connector_type, dp->link->sink_count,
> >>>> -               dp->dp_display.link_ready, dp->core_initialized,
> >>>> -               dp->phy_initialized, dp_display->power_on);
> >>>> -
> >>>> -       mutex_unlock(&dp->event_mutex);
> >>>> -
> >>>> -       return 0;
> >>>> -}
> >>>> -
> >>>> -static int dp_pm_suspend(struct device *dev)
> >>>> -{
> >>>> -       struct platform_device *pdev = to_platform_device(dev);
> >>>> -       struct msm_dp *dp_display = platform_get_drvdata(pdev);
> >>>> -       struct dp_display_private *dp;
> >>>> -
> >>>> -       dp = container_of(dp_display, struct dp_display_private, dp_display);
> >>>> -
> >>>> -       mutex_lock(&dp->event_mutex);
> >>>> -
> >>>> -       drm_dbg_dp(dp->drm_dev,
> >>>> -               "Before, type=%d core_inited=%d  phy_inited=%d power_on=%d\n",
> >>>> -               dp->dp_display.connector_type, dp->core_initialized,
> >>>> -               dp->phy_initialized, dp_display->power_on);
> >>>> -
> >>>> -       /* mainlink enabled */
> >>>> -       if (dp_power_clk_status(dp->power, DP_CTRL_PM))
> >>>> -               dp_ctrl_off_link_stream(dp->ctrl);
> >>>> -
> >>>> -       dp_display_host_phy_exit(dp);
> >>> I was under the impression that dp_pm_runtime_suspend / _resume
> >>> functions perform phy init/exit only in eDP cases. Can we really drop
> >>> the main suspend/resume functions?
> >> yes on eDP case since it is embedded.
> > Let me ask the same question in a different way:
> >
> > dp_pm_suspend() / dp_pm_resume() functions contain several calls to DP
> > functions. Why can we drop them now? Maybe they had to be dropped in
> > one of the previous patches, when you have added proper runtime PM
> > support?
> >
> > Could you please confirm that after each patch the DP driver is
> > working, that there are no hidden dependencies between patches?
>
> patch #5 ==>  drm/msm/dp: incorporate pm_runtime framework into DP driver
>
> patch #6 ==> drm/msm/dp: delete EV_HPD_INIT_SETUP
>
> patch #7 ==>  drm/msm/dp: add pm_runtime_force_suspend()/resume() <==
> both dp_pm_suspend() and dp_pm_resume() are dropped here
>
>
> Patch #5 is this patch and dp_pm_suspend() and dp_pm_resume() still kept.
>
> patch #7 drop both dp_pm_suspend() and dp_pm_resume().

This makes me wonder if squashing #5 and #7 would have resulted in a
cleaner and easier to understand patch. This way we'd be able to
observer code moved from suspend/resume to runtime_suspend /
runtime_resume

>
> In order to keep every patch work for  suspend/resume test, I drop
> dp_pm_susend() and dp_pm_resuem() at patch #7.
>
> yes, i confirm each patch DP driver is working.

Ack, thank you.

>
>
> >
> >> for external DP case, there are two steps
> >>
> >> step 1: enable DP controller's  hpd block and start waiting for hpd
> >> interrupts at dp_display_hpd_enable()
> The step number I mentioned here is for hpd_internal == true case.
> > Step 1 should be optional. DP should be functional even if the
> > .hpd_enable was not called. Have you tested this usecase?
>
> yes, for hpd_internal == false, step #1 is not required.
>
> however I do not have device to test it.

You have. Just add a dp-connector with hpd-gpios to any of your
DP-enabled devices and change the pinctrl for hpd pin to use the gpio
function instead of HPD.

>
> But i think it should work since  pm_runtime_resume_and_get() and
> pm_runtime_put_sync() to dp_hpd_plug_handle() and dp_hpd_unplug_handle()
> respectively.

Ack.


> >> step 2:  at plugin interrupts,  dp_display_host_phy_init()
> >>
> >> step 3: at unplug interrupt: dp_bridge_atomic_post_disable()
> >> dp_display_host_phy_exi()
> >>
> >> at runtime, there is loop between step 2 and step 3
> >>
> >> step  4: disable DP controller's  hpd block
> >>
> >>>> -
> >>>> -       /* host_init will be called at pm_resume */
> >>>> -       dp_display_host_deinit(dp);
> >>>> -
> >>>> -       dp->hpd_state = ST_SUSPENDED;
> >>>> -
> >>>> -       drm_dbg_dp(dp->drm_dev,
> >>>> -               "After, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
> >>>> -               dp->dp_display.connector_type, dp->core_initialized,
> >>>> -               dp->phy_initialized, dp_display->power_on);
> >>>> -
> >>>> -       mutex_unlock(&dp->event_mutex);
> >>>> -
> >>>> -       return 0;
> >>>> -}
> >>>> -
> >>>>    static const struct dev_pm_ops dp_pm_ops = {
> >>>>           SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL)
> >>>> -       .suspend = dp_pm_suspend,
> >>>> -       .resume =  dp_pm_resume,
> >>>> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >>>> +                                pm_runtime_force_resume)
> >>>>    };
> >>>>
> >>>>    static struct platform_driver dp_display_driver = {
> >>>> @@ -1658,9 +1557,6 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
> >>>>
> >>>>           dp_display = container_of(dp, struct dp_display_private, dp_display);
> >>>>
> >>>> -       if (dp->is_edp)
> >>>> -               dp_hpd_unplug_handle(dp_display, 0);
> >>> Why?
> >> dp_hpd_unplug_handle() does not tear down phy.
> >>
> >> Therefore eDP does not need to call unplug handle.
> > I don't fully understand your argument here. Could you please
> > describe, why this function call was necessary beforehand and what is
> > being changed now, so that it becomes unnecessary?
>
> dp_hpd_unplug_handle() is not necessary for eDP from very beginning
> since dp_bridge_atomic_enable() do it all (tear down link and phy).

Excuse me? Where does dp_bridge_atomic_enable() tear down the link?

>
> I think it was added long time ago mistakenly  just like to be
> compatible with external DP since external DP always have
> dp_hpd_unplug_handle() called from irq_handle().
>
> i can restore it back if you insist it.

I insist on having clean documented patches. If function call was not
necessary beforehand, please drop it in the separate patch with proper
description of why and what.

>
>
> >>
> >>
> >>>> -
> >>>>           mutex_lock(&dp_display->event_mutex);
> >>>>
> >>>>           state = dp_display->hpd_state;
> >>>> @@ -1748,6 +1644,7 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
> >>>>           dp_catalog_ctrl_hpd_disable(dp->catalog);
> >>>>
> >>>>           dp_display->internal_hpd = false;
> >>>> +       dp->hpd_state = ST_DISCONNECTED;
> >>> Why? We have only disabled sending of the HPD events. The dongle might
> >>> still be connected.
> >> dp_bridge_hpd_disable() disable dp controller hpd block (no more hpd
> >> interrupt will be received).
> >>
> >> dp_bridge_hpd_disable() should happen after DP main link had been teared
> >> down already.
> > No, this assumption is incorrect. hpd_disable can happen at any point
> > during runtime.
> > It merely disables HPD interrupt generation, it has nothing to do with
> > the DP block being enabled or not.
> >
> >> Therefore hpd_state need to be in default state so that next plugin
> >> handle will be start with correct state.
> >>
> >>
> >>>>           pm_runtime_mark_last_busy(&dp->pdev->dev);
> >>>>           pm_runtime_put_autosuspend(&dp->pdev->dev);
> >>>> --
> >>>> 2.7.4
> >>>>
> >>> --
> >>> With best wishes
> >>>
> >>> Dmitry
> >
> >



-- 
With best wishes
Dmitry

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

[Kuogee Hsieh]

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

> ok, will restore it back.
>>
>>>>
>>>>>> -
>>>>>>            mutex_lock(&dp_display->event_mutex);
>>>>>>
>>>>>>            state = dp_display->hpd_state;
>>>>>> @@ -1748,6 +1644,7 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>>>>>>            dp_catalog_ctrl_hpd_disable(dp->catalog);
>>>>>>
>>>>>>            dp_display->internal_hpd = false;
>>>>>> +       dp->hpd_state = ST_DISCONNECTED;
>>>>> Why? We have only disabled sending of the HPD events. The dongle might
>>>>> still be connected.
>>>> dp_bridge_hpd_disable() disable dp controller hpd block (no more hpd
>>>> interrupt will be received).
>>>>
>>>> dp_bridge_hpd_disable() should happen after DP main link had been teared
>>>> down already.
>>> No, this assumption is incorrect. hpd_disable can happen at any point
>>> during runtime.
>>> It merely disables HPD interrupt generation, it has nothing to do with
>>> the DP block being enabled or not.
>>>
>>>> Therefore hpd_state need to be in default state so that next plugin
>>>> handle will be start with correct state.
>>>>
>>>>
>>>>>>            pm_runtime_mark_last_busy(&dp->pdev->dev);
>>>>>>            pm_runtime_put_autosuspend(&dp->pdev->dev);
>>>>>> --
>>>>>> 2.7.4
>>>>>>
>>>>> --
>>>>> With best wishes
>>>>>
>>>>> Dmitry
>>>
>
>