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

[PATCH v3 0/7] 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().

-- add v3 changes log

Kuogee Hsieh (7):
  drm/msm/dp: tie dp_display_irq_handler() with dp driver
  drm/msm/dp: replace is_connected with link_ready
  drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes
  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         |  30 +++
 drivers/gpu/drm/msm/dp/dp_display.c     | 348 ++++++++++++++------------------
 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       |   9 -
 drivers/gpu/drm/msm/msm_drv.h           |   5 -
 7 files changed, 185 insertions(+), 228 deletions(-)

-- 
2.7.4

[PATCH v3 1/7] 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 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 | 35 +++++++++++++----------------------
 drivers/gpu/drm/msm/dp/dp_display.h |  1 -
 2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 76f1395..c217430 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1193,30 +1193,23 @@ 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);
 	if (!dp->irq) {
-		DRM_ERROR("failed to get irq\n");
-		return -EINVAL;
+		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 +1280,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 +1546,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 v3 2/7] drm/msm/dp: replace is_connected with link_ready

[Kuogee Hsieh]

The is_connected flag is set to true after DP mainlink successfully
finish link training. Replace the is_connected flag with link_ready
flag to avoid confusing.

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 c217430..18d16c7 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;
@@ -1352,16 +1351,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);
@@ -1754,8 +1753,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 v3 3/7] drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes

[Kuogee Hsieh]

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

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 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 18d16c7..59f9d85 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 v3 4/7] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Kuogee Hsieh]

Currently DP driver is executed independent of PM runtime framework.
This lead DP driver incompatible with others. 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. Both EV_POWER_PM_GET and EV_POWER_PM_PUT events
are introduced to perform pm runtime control for the HPD GPIO routing to a
display-connector case.

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>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
 drivers/gpu/drm/msm/dp/dp_display.c | 114 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/msm/dp/dp_power.c   |   9 ---
 3 files changed, 90 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 8e3b677..8fa93c5 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -291,6 +291,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		return -EINVAL;
 	}
 
+	if (pm_runtime_resume_and_get(dp_aux->dev))
+		return  -EINVAL;
+
 	mutex_lock(&aux->mutex);
 	if (!aux->initted) {
 		ret = -EIO;
@@ -364,6 +367,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 59f9d85..e7af7f7 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -60,6 +60,8 @@ enum {
 	EV_IRQ_HPD_INT,
 	EV_HPD_UNPLUG_INT,
 	EV_USER_NOTIFICATION,
+	EV_POWER_PM_GET,
+	EV_POWER_PM_PUT,
 };
 
 #define EVENT_TIMEOUT	(HZ/10)	/* 100ms */
@@ -276,13 +278,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;
 	dp->aux->drm_dev = drm;
 	rc = dp_aux_register(dp->aux);
@@ -291,12 +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) {
 		DRM_ERROR("Audio registration Dp failed\n");
@@ -320,10 +309,6 @@ 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);
@@ -467,6 +452,18 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
 	dp->core_initialized = false;
 }
 
+static void dp_display_pm_get(struct dp_display_private *dp)
+{
+	if (pm_runtime_resume_and_get(&dp->pdev->dev))
+		DRM_ERROR("failed to start power\n");
+}
+
+static void dp_display_pm_put(struct dp_display_private *dp)
+{
+	pm_runtime_mark_last_busy(&dp->pdev->dev);
+	pm_runtime_put_autosuspend(&dp->pdev->dev);
+}
+
 static int dp_display_usbpd_configure_cb(struct device *dev)
 {
 	struct dp_display_private *dp = dev_get_dp_display_private(dev);
@@ -1096,7 +1093,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);
@@ -1111,6 +1107,12 @@ static int hpd_event_thread(void *data)
 			dp_display_send_hpd_notification(dp_priv,
 						todo->data);
 			break;
+		case EV_POWER_PM_GET:
+			dp_display_pm_get(dp_priv);
+			break;
+		case EV_POWER_PM_PUT:
+			dp_display_pm_put(dp_priv);
+			break;
 		default:
 			break;
 		}
@@ -1251,6 +1253,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);
@@ -1263,6 +1277,10 @@ static int dp_display_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dp->dp_display);
 
+	devm_pm_runtime_enable(&pdev->dev);
+	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 +1303,36 @@ static int dp_display_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
+	pm_runtime_put_sync_suspend(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	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 +1437,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 +1522,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 +1623,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) {
@@ -1658,6 +1708,8 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
 	}
 
 	drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
+
+	pm_runtime_put_sync(&dp_display->pdev->dev);
 	mutex_unlock(&dp_display->event_mutex);
 }
 
@@ -1697,6 +1749,9 @@ 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");
+
 	dp_catalog_ctrl_hpd_enable(dp->catalog);
 
 	/* enable HDP interrupts */
@@ -1718,6 +1773,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 +1790,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)
+	if (!dp_display->link_ready && status == connector_status_connected) {
+		dp_add_event(dp, EV_POWER_PM_GET, 0, 0);
 		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);
+		dp_add_event(dp, EV_POWER_PM_PUT, 0, 0);
+	}
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index 5cb84ca..ed2f62a 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -152,8 +152,6 @@ 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);
 }
 
@@ -162,8 +160,6 @@ 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)
@@ -173,11 +169,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 +181,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;
 }
 
-- 
2.7.4

[PATCH v3 5/7] drm/msm/dp: delete EV_HPD_INIT_SETUP

[Kuogee Hsieh]

EV_HPD_INIT_SETUP flag is used to trigger the initialization of external
DP host controller. Since external DP host controller initialization had
been incorporated into pm_runtime_resume(), this flag become obsolete.
Lets get rid of it.

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

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 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 aa8499d..71d0670 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -870,7 +870,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;
@@ -879,9 +878,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 e7af7f7..b6992202 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,
@@ -1092,8 +1091,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;
@@ -1469,19 +1466,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 e13a8cb..ff8be59 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -381,7 +381,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);
@@ -402,10 +401,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 v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Kuogee Hsieh]

Add pm_runtime_force_suspend()/resume() to complete incorporating pm
runtime framework into DP driver. Both dp_pm_prepare() and dp_pm_complete()
are added to set hpd_state to correct state. After resume, DP driver will
re training its main link after .hpd_enable() callback enabled HPD
interrupts and bring up display accordingly.

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 | 87 +++++--------------------------------
 1 file changed, 10 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index b6992202..b58cb02 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1333,101 +1333,35 @@ static int dp_pm_runtime_resume(struct device *dev)
 	return 0;
 }
 
-static int dp_pm_resume(struct device *dev)
+static void dp_pm_complete(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);
+	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 
 	mutex_lock(&dp->event_mutex);
 
 	drm_dbg_dp(dp->drm_dev,
-		"Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
+		"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);
+		dp->phy_initialized, dp->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)
+static int dp_pm_prepare(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);
+	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 
 	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;
@@ -1435,8 +1369,10 @@ 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,
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+                                pm_runtime_force_resume)
+	.prepare = dp_pm_prepare,
+	.complete = dp_pm_complete,
 };
 
 static struct platform_driver dp_display_driver = {
@@ -1670,9 +1606,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;
-- 
2.7.4

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

[Kuogee Hsieh]

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

Changes in 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     | 25 ++++++++++++
 drivers/gpu/drm/msm/dp/dp_display.c | 79 ++++++++++++++++++-------------------
 2 files changed, 64 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 8fa93c5..79f0c6e 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -507,6 +507,21 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
 	drm_dp_aux_unregister(dp_aux);
 }
 
+static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux,
+				 unsigned long wait_us)
+{
+	int ret;
+	struct dp_aux_private *aux;
+
+	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
+
+	pm_runtime_get_sync(aux->dev);
+	ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+	pm_runtime_put_sync(aux->dev);
+
+	return ret;
+}
+
 struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
 			      bool is_edp)
 {
@@ -530,6 +545,16 @@ 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.
+	 */
+	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 b58cb02..886fae5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -310,8 +310,6 @@ static void dp_display_unbind(struct device *dev, struct device *master,
 
 	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);
@@ -1217,6 +1215,31 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde
 	return NULL;
 }
 
+static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
+{
+	int rc;
+
+	rc = component_add(aux->dev, &dp_display_comp_ops);
+	if (rc)
+		DRM_ERROR("eDP component add failed, rc=%d\n", rc);
+
+	return rc;
+}
+
+static int dp_display_auxbus_population(struct dp_display_private *dp)
+{
+	struct device *dev = &dp->pdev->dev;
+	struct device_node *aux_bus;
+	int ret = 0;
+
+	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+	if (aux_bus)
+		ret = devm_of_dp_aux_populate_bus(dp->aux, dp_auxbus_done_probe);
+
+	return ret;
+}
+
 static int dp_display_probe(struct platform_device *pdev)
 {
 	int rc = 0;
@@ -1282,10 +1305,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;
@@ -1296,14 +1325,13 @@ 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);
 
-	pm_runtime_put_sync_suspend(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+	dp_display_deinit_sub_modules(dp);
+
 	return 0;
 }
 
@@ -1432,31 +1460,10 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 
 static int dp_display_get_next_bridge(struct msm_dp *dp)
 {
-	int rc;
+	int rc = 0;
 	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
@@ -1469,17 +1476,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 v3 1/7] drm/msm/dp: tie dp_display_irq_handler() with dp driver

[Dmitry Baryshkov]

On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> 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 v3:
> -- move calling dp_display_irq_handler() to probe

Was there a changelog for the previous reivions? What is the
difference between v1 and v2?

>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
>  drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>  2 files changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 76f1395..c217430 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1193,30 +1193,23 @@ 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);
>         if (!dp->irq) {

What is the point in this check?

> -               DRM_ERROR("failed to get irq\n");
> -               return -EINVAL;
> +               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 +1280,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;

This way the IRQ ends up being enabled in _probe. Are we ready to
handle it here? Is the DP device fully setup at this moment?

> +
>         rc = component_add(&pdev->dev, &dp_display_comp_ops);
>         if (rc) {
>                 DRM_ERROR("component add failed, rc=%d\n", rc);
> @@ -1549,12 +1546,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 v3 3/7] drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes

[Dmitry Baryshkov]

On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> 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>

Also see the comment below.

> ---
>  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 18d16c7..59f9d85 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)

Note, this part (resetting the video_test and setting of is_connected)
should be moved to the dp_bridge_hpd_notify() too. Please ignore this
comment if this is handled later in the series.

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


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Currently DP driver is executed independent of PM runtime framework.
> This lead DP driver incompatible with others. Incorporating pm runtime

Why is it incompatible? Which others are mentioned here?

> 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. Both EV_POWER_PM_GET and EV_POWER_PM_PUT events
> are introduced to perform pm runtime control for the HPD GPIO routing to a
> display-connector case.
>
> 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

Previous changelog?

>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
>  drivers/gpu/drm/msm/dp/dp_display.c | 114 +++++++++++++++++++++++++++---------
>  drivers/gpu/drm/msm/dp/dp_power.c   |   9 ---
>  3 files changed, 90 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 8e3b677..8fa93c5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -291,6 +291,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>                 return -EINVAL;
>         }
>
> +       if (pm_runtime_resume_and_get(dp_aux->dev))
> +               return  -EINVAL;

Please propagate error values instead of reinventing them.

> +
>         mutex_lock(&aux->mutex);
>         if (!aux->initted) {
>                 ret = -EIO;
> @@ -364,6 +367,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);

What is the reason for using autosuspend? Such design decisions should
be described in the commit message.

>
>         return ret;
>  }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 59f9d85..e7af7f7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -60,6 +60,8 @@ enum {
>         EV_IRQ_HPD_INT,
>         EV_HPD_UNPLUG_INT,
>         EV_USER_NOTIFICATION,
> +       EV_POWER_PM_GET,
> +       EV_POWER_PM_PUT,
>  };
>
>  #define EVENT_TIMEOUT  (HZ/10) /* 100ms */
> @@ -276,13 +278,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;
>         dp->aux->drm_dev = drm;
>         rc = dp_aux_register(dp->aux);
> @@ -291,12 +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) {
>                 DRM_ERROR("Audio registration Dp failed\n");
> @@ -320,10 +309,6 @@ 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);
> @@ -467,6 +452,18 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>         dp->core_initialized = false;
>  }
>
> +static void dp_display_pm_get(struct dp_display_private *dp)
> +{
> +       if (pm_runtime_resume_and_get(&dp->pdev->dev))
> +               DRM_ERROR("failed to start power\n");
> +}

Huge NAK here. This means that the error is completely ignored (other
than being dumped to the log). This is a short path to Sync error and
other kinds of reboot.

> +
> +static void dp_display_pm_put(struct dp_display_private *dp)
> +{
> +       pm_runtime_mark_last_busy(&dp->pdev->dev);
> +       pm_runtime_put_autosuspend(&dp->pdev->dev);
> +}
> +
>  static int dp_display_usbpd_configure_cb(struct device *dev)
>  {
>         struct dp_display_private *dp = dev_get_dp_display_private(dev);
> @@ -1096,7 +1093,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);
> @@ -1111,6 +1107,12 @@ static int hpd_event_thread(void *data)
>                         dp_display_send_hpd_notification(dp_priv,
>                                                 todo->data);
>                         break;
> +               case EV_POWER_PM_GET:
> +                       dp_display_pm_get(dp_priv);
> +                       break;
> +               case EV_POWER_PM_PUT:
> +                       dp_display_pm_put(dp_priv);
> +                       break;

No. runtime_get / runtime_put are not HPD events. They should be
executed directly from the place where the drivers needs the device to
be powered up.

>                 default:
>                         break;
>                 }
> @@ -1251,6 +1253,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;
> +       }

Why? This moves resource allocation to the probe function, which is
irrelevant to the pm_runtime code. If this is required, you can move
these changes to a separate patch.

> +
>         /* setup event q */
>         mutex_init(&dp->event_mutex);
>         init_waitqueue_head(&dp->event_q);
> @@ -1263,6 +1277,10 @@ static int dp_display_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, &dp->dp_display);
>
> +       devm_pm_runtime_enable(&pdev->dev);

error code handling?

> +       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 +1303,36 @@ static int dp_display_remove(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, NULL);
>
> +       pm_runtime_put_sync_suspend(&pdev->dev);

Why? Who is holding the pm count here?

> +       pm_runtime_dont_use_autosuspend(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);

Why do you need _disable if you have a devm_pm_runtime_enable()? Not
to mention that pm_runtime_disable_action() already has a call to
pm_runtime_dont_use_autosuspend()

> +
> +       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 +1437,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 +1522,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 +1623,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) {
> @@ -1658,6 +1708,8 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
>         }
>
>         drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
> +
> +       pm_runtime_put_sync(&dp_display->pdev->dev);

So, no autosuspend now?

Also, I think we can get an unbalanced runtime status, as there is no
guarantee that atomic_enable / atomic_disable will be paired. Please
correct me if I'm wrong.
And also there is a possible return earlier in this function. The
driver will leak the runtime status again.

>         mutex_unlock(&dp_display->event_mutex);
>  }
>
> @@ -1697,6 +1749,9 @@ 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");

Return?

> +
>         dp_catalog_ctrl_hpd_enable(dp->catalog);
>
>         /* enable HDP interrupts */
> @@ -1718,6 +1773,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 +1790,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)
> +       if (!dp_display->link_ready && status == connector_status_connected) {
> +               dp_add_event(dp, EV_POWER_PM_GET, 0, 0);

Why? What for?

>                 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);
> +               dp_add_event(dp, EV_POWER_PM_PUT, 0, 0);
> +       }
>  }
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index 5cb84ca..ed2f62a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -152,8 +152,6 @@ 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);
>  }
>
> @@ -162,8 +160,6 @@ 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)
> @@ -173,11 +169,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 +181,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;
>  }
>
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 5/7] drm/msm/dp: delete EV_HPD_INIT_SETUP

[Dmitry Baryshkov]

On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> 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 become obsolete.

became

> Lets get rid of it.

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

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

This is not a changelog of the patch. It is a short description of the
patch itself. Please describe changes.

>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  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 aa8499d..71d0670 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -870,7 +870,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;
> @@ -879,9 +878,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 e7af7f7..b6992202 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,
> @@ -1092,8 +1091,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;
> @@ -1469,19 +1466,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 e13a8cb..ff8be59 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -381,7 +381,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);
> @@ -402,10 +401,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
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Dmitry Baryshkov]

On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Add pm_runtime_force_suspend()/resume() to complete incorporating pm
> runtime framework into DP driver. Both dp_pm_prepare() and dp_pm_complete()
> are added to set hpd_state to correct state. After resume, DP driver will
> re training its main link after .hpd_enable() callback enabled HPD
> interrupts and bring up display accordingly.

How will it re-train the main link? What is the code path for that?

I think this is a misuse for prepare/complete callbacks, at least
judging from their documentation.

>
> 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 | 87 +++++--------------------------------
>  1 file changed, 10 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index b6992202..b58cb02 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1333,101 +1333,35 @@ static int dp_pm_runtime_resume(struct device *dev)
>         return 0;
>  }
>
> -static int dp_pm_resume(struct device *dev)
> +static void dp_pm_complete(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);
> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>
>         mutex_lock(&dp->event_mutex);
>
>         drm_dbg_dp(dp->drm_dev,
> -               "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
> +               "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);
> +               dp->phy_initialized, dp->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)
> +static int dp_pm_prepare(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);
> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>
>         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;
> @@ -1435,8 +1369,10 @@ 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,
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                                pm_runtime_force_resume)
> +       .prepare = dp_pm_prepare,
> +       .complete = dp_pm_complete,
>  };
>
>  static struct platform_driver dp_display_driver = {
> @@ -1670,9 +1606,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;
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Sat, 16 Sept 2023 at 00:39, 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 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     | 25 ++++++++++++
>  drivers/gpu/drm/msm/dp/dp_display.c | 79 ++++++++++++++++++-------------------
>  2 files changed, 64 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 8fa93c5..79f0c6e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -507,6 +507,21 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
>         drm_dp_aux_unregister(dp_aux);
>  }
>
> +static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux,
> +                                unsigned long wait_us)
> +{
> +       int ret;
> +       struct dp_aux_private *aux;
> +
> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> +
> +       pm_runtime_get_sync(aux->dev);
> +       ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> +       pm_runtime_put_sync(aux->dev);
> +
> +       return ret;
> +}
> +
>  struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>                               bool is_edp)
>  {
> @@ -530,6 +545,16 @@ 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.
> +        */

Usual comment: this describes what, but should be documenting why.

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

Then the relevant code should be removed before a call to drm_dp_aux_register().

> +       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 b58cb02..886fae5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -310,8 +310,6 @@ static void dp_display_unbind(struct device *dev, struct device *master,
>
>         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);
> @@ -1217,6 +1215,31 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde
>         return NULL;
>  }
>
> +static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
> +{
> +       int rc;
> +
> +       rc = component_add(aux->dev, &dp_display_comp_ops);
> +       if (rc)
> +               DRM_ERROR("eDP component add failed, rc=%d\n", rc);
> +
> +       return rc;
> +}
> +
> +static int dp_display_auxbus_population(struct dp_display_private *dp)
> +{
> +       struct device *dev = &dp->pdev->dev;
> +       struct device_node *aux_bus;
> +       int ret = 0;
> +
> +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");

device_node refcount leak.

But I think that the aux-bus existence check is incorrect here.
of_dp_aux_populate_bus() will check and return -ENODEV if there is no
aux-bus subnode.

And once you have dropped the aux_bus check, you can safely inline
this function.

> +
> +       if (aux_bus)
> +               ret = devm_of_dp_aux_populate_bus(dp->aux, dp_auxbus_done_probe);
> +
> +       return ret;
> +}
> +
>  static int dp_display_probe(struct platform_device *pdev)
>  {
>         int rc = 0;
> @@ -1282,10 +1305,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;
> @@ -1296,14 +1325,13 @@ 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);
>
> -       pm_runtime_put_sync_suspend(&pdev->dev);
>         pm_runtime_dont_use_autosuspend(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
>
> +       dp_display_deinit_sub_modules(dp);

These changes should be reasoned.

> +
>         return 0;
>  }
>
> @@ -1432,31 +1460,10 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
>
>  static int dp_display_get_next_bridge(struct msm_dp *dp)
>  {
> -       int rc;
> +       int rc = 0;

Is there a need to set it to 0?


>         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
> @@ -1469,17 +1476,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 v3 2/7] drm/msm/dp: replace is_connected with link_ready

[Dmitry Baryshkov]

On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> The is_connected flag is set to true after DP mainlink successfully
> finish link training. Replace the is_connected flag with link_ready

finishes.
Also this is not a replace, this patch renames the flag.

> flag to avoid confusing.

confusing what with what?

>
> 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 c217430..18d16c7 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;
> @@ -1352,16 +1351,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);
> @@ -1754,8 +1753,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 v3 4/7] drm/msm/dp: incorporate pm_runtime framework into DP driver

[kernel test robot]

Hi Kuogee,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes linus/master v6.6-rc1]
[cannot apply to drm-misc/drm-misc-next drm-tip/drm-tip next-20230915]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuogee-Hsieh/drm-msm-dp-tie-dp_display_irq_handler-with-dp-driver/20230916-054014
base:   git://anongit.freedesktop.org/drm/drm drm-next
patch link:    https://lore.kernel.org/r/1694813901-26952-5-git-send-email-quic_khsieh%40quicinc.com
patch subject: [PATCH v3 4/7] drm/msm/dp: incorporate pm_runtime framework into DP driver
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230916/202309161321.UeiYRcIs-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230916/202309161321.UeiYRcIs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309161321.UeiYRcIs-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/msm/dp/dp_power.c: In function 'dp_power_client_deinit':
>> drivers/gpu/drm/msm/dp/dp_power.c:160:34: warning: variable 'power' set but not used [-Wunused-but-set-variable]
     160 |         struct dp_power_private *power;
         |                                  ^~~~~
   drivers/gpu/drm/msm/dp/dp_power.c: In function 'dp_power_init':
   drivers/gpu/drm/msm/dp/dp_power.c:168:34: warning: variable 'power' set but not used [-Wunused-but-set-variable]
     168 |         struct dp_power_private *power = NULL;
         |                                  ^~~~~
   drivers/gpu/drm/msm/dp/dp_power.c: In function 'dp_power_deinit':
   drivers/gpu/drm/msm/dp/dp_power.c:179:34: warning: variable 'power' set but not used [-Wunused-but-set-variable]
     179 |         struct dp_power_private *power;
         |                                  ^~~~~


vim +/power +160 drivers/gpu/drm/msm/dp/dp_power.c

c943b4948b5848 Chandan Uddaraju 2020-08-27  157  
c943b4948b5848 Chandan Uddaraju 2020-08-27  158  void dp_power_client_deinit(struct dp_power *dp_power)
c943b4948b5848 Chandan Uddaraju 2020-08-27  159  {
c943b4948b5848 Chandan Uddaraju 2020-08-27 @160  	struct dp_power_private *power;
c943b4948b5848 Chandan Uddaraju 2020-08-27  161  
c943b4948b5848 Chandan Uddaraju 2020-08-27  162  	power = container_of(dp_power, struct dp_power_private, dp_power);
c943b4948b5848 Chandan Uddaraju 2020-08-27  163  }
c943b4948b5848 Chandan Uddaraju 2020-08-27  164  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Kuogee Hsieh]

On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> 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 v3:
>> -- move calling dp_display_irq_handler() to probe
> Was there a changelog for the previous reivions? What is the
> difference between v1 and v2?

Sorry, v2 is same as v3.

I submitted v2 first but found i forget to add change logs from review 
comments of v1.

Therefore i submit v3 to add changes logs which missing at v2.



>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
>>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>>   2 files changed, 13 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 76f1395..c217430 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1193,30 +1193,23 @@ 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);
>>          if (!dp->irq) {
> What is the point in this check?
>
>> -               DRM_ERROR("failed to get irq\n");
>> -               return -EINVAL;
>> +               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 +1280,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;
> This way the IRQ ends up being enabled in _probe. Are we ready to
> handle it here? Is the DP device fully setup at this moment?
>
>> +
>>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>          if (rc) {
>>                  DRM_ERROR("component add failed, rc=%d\n", rc);
>> @@ -1549,12 +1546,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>
>>          dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
>>
>> -       ret = dp_display_request_irq(dp_display);
>> -       if (ret) {
>> -               DRM_ERROR("request_irq failed, ret=%d\n", ret);
>> -               return ret;
>> -       }
>> -
>>          ret = dp_display_get_next_bridge(dp_display);
>>          if (ret)
>>                  return ret;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>> index 1e9415a..b3c08de 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -35,7 +35,6 @@ struct msm_dp {
>>   int dp_display_set_plugged_cb(struct msm_dp *dp_display,
>>                  hdmi_codec_plugged_cb fn, struct device *codec_dev);
>>   int dp_display_get_modes(struct msm_dp *dp_display);
>> -int dp_display_request_irq(struct msm_dp *dp_display);
>>   bool dp_display_check_video_test(struct msm_dp *dp_display);
>>   int dp_display_get_test_bpp(struct msm_dp *dp_display);
>>   void dp_display_signal_audio_start(struct msm_dp *dp_display);
>> --
>> 2.7.4
>>
>

Re: [PATCH v3 2/7] drm/msm/dp: replace is_connected with link_ready

[Kuogee Hsieh]

On 9/15/2023 6:51 PM, Dmitry Baryshkov wrote:
> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> The is_connected flag is set to true after DP mainlink successfully
>> finish link training. Replace the is_connected flag with link_ready
> finishes.
> Also this is not a replace, this patch renames the flag.
yes, it is rename.
>
>> flag to avoid confusing.
> confusing what with what?

we have ST_MAINLINK_RAEDY state which means mainlink had finished link 
training and ready for video.

Therefore I think link_ready is more meaningful than is_connected.


>
>> 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 c217430..18d16c7 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;
>> @@ -1352,16 +1351,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);
>> @@ -1754,8 +1753,8 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
>>                  return;
>>          }
>>
>> -       if (!dp_display->is_connected && status == connector_status_connected)
>> +       if (!dp_display->link_ready && status == connector_status_connected)
>>                  dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
>> -       else if (dp_display->is_connected && status == connector_status_disconnected)
>> +       else if (dp_display->link_ready && status == connector_status_disconnected)
>>                  dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
>>   }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>> index b3c08de..d65693e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -16,7 +16,7 @@ struct msm_dp {
>>          struct drm_bridge *bridge;
>>          struct drm_connector *connector;
>>          struct drm_bridge *next_bridge;
>> -       bool is_connected;
>> +       bool link_ready;
>>          bool audio_enabled;
>>          bool power_on;
>>          unsigned int connector_type;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
>> index 785d766..ee945ca 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>> @@ -24,10 +24,10 @@ static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge)
>>
>>          dp = to_dp_bridge(bridge)->dp_display;
>>
>> -       drm_dbg_dp(dp->drm_dev, "is_connected = %s\n",
>> -               (dp->is_connected) ? "true" : "false");
>> +       drm_dbg_dp(dp->drm_dev, "link_ready = %s\n",
>> +               (dp->link_ready) ? "true" : "false");
>>
>> -       return (dp->is_connected) ? connector_status_connected :
>> +       return (dp->link_ready) ? connector_status_connected :
>>                                          connector_status_disconnected;
>>   }
>>
>> @@ -40,8 +40,8 @@ static int dp_bridge_atomic_check(struct drm_bridge *bridge,
>>
>>          dp = to_dp_bridge(bridge)->dp_display;
>>
>> -       drm_dbg_dp(dp->drm_dev, "is_connected = %s\n",
>> -               (dp->is_connected) ? "true" : "false");
>> +       drm_dbg_dp(dp->drm_dev, "link_ready = %s\n",
>> +               (dp->link_ready) ? "true" : "false");
>>
>>          /*
>>           * There is no protection in the DRM framework to check if the display
>> @@ -55,7 +55,7 @@ static int dp_bridge_atomic_check(struct drm_bridge *bridge,
>>           * After that this piece of code can be removed.
>>           */
>>          if (bridge->ops & DRM_BRIDGE_OP_HPD)
>> -               return (dp->is_connected) ? 0 : -ENOTCONN;
>> +               return (dp->link_ready) ? 0 : -ENOTCONN;
>>
>>          return 0;
>>   }
>> @@ -78,7 +78,7 @@ static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *
>>          dp = to_dp_bridge(bridge)->dp_display;
>>
>>          /* pluggable case assumes EDID is read when HPD */
>> -       if (dp->is_connected) {
>> +       if (dp->link_ready) {
>>                  rc = dp_display_get_modes(dp);
>>                  if (rc <= 0) {
>>                          DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
>> --
>> 2.7.4
>>
>

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Kuogee Hsieh]

On 9/15/2023 6:21 PM, Dmitry Baryshkov wrote:
> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> Add pm_runtime_force_suspend()/resume() to complete incorporating pm
>> runtime framework into DP driver. Both dp_pm_prepare() and dp_pm_complete()
>> are added to set hpd_state to correct state. After resume, DP driver will
>> re training its main link after .hpd_enable() callback enabled HPD
>> interrupts and bring up display accordingly.
> How will it re-train the main link? What is the code path for that?

1) for edp, dp_bridge_atomic_enable(), called from framework, to start 
link training and bring up display.

2) for external DP, HPD_PLUG_INT will be generated to start link 
training and bring up display.

>
> I think this is a misuse for prepare/complete callbacks, at least
> judging from their documentation.

1) dp_pm_prepare() is called to make sure eDP/DP related power/clocks 
are off and set hpd_state  to ST_SUSPENDED and nothing else.

2) dp_pm_completed() is called to set hpd_state to ST_ST_DISCONNECTED 
(default state) and nothing else.

I think both are doing proper action.


>
>> 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 | 87 +++++--------------------------------
>>   1 file changed, 10 insertions(+), 77 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index b6992202..b58cb02 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1333,101 +1333,35 @@ static int dp_pm_runtime_resume(struct device *dev)
>>          return 0;
>>   }
>>
>> -static int dp_pm_resume(struct device *dev)
>> +static void dp_pm_complete(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);
>> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>
>>          mutex_lock(&dp->event_mutex);
>>
>>          drm_dbg_dp(dp->drm_dev,
>> -               "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
>> +               "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);
>> +               dp->phy_initialized, dp->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)
>> +static int dp_pm_prepare(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);
>> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>
>>          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;
>> @@ -1435,8 +1369,10 @@ 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,
>> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +                                pm_runtime_force_resume)
>> +       .prepare = dp_pm_prepare,
>> +       .complete = dp_pm_complete,
>>   };
>>
>>   static struct platform_driver dp_display_driver = {
>> @@ -1670,9 +1606,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;
>> --
>> 2.7.4
>>
>

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

[Kuogee Hsieh]

On 9/15/2023 5:41 PM, Dmitry Baryshkov wrote:
> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> 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>
>
> Also see the comment below.
>
>> ---
>>   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 18d16c7..59f9d85 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)
> Note, this part (resetting the video_test and setting of is_connected)
> should be moved to the dp_bridge_hpd_notify() too. Please ignore this
> comment if this is handled later in the series.

I think keep them here is better since eDP does not populate hpd_enable, 
hpd_disable and hpd_notify at edp_bridge_ops at drm_bridge_attach().

Keep them here will work for both eDP and DP.


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

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

[Dmitry Baryshkov]

On Mon, 18 Sept 2023 at 20:03, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
> > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> 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 v3:
> >> -- move calling dp_display_irq_handler() to probe
> > Was there a changelog for the previous reivions? What is the
> > difference between v1 and v2?
>
> Sorry, v2 is same as v3.
>
> I submitted v2 first but found i forget to add change logs from review
> comments of v1.
>
> Therefore i submit v3 to add changes logs which missing at v2.

This doesn't consist of a change. It should have been a RESEND or just
responding with the changelog. V3 means new revision.

>
>
>
> >
> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
> >>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
> >>   2 files changed, 13 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 76f1395..c217430 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1193,30 +1193,23 @@ 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);
> >>          if (!dp->irq) {
> > What is the point in this check?
> >
> >> -               DRM_ERROR("failed to get irq\n");
> >> -               return -EINVAL;
> >> +               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 +1280,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;
> > This way the IRQ ends up being enabled in _probe. Are we ready to
> > handle it here? Is the DP device fully setup at this moment?
> >
> >> +
> >>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
> >>          if (rc) {
> >>                  DRM_ERROR("component add failed, rc=%d\n", rc);
> >> @@ -1549,12 +1546,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 v3 2/7] drm/msm/dp: replace is_connected with link_ready

[Dmitry Baryshkov]

On Mon, 18 Sept 2023 at 20:09, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 9/15/2023 6:51 PM, Dmitry Baryshkov wrote:
> > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> The is_connected flag is set to true after DP mainlink successfully
> >> finish link training. Replace the is_connected flag with link_ready
> > finishes.
> > Also this is not a replace, this patch renames the flag.
> yes, it is rename.
> >
> >> flag to avoid confusing.
> > confusing what with what?
>
> we have ST_MAINLINK_RAEDY state which means mainlink had finished link
> training and ready for video.
>
> Therefore I think link_ready is more meaningful than is_connected.

Guess, all this should have been in the commit message.

>
>
> >
> >> 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 c217430..18d16c7 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;
> >> @@ -1352,16 +1351,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);
> >> @@ -1754,8 +1753,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 v3 3/7] drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes

[Dmitry Baryshkov]

On Mon, 18 Sept 2023 at 23:16, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 9/15/2023 5:41 PM, Dmitry Baryshkov wrote:
> > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> 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>
> >
> > Also see the comment below.
> >
> >> ---
> >>   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 18d16c7..59f9d85 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)
> > Note, this part (resetting the video_test and setting of is_connected)
> > should be moved to the dp_bridge_hpd_notify() too. Please ignore this
> > comment if this is handled later in the series.
>
> I think keep them here is better since eDP does not populate hpd_enable,
> hpd_disable and hpd_notify at edp_bridge_ops at drm_bridge_attach().
>
> Keep them here will work for both eDP and DP.

Having them here doesn't work for DP-with-external-bridges, which will
not use dp_display_send_hpd_notification.

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



-- 
With best wishes
Dmitry

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Dmitry Baryshkov]

On Mon, 18 Sept 2023 at 20:48, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 9/15/2023 6:21 PM, Dmitry Baryshkov wrote:
> > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> Add pm_runtime_force_suspend()/resume() to complete incorporating pm
> >> runtime framework into DP driver. Both dp_pm_prepare() and dp_pm_complete()
> >> are added to set hpd_state to correct state. After resume, DP driver will
> >> re training its main link after .hpd_enable() callback enabled HPD
> >> interrupts and bring up display accordingly.
> > How will it re-train the main link? What is the code path for that?
>
> 1) for edp, dp_bridge_atomic_enable(), called from framework, to start
> link training and bring up display.

And this path doesn't use .hpd_enable() which you have mentioned in
the commit message. Please don't try to shorten the commit message.
You see, I have had questions to several of them, which means that
they were not verbose enough.

>
> 2) for external DP, HPD_PLUG_INT will be generated to start link
> training and bring up display.

This should be hpd_notify, who starts link training, not some event.

>
> >
> > I think this is a misuse for prepare/complete callbacks, at least
> > judging from their documentation.
>
> 1) dp_pm_prepare() is called to make sure eDP/DP related power/clocks
> are off and set hpd_state  to ST_SUSPENDED and nothing else.
>
> 2) dp_pm_completed() is called to set hpd_state to ST_ST_DISCONNECTED
> (default state) and nothing else.
>
> I think both are doing proper action.

Have you read the prepare() / complete() documentation? Does your
usage follow the documented use case?

>
>
> >
> >> 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 | 87 +++++--------------------------------
> >>   1 file changed, 10 insertions(+), 77 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index b6992202..b58cb02 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1333,101 +1333,35 @@ static int dp_pm_runtime_resume(struct device *dev)
> >>          return 0;
> >>   }
> >>
> >> -static int dp_pm_resume(struct device *dev)
> >> +static void dp_pm_complete(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);
> >> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >>
> >>          mutex_lock(&dp->event_mutex);
> >>
> >>          drm_dbg_dp(dp->drm_dev,
> >> -               "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
> >> +               "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);
> >> +               dp->phy_initialized, dp->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)
> >> +static int dp_pm_prepare(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);
> >> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >>
> >>          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;
> >> @@ -1435,8 +1369,10 @@ 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,
> >> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >> +                                pm_runtime_force_resume)
> >> +       .prepare = dp_pm_prepare,
> >> +       .complete = dp_pm_complete,
> >>   };
> >>
> >>   static struct platform_driver dp_display_driver = {
> >> @@ -1670,9 +1606,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;
> >> --
> >> 2.7.4
> >>
> >



-- 
With best wishes
Dmitry

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Kuogee Hsieh]

On 9/19/2023 2:50 AM, Dmitry Baryshkov wrote:
> On Mon, 18 Sept 2023 at 20:48, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 9/15/2023 6:21 PM, Dmitry Baryshkov wrote:
>>> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>> Add pm_runtime_force_suspend()/resume() to complete incorporating pm
>>>> runtime framework into DP driver. Both dp_pm_prepare() and dp_pm_complete()
>>>> are added to set hpd_state to correct state. After resume, DP driver will
>>>> re training its main link after .hpd_enable() callback enabled HPD
>>>> interrupts and bring up display accordingly.
>>> How will it re-train the main link? What is the code path for that?
>> 1) for edp, dp_bridge_atomic_enable(), called from framework, to start
>> link training and bring up display.
> And this path doesn't use .hpd_enable() which you have mentioned in
> the commit message. Please don't try to shorten the commit message.
> You see, I have had questions to several of them, which means that
> they were not verbose enough.

ok, my bad,

I will add more explain to commit text.

>> 2) for external DP, HPD_PLUG_INT will be generated to start link
>> training and bring up display.
> This should be hpd_notify, who starts link training, not some event.
>
>>> I think this is a misuse for prepare/complete callbacks, at least
>>> judging from their documentation.
>> 1) dp_pm_prepare() is called to make sure eDP/DP related power/clocks
>> are off and set hpd_state  to ST_SUSPENDED and nothing else.
>>
>> 2) dp_pm_completed() is called to set hpd_state to ST_ST_DISCONNECTED
>> (default state) and nothing else.
>>
>> I think both are doing proper action.
> Have you read the prepare() / complete() documentation? Does your
> usage follow the documented use case?
I think I can just remove both dp_pm_prepare and dp_pm_complete fro 
this  patch.
>
>>
>>>> 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 | 87 +++++--------------------------------
>>>>    1 file changed, 10 insertions(+), 77 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index b6992202..b58cb02 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -1333,101 +1333,35 @@ static int dp_pm_runtime_resume(struct device *dev)
>>>>           return 0;
>>>>    }
>>>>
>>>> -static int dp_pm_resume(struct device *dev)
>>>> +static void dp_pm_complete(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);
>>>> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>>>
>>>>           mutex_lock(&dp->event_mutex);
>>>>
>>>>           drm_dbg_dp(dp->drm_dev,
>>>> -               "Before, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
>>>> +               "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);
>>>> +               dp->phy_initialized, dp->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)
>>>> +static int dp_pm_prepare(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);
>>>> +       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>>>
>>>>           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;
>>>> @@ -1435,8 +1369,10 @@ 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,
>>>> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>> +                                pm_runtime_force_resume)
>>>> +       .prepare = dp_pm_prepare,
>>>> +       .complete = dp_pm_complete,
>>>>    };
>>>>
>>>>    static struct platform_driver dp_display_driver = {
>>>> @@ -1670,9 +1606,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;
>>>> --
>>>> 2.7.4
>>>>
>
>

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

[Kuogee Hsieh]

On 9/15/2023 6:07 PM, Dmitry Baryshkov wrote:
> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> Currently DP driver is executed independent of PM runtime framework.
>> This lead DP driver incompatible with others. Incorporating pm runtime
> Why is it incompatible? Which others are mentioned here?
>
>> 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. Both EV_POWER_PM_GET and EV_POWER_PM_PUT events
>> are introduced to perform pm runtime control for the HPD GPIO routing to a
>> display-connector case.
>>
>> 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
> Previous changelog?
>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
>>   drivers/gpu/drm/msm/dp/dp_display.c | 114 +++++++++++++++++++++++++++---------
>>   drivers/gpu/drm/msm/dp/dp_power.c   |   9 ---
>>   3 files changed, 90 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 8e3b677..8fa93c5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -291,6 +291,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>>                  return -EINVAL;
>>          }
>>
>> +       if (pm_runtime_resume_and_get(dp_aux->dev))
>> +               return  -EINVAL;
> Please propagate error values instead of reinventing them.
>
>> +
>>          mutex_lock(&aux->mutex);
>>          if (!aux->initted) {
>>                  ret = -EIO;
>> @@ -364,6 +367,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);
> What is the reason for using autosuspend? Such design decisions should
> be described in the commit message.
>
>>          return ret;
>>   }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 59f9d85..e7af7f7 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -60,6 +60,8 @@ enum {
>>          EV_IRQ_HPD_INT,
>>          EV_HPD_UNPLUG_INT,
>>          EV_USER_NOTIFICATION,
>> +       EV_POWER_PM_GET,
>> +       EV_POWER_PM_PUT,
>>   };
>>
>>   #define EVENT_TIMEOUT  (HZ/10) /* 100ms */
>> @@ -276,13 +278,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;
>>          dp->aux->drm_dev = drm;
>>          rc = dp_aux_register(dp->aux);
>> @@ -291,12 +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) {
>>                  DRM_ERROR("Audio registration Dp failed\n");
>> @@ -320,10 +309,6 @@ 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);
>> @@ -467,6 +452,18 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>>          dp->core_initialized = false;
>>   }
>>
>> +static void dp_display_pm_get(struct dp_display_private *dp)
>> +{
>> +       if (pm_runtime_resume_and_get(&dp->pdev->dev))
>> +               DRM_ERROR("failed to start power\n");
>> +}
> Huge NAK here. This means that the error is completely ignored (other
> than being dumped to the log). This is a short path to Sync error and
> other kinds of reboot.
>
>> +
>> +static void dp_display_pm_put(struct dp_display_private *dp)
>> +{
>> +       pm_runtime_mark_last_busy(&dp->pdev->dev);
>> +       pm_runtime_put_autosuspend(&dp->pdev->dev);
>> +}
>> +
>>   static int dp_display_usbpd_configure_cb(struct device *dev)
>>   {
>>          struct dp_display_private *dp = dev_get_dp_display_private(dev);
>> @@ -1096,7 +1093,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);
>> @@ -1111,6 +1107,12 @@ static int hpd_event_thread(void *data)
>>                          dp_display_send_hpd_notification(dp_priv,
>>                                                  todo->data);
>>                          break;
>> +               case EV_POWER_PM_GET:
>> +                       dp_display_pm_get(dp_priv);
>> +                       break;
>> +               case EV_POWER_PM_PUT:
>> +                       dp_display_pm_put(dp_priv);
>> +                       break;
> No. runtime_get / runtime_put are not HPD events. They should be
> executed directly from the place where the drivers needs the device to
> be powered up.
>
>>                  default:
>>                          break;
>>                  }
>> @@ -1251,6 +1253,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;
>> +       }
> Why? This moves resource allocation to the probe function, which is
> irrelevant to the pm_runtime code. If this is required, you can move
> these changes to a separate patch.
>
>> +
>>          /* setup event q */
>>          mutex_init(&dp->event_mutex);
>>          init_waitqueue_head(&dp->event_q);
>> @@ -1263,6 +1277,10 @@ static int dp_display_probe(struct platform_device *pdev)
>>
>>          platform_set_drvdata(pdev, &dp->dp_display);
>>
>> +       devm_pm_runtime_enable(&pdev->dev);
> error code handling?
>
>> +       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 +1303,36 @@ static int dp_display_remove(struct platform_device *pdev)
>>
>>          platform_set_drvdata(pdev, NULL);
>>
>> +       pm_runtime_put_sync_suspend(&pdev->dev);
> Why? Who is holding the pm count here?
>
>> +       pm_runtime_dont_use_autosuspend(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
> Why do you need _disable if you have a devm_pm_runtime_enable()? Not
> to mention that pm_runtime_disable_action() already has a call to
> pm_runtime_dont_use_autosuspend()
>
>> +
>> +       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 +1437,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 +1522,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 +1623,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) {
>> @@ -1658,6 +1708,8 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
>>          }
>>
>>          drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
>> +
>> +       pm_runtime_put_sync(&dp_display->pdev->dev);
> So, no autosuspend now?
>
> Also, I think we can get an unbalanced runtime status, as there is no
> guarantee that atomic_enable / atomic_disable will be paired. Please
> correct me if I'm wrong.

  I always assume atomic_enable / atomic_disable should be paired.
Otherwise nothing will work.
Could you please give me example what situations they are not paired?

> And also there is a possible return earlier in this function. The
> driver will leak the runtime status again.
>
>>          mutex_unlock(&dp_display->event_mutex);
>>   }
>>
>> @@ -1697,6 +1749,9 @@ 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");
> Return?
>
>> +
>>          dp_catalog_ctrl_hpd_enable(dp->catalog);
>>
>>          /* enable HDP interrupts */
>> @@ -1718,6 +1773,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 +1790,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)
>> +       if (!dp_display->link_ready && status == connector_status_connected) {
>> +               dp_add_event(dp, EV_POWER_PM_GET, 0, 0);
> Why? What for?
>
>>                  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);
>> +               dp_add_event(dp, EV_POWER_PM_PUT, 0, 0);
>> +       }
>>   }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
>> index 5cb84ca..ed2f62a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>> @@ -152,8 +152,6 @@ 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);
>>   }
>>
>> @@ -162,8 +160,6 @@ 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)
>> @@ -173,11 +169,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 +181,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;
>>   }
>>
>> --
>> 2.7.4
>>
>

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Stephen Boyd]

Quoting Dmitry Baryshkov (2023-09-19 02:50:12)
> On Mon, 18 Sept 2023 at 20:48, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >
> >
> > On 9/15/2023 6:21 PM, Dmitry Baryshkov wrote:
> > > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> > >> Add pm_runtime_force_suspend()/resume() to complete incorporating pm
> > >> runtime framework into DP driver. Both dp_pm_prepare() and dp_pm_complete()
> > >> are added to set hpd_state to correct state. After resume, DP driver will
> > >> re training its main link after .hpd_enable() callback enabled HPD
> > >> interrupts and bring up display accordingly.
> > > How will it re-train the main link? What is the code path for that?
> >
> > 1) for edp, dp_bridge_atomic_enable(), called from framework, to start
> > link training and bring up display.
>
> And this path doesn't use .hpd_enable() which you have mentioned in
> the commit message. Please don't try to shorten the commit message.
> You see, I have had questions to several of them, which means that
> they were not verbose enough.
>
> >
> > 2) for external DP, HPD_PLUG_INT will be generated to start link
> > training and bring up display.
>
> This should be hpd_notify, who starts link training, not some event.

I think this driver should train the link during atomic_enable(), not
hpd_notify() (or directly from the irq handler). The drm_bridge_funcs
talk a bit about when the clocks and timing signals are supposed to be
enabled. For example, struct drm_bridge_funcs::atomic_pre_enable() says
the "display pipe (i.e.  clocks and timing signals) feeding this bridge
will not yet be running when this callback is called". And struct
drm_bridge_funcs::atomic_enable() says "this callback must enable the
display link feeding the next bridge in the chain if there is one."

That looks to me like link training, i.e. the display link, should
happen in the enable path and not hpd_notify. It looks like link
training could fail, but when that happens I believe the driver should
call drm_connector_set_link_status_property() with
DRM_MODE_LINK_STATUS_BAD. The two callers of that which exist in the
tree also call drm_kms_helper_hotplug_event() or
drm_kms_helper_connector_hotplug_event() after updating the link so that
userspace knows to try again.

It would be nice if there was some drm_bridge_set_link_status_bad() API
that bridge drivers could use to signal that the link status is bad and
call the hotplug helper. Maybe it could also record some diagnostics
about which bridge failed to setup the link and stop the atomic_enable()
chain for that connector.

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

[Kuogee Hsieh]

On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> 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 v3:
>> -- move calling dp_display_irq_handler() to probe
> Was there a changelog for the previous reivions? What is the
> difference between v1 and v2?
>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
>>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>>   2 files changed, 13 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 76f1395..c217430 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1193,30 +1193,23 @@ 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);
>>          if (!dp->irq) {
> What is the point in this check?
>
>> -               DRM_ERROR("failed to get irq\n");
>> -               return -EINVAL;
>> +               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 +1280,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;
> This way the IRQ ends up being enabled in _probe. Are we ready to
> handle it here? Is the DP device fully setup at this moment?

The irq is enabled here.

but DP driver hpd hardware block has not yet be enabled. this means no 
irq will be delivered.

  .hpd_enable() will call pm_runtime_resume_and_get() and 
dp_catalog_ctrl_hpd_enable().

after .hpd_enable() irq will be delivered and handled properly.



>> +
>>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>          if (rc) {
>>                  DRM_ERROR("component add failed, rc=%d\n", rc);
>> @@ -1549,12 +1546,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>
>>          dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
>>
>> -       ret = dp_display_request_irq(dp_display);
>> -       if (ret) {
>> -               DRM_ERROR("request_irq failed, ret=%d\n", ret);
>> -               return ret;
>> -       }
>> -
>>          ret = dp_display_get_next_bridge(dp_display);
>>          if (ret)
>>                  return ret;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>> index 1e9415a..b3c08de 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -35,7 +35,6 @@ struct msm_dp {
>>   int dp_display_set_plugged_cb(struct msm_dp *dp_display,
>>                  hdmi_codec_plugged_cb fn, struct device *codec_dev);
>>   int dp_display_get_modes(struct msm_dp *dp_display);
>> -int dp_display_request_irq(struct msm_dp *dp_display);
>>   bool dp_display_check_video_test(struct msm_dp *dp_display);
>>   int dp_display_get_test_bpp(struct msm_dp *dp_display);
>>   void dp_display_signal_audio_start(struct msm_dp *dp_display);
>> --
>> 2.7.4
>>
>

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Abhinav Kumar]

Hi Stephen

On 9/22/2023 2:54 PM, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2023-09-19 02:50:12)
>> On Mon, 18 Sept 2023 at 20:48, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>
>>>
>>> On 9/15/2023 6:21 PM, Dmitry Baryshkov wrote:
>>>> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>> Add pm_runtime_force_suspend()/resume() to complete incorporating pm
>>>>> runtime framework into DP driver. Both dp_pm_prepare() and dp_pm_complete()
>>>>> are added to set hpd_state to correct state. After resume, DP driver will
>>>>> re training its main link after .hpd_enable() callback enabled HPD
>>>>> interrupts and bring up display accordingly.
>>>> How will it re-train the main link? What is the code path for that?
>>>
>>> 1) for edp, dp_bridge_atomic_enable(), called from framework, to start
>>> link training and bring up display.
>>
>> And this path doesn't use .hpd_enable() which you have mentioned in
>> the commit message. Please don't try to shorten the commit message.
>> You see, I have had questions to several of them, which means that
>> they were not verbose enough.
>>
>>>
>>> 2) for external DP, HPD_PLUG_INT will be generated to start link
>>> training and bring up display.
>>
>> This should be hpd_notify, who starts link training, not some event.
> 
> I think this driver should train the link during atomic_enable(), not
> hpd_notify() (or directly from the irq handler). The drm_bridge_funcs
> talk a bit about when the clocks and timing signals are supposed to be
> enabled. For example, struct drm_bridge_funcs::atomic_pre_enable() says
> the "display pipe (i.e.  clocks and timing signals) feeding this bridge
> will not yet be running when this callback is called". And struct
> drm_bridge_funcs::atomic_enable() says "this callback must enable the
> display link feeding the next bridge in the chain if there is one."
> 
> That looks to me like link training, i.e. the display link, should
> happen in the enable path and not hpd_notify. It looks like link
> training could fail, but when that happens I believe the driver should
> call drm_connector_set_link_status_property() with
> DRM_MODE_LINK_STATUS_BAD. The two callers of that which exist in the
> tree also call drm_kms_helper_hotplug_event() or
> drm_kms_helper_connector_hotplug_event() after updating the link so that
> userspace knows to try again.
> 
> It would be nice if there was some drm_bridge_set_link_status_bad() API
> that bridge drivers could use to signal that the link status is bad and
> call the hotplug helper. Maybe it could also record some diagnostics
> about which bridge failed to setup the link and stop the atomic_enable()
> chain for that connector.

Doing link training when we get hpd instead of atomic_enable() is a 
design choice we have been following for a while because for the case 
when link training fails in atomic_enable() and setting the link status 
property as you mentioned, the compositor needs to be able to handle 
that and also needs to try with a different resolution or take some 
other corrective action. We have seen many compositors not able to 
handle this complexity. So the design sends the hotplug to usermode only 
after link training succeeds.

I do not think we should change this design unless prototyped with an 
existing compositor such as chrome or android at this point.

Thanks

Abhinav

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

[Dmitry Baryshkov]

On Sat, 23 Sept 2023 at 02:03, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
> > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> 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 v3:
> >> -- move calling dp_display_irq_handler() to probe
> > Was there a changelog for the previous reivions? What is the
> > difference between v1 and v2?
> >
> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
> >>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
> >>   2 files changed, 13 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 76f1395..c217430 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1193,30 +1193,23 @@ 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);
> >>          if (!dp->irq) {
> > What is the point in this check?
> >
> >> -               DRM_ERROR("failed to get irq\n");
> >> -               return -EINVAL;
> >> +               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 +1280,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;
> > This way the IRQ ends up being enabled in _probe. Are we ready to
> > handle it here? Is the DP device fully setup at this moment?
>
> The irq is enabled here.
>
> but DP driver hpd hardware block has not yet be enabled. this means no
> irq will be delivered.

There are other IRQ kinds, not only just HPD ones.

>
>   .hpd_enable() will call pm_runtime_resume_and_get() and
> dp_catalog_ctrl_hpd_enable().
>
> after .hpd_enable() irq will be delivered and handled properly.
>
>
>
> >> +
> >>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
> >>          if (rc) {
> >>                  DRM_ERROR("component add failed, rc=%d\n", rc);
> >> @@ -1549,12 +1546,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 v3 4/7] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Dmitry Baryshkov]

On Thu, 21 Sept 2023 at 01:46, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 9/15/2023 6:07 PM, Dmitry Baryshkov wrote:
> > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> Currently DP driver is executed independent of PM runtime framework.
> >> This lead DP driver incompatible with others. Incorporating pm runtime
> > Why is it incompatible? Which others are mentioned here?
> >
> >> 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. Both EV_POWER_PM_GET and EV_POWER_PM_PUT events
> >> are introduced to perform pm runtime control for the HPD GPIO routing to a
> >> display-connector case.
> >>
> >> 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
> > Previous changelog?
> >
> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_aux.c     |   5 ++
> >>   drivers/gpu/drm/msm/dp/dp_display.c | 114 +++++++++++++++++++++++++++---------
> >>   drivers/gpu/drm/msm/dp/dp_power.c   |   9 ---
> >>   3 files changed, 90 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> index 8e3b677..8fa93c5 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> @@ -291,6 +291,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> >>                  return -EINVAL;
> >>          }
> >>
> >> +       if (pm_runtime_resume_and_get(dp_aux->dev))
> >> +               return  -EINVAL;
> > Please propagate error values instead of reinventing them.
> >
> >> +
> >>          mutex_lock(&aux->mutex);
> >>          if (!aux->initted) {
> >>                  ret = -EIO;
> >> @@ -364,6 +367,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);
> > What is the reason for using autosuspend? Such design decisions should
> > be described in the commit message.
> >
> >>          return ret;
> >>   }
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 59f9d85..e7af7f7 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -60,6 +60,8 @@ enum {
> >>          EV_IRQ_HPD_INT,
> >>          EV_HPD_UNPLUG_INT,
> >>          EV_USER_NOTIFICATION,
> >> +       EV_POWER_PM_GET,
> >> +       EV_POWER_PM_PUT,
> >>   };
> >>
> >>   #define EVENT_TIMEOUT  (HZ/10) /* 100ms */
> >> @@ -276,13 +278,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;
> >>          dp->aux->drm_dev = drm;
> >>          rc = dp_aux_register(dp->aux);
> >> @@ -291,12 +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) {
> >>                  DRM_ERROR("Audio registration Dp failed\n");
> >> @@ -320,10 +309,6 @@ 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);
> >> @@ -467,6 +452,18 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
> >>          dp->core_initialized = false;
> >>   }
> >>
> >> +static void dp_display_pm_get(struct dp_display_private *dp)
> >> +{
> >> +       if (pm_runtime_resume_and_get(&dp->pdev->dev))
> >> +               DRM_ERROR("failed to start power\n");
> >> +}
> > Huge NAK here. This means that the error is completely ignored (other
> > than being dumped to the log). This is a short path to Sync error and
> > other kinds of reboot.
> >
> >> +
> >> +static void dp_display_pm_put(struct dp_display_private *dp)
> >> +{
> >> +       pm_runtime_mark_last_busy(&dp->pdev->dev);
> >> +       pm_runtime_put_autosuspend(&dp->pdev->dev);
> >> +}
> >> +
> >>   static int dp_display_usbpd_configure_cb(struct device *dev)
> >>   {
> >>          struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >> @@ -1096,7 +1093,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);
> >> @@ -1111,6 +1107,12 @@ static int hpd_event_thread(void *data)
> >>                          dp_display_send_hpd_notification(dp_priv,
> >>                                                  todo->data);
> >>                          break;
> >> +               case EV_POWER_PM_GET:
> >> +                       dp_display_pm_get(dp_priv);
> >> +                       break;
> >> +               case EV_POWER_PM_PUT:
> >> +                       dp_display_pm_put(dp_priv);
> >> +                       break;
> > No. runtime_get / runtime_put are not HPD events. They should be
> > executed directly from the place where the drivers needs the device to
> > be powered up.
> >
> >>                  default:
> >>                          break;
> >>                  }
> >> @@ -1251,6 +1253,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;
> >> +       }
> > Why? This moves resource allocation to the probe function, which is
> > irrelevant to the pm_runtime code. If this is required, you can move
> > these changes to a separate patch.
> >
> >> +
> >>          /* setup event q */
> >>          mutex_init(&dp->event_mutex);
> >>          init_waitqueue_head(&dp->event_q);
> >> @@ -1263,6 +1277,10 @@ static int dp_display_probe(struct platform_device *pdev)
> >>
> >>          platform_set_drvdata(pdev, &dp->dp_display);
> >>
> >> +       devm_pm_runtime_enable(&pdev->dev);
> > error code handling?
> >
> >> +       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 +1303,36 @@ static int dp_display_remove(struct platform_device *pdev)
> >>
> >>          platform_set_drvdata(pdev, NULL);
> >>
> >> +       pm_runtime_put_sync_suspend(&pdev->dev);
> > Why? Who is holding the pm count here?
> >
> >> +       pm_runtime_dont_use_autosuspend(&pdev->dev);
> >> +       pm_runtime_disable(&pdev->dev);
> > Why do you need _disable if you have a devm_pm_runtime_enable()? Not
> > to mention that pm_runtime_disable_action() already has a call to
> > pm_runtime_dont_use_autosuspend()
> >
> >> +
> >> +       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 +1437,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 +1522,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 +1623,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) {
> >> @@ -1658,6 +1708,8 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
> >>          }
> >>
> >>          drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
> >> +
> >> +       pm_runtime_put_sync(&dp_display->pdev->dev);
> > So, no autosuspend now?
> >
> > Also, I think we can get an unbalanced runtime status, as there is no
> > guarantee that atomic_enable / atomic_disable will be paired. Please
> > correct me if I'm wrong.
>
>   I always assume atomic_enable / atomic_disable should be paired.
> Otherwise nothing will work.
> Could you please give me example what situations they are not paired?

Please excuse me, it took a while to check the docs. Indeed, you were
right. For atomic drivers corresponding encoder helper calls are
required to be inverse pairs. Thus bridge calls should also be an
inverse of each other.

>
> > And also there is a possible return earlier in this function. The
> > driver will leak the runtime status again.
> >
> >>          mutex_unlock(&dp_display->event_mutex);
> >>   }
> >>
> >> @@ -1697,6 +1749,9 @@ 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");
> > Return?
> >
> >> +
> >>          dp_catalog_ctrl_hpd_enable(dp->catalog);
> >>
> >>          /* enable HDP interrupts */
> >> @@ -1718,6 +1773,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 +1790,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)
> >> +       if (!dp_display->link_ready && status == connector_status_connected) {
> >> +               dp_add_event(dp, EV_POWER_PM_GET, 0, 0);
> > Why? What for?
> >
> >>                  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);
> >> +               dp_add_event(dp, EV_POWER_PM_PUT, 0, 0);
> >> +       }
> >>   }
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> >> index 5cb84ca..ed2f62a 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> >> @@ -152,8 +152,6 @@ 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);
> >>   }
> >>
> >> @@ -162,8 +160,6 @@ 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)
> >> @@ -173,11 +169,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 +181,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;
> >>   }
> >>
> >> --
> >> 2.7.4
> >>
> >



-- 
With best wishes
Dmitry

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Kuogee Hsieh]

On 9/22/2023 6:35 PM, Abhinav Kumar wrote:
> Hi Stephen
>
> On 9/22/2023 2:54 PM, Stephen Boyd wrote:
>> Quoting Dmitry Baryshkov (2023-09-19 02:50:12)
>>> On Mon, 18 Sept 2023 at 20:48, Kuogee Hsieh 
>>> <quic_khsieh@quicinc.com> wrote:
>>>>
>>>>
>>>> On 9/15/2023 6:21 PM, Dmitry Baryshkov wrote:
>>>>> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh 
>>>>> <quic_khsieh@quicinc.com> wrote:
>>>>>> Add pm_runtime_force_suspend()/resume() to complete incorporating pm
>>>>>> runtime framework into DP driver. Both dp_pm_prepare() and 
>>>>>> dp_pm_complete()
>>>>>> are added to set hpd_state to correct state. After resume, DP 
>>>>>> driver will
>>>>>> re training its main link after .hpd_enable() callback enabled HPD
>>>>>> interrupts and bring up display accordingly.
>>>>> How will it re-train the main link? What is the code path for that?
>>>>
>>>> 1) for edp, dp_bridge_atomic_enable(), called from framework, to start
>>>> link training and bring up display.
>>>
>>> And this path doesn't use .hpd_enable() which you have mentioned in
>>> the commit message. Please don't try to shorten the commit message.
>>> You see, I have had questions to several of them, which means that
>>> they were not verbose enough.
>>>
>>>>
>>>> 2) for external DP, HPD_PLUG_INT will be generated to start link
>>>> training and bring up display.
>>>
>>> This should be hpd_notify, who starts link training, not some event.
>>
>> I think this driver should train the link during atomic_enable(), not
>> hpd_notify() (or directly from the irq handler). The drm_bridge_funcs
>> talk a bit about when the clocks and timing signals are supposed to be
>> enabled. For example, struct drm_bridge_funcs::atomic_pre_enable() says
>> the "display pipe (i.e.  clocks and timing signals) feeding this bridge
>> will not yet be running when this callback is called". And struct
>> drm_bridge_funcs::atomic_enable() says "this callback must enable the
>> display link feeding the next bridge in the chain if there is one."
>>
>> That looks to me like link training, i.e. the display link, should
>> happen in the enable path and not hpd_notify. It looks like link
>> training could fail, but when that happens I believe the driver should
>> call drm_connector_set_link_status_property() with
>> DRM_MODE_LINK_STATUS_BAD. The two callers of that which exist in the
>> tree also call drm_kms_helper_hotplug_event() or
>> drm_kms_helper_connector_hotplug_event() after updating the link so that
>> userspace knows to try again.
>>
>> It would be nice if there was some drm_bridge_set_link_status_bad() API
>> that bridge drivers could use to signal that the link status is bad and
>> call the hotplug helper. Maybe it could also record some diagnostics
>> about which bridge failed to setup the link and stop the atomic_enable()
>> chain for that connector.
>
> Doing link training when we get hpd instead of atomic_enable() is a 
> design choice we have been following for a while because for the case 
> when link training fails in atomic_enable() and setting the link 
> status property as you mentioned, the compositor needs to be able to 
> handle that and also needs to try with a different resolution or take 
> some other corrective action. We have seen many compositors not able 
> to handle this complexity. So the design sends the hotplug to usermode 
> only after link training succeeds.
>
> I do not think we should change this design unless prototyped with an 
> existing compositor such as chrome or android at this point.
>
> Thanks
>
> Abhinav


We did perform link training at atomic_enable() at eDP case since we can 
assume link training will always success without link rate or link lane 
being reduced.

However for external DP case, link training can not be guarantee always 
success without link rate or lane being reduced as Abhinav mentioned.

In addition,  CTS (compliance test) it required to complete link 
training within 10ms after hpd asserted.

I am not sure do link training at atomic_enable() can meet this timing 
requirement.

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

[Kuogee Hsieh]

On 9/23/2023 11:45 AM, Dmitry Baryshkov wrote:
> On Sat, 23 Sept 2023 at 02:03, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
>>> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>> 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 v3:
>>>> -- move calling dp_display_irq_handler() to probe
>>> Was there a changelog for the previous reivions? What is the
>>> difference between v1 and v2?
>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
>>>>    drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>>>>    2 files changed, 13 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index 76f1395..c217430 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -1193,30 +1193,23 @@ 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);
>>>>           if (!dp->irq) {
>>> What is the point in this check?
>>>
>>>> -               DRM_ERROR("failed to get irq\n");
>>>> -               return -EINVAL;
>>>> +               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 +1280,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;
>>> This way the IRQ ends up being enabled in _probe. Are we ready to
>>> handle it here? Is the DP device fully setup at this moment?
>> The irq is enabled here.
>>
>> but DP driver hpd hardware block has not yet be enabled. this means no
>> irq will be delivered.
> There are other IRQ kinds, not only just HPD ones.

pm_runtime_resume_and_get() will enable host controller (including hpd and aux block).
so that as long as pm_runtime_resume_and_get() called, then all DP related interrupts will be handled accordingly.

>
>>    .hpd_enable() will call pm_runtime_resume_and_get() and
>> dp_catalog_ctrl_hpd_enable().
>>
>> after .hpd_enable() irq will be delivered and handled properly.
>>
>>
>>
>>>> +
>>>>           rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>>>           if (rc) {
>>>>                   DRM_ERROR("component add failed, rc=%d\n", rc);
>>>> @@ -1549,12 +1546,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>>>
>>>>           dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
>>>>
>>>> -       ret = dp_display_request_irq(dp_display);
>>>> -       if (ret) {
>>>> -               DRM_ERROR("request_irq failed, ret=%d\n", ret);
>>>> -               return ret;
>>>> -       }
>>>> -
>>>>           ret = dp_display_get_next_bridge(dp_display);
>>>>           if (ret)
>>>>                   return ret;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>>>> index 1e9415a..b3c08de 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>>>> @@ -35,7 +35,6 @@ struct msm_dp {
>>>>    int dp_display_set_plugged_cb(struct msm_dp *dp_display,
>>>>                   hdmi_codec_plugged_cb fn, struct device *codec_dev);
>>>>    int dp_display_get_modes(struct msm_dp *dp_display);
>>>> -int dp_display_request_irq(struct msm_dp *dp_display);
>>>>    bool dp_display_check_video_test(struct msm_dp *dp_display);
>>>>    int dp_display_get_test_bpp(struct msm_dp *dp_display);
>>>>    void dp_display_signal_audio_start(struct msm_dp *dp_display);
>>>> --
>>>> 2.7.4
>>>>
>
>

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Stephen Boyd]

Quoting Abhinav Kumar (2023-09-22 18:35:27)
> On 9/22/2023 2:54 PM, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2023-09-19 02:50:12)
> >>
> >> This should be hpd_notify, who starts link training, not some event.
> >
> > I think this driver should train the link during atomic_enable(), not
> > hpd_notify() (or directly from the irq handler). The drm_bridge_funcs
> > talk a bit about when the clocks and timing signals are supposed to be
> > enabled. For example, struct drm_bridge_funcs::atomic_pre_enable() says
> > the "display pipe (i.e.  clocks and timing signals) feeding this bridge
> > will not yet be running when this callback is called". And struct
> > drm_bridge_funcs::atomic_enable() says "this callback must enable the
> > display link feeding the next bridge in the chain if there is one."
> >
> > That looks to me like link training, i.e. the display link, should
> > happen in the enable path and not hpd_notify. It looks like link
> > training could fail, but when that happens I believe the driver should
> > call drm_connector_set_link_status_property() with
> > DRM_MODE_LINK_STATUS_BAD. The two callers of that which exist in the
> > tree also call drm_kms_helper_hotplug_event() or
> > drm_kms_helper_connector_hotplug_event() after updating the link so that
> > userspace knows to try again.
> >
> > It would be nice if there was some drm_bridge_set_link_status_bad() API
> > that bridge drivers could use to signal that the link status is bad and
> > call the hotplug helper. Maybe it could also record some diagnostics
> > about which bridge failed to setup the link and stop the atomic_enable()
> > chain for that connector.
>
> Doing link training when we get hpd instead of atomic_enable() is a
> design choice we have been following for a while because for the case
> when link training fails in atomic_enable() and setting the link status
> property as you mentioned, the compositor needs to be able to handle
> that and also needs to try with a different resolution or take some
> other corrective action. We have seen many compositors not able to
> handle this complexity.

The chrome compositor handles this case[1]. If the link status is set to
bad and there are non-zero number of modes on a connected connector it
resets the status to good to try again.

> So the design sends the hotplug to usermode only
> after link training succeeds.

I suspect this is why my trogdor device turns off after rebooting when I
apply a ChromeOS update with the lid closed and DP connected. Userspace
wants to know that a display is connected, but this driver is still link
training by the time userspace boots up so we don't see any drm
connector indicating status is connected. No drm connectors connected
means the OS should shutdown.

>
> I do not think we should change this design unless prototyped with an
> existing compositor such as chrome or android at this point.

Is this driver used with android?

[1] https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc;l=114;drc=67520ac99db89395b10f2ab728b540eef0da8292

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Stephen Boyd]

Quoting Kuogee Hsieh (2023-09-25 09:07:18)
>
> On 9/22/2023 6:35 PM, Abhinav Kumar wrote:
> >
> > Doing link training when we get hpd instead of atomic_enable() is a
> > design choice we have been following for a while because for the case
> > when link training fails in atomic_enable() and setting the link
> > status property as you mentioned, the compositor needs to be able to
> > handle that and also needs to try with a different resolution or take
> > some other corrective action. We have seen many compositors not able
> > to handle this complexity. So the design sends the hotplug to usermode
> > only after link training succeeds.
> >
> > I do not think we should change this design unless prototyped with an
> > existing compositor such as chrome or android at this point.
> >
> > Thanks
> >
> > Abhinav
>
>
> We did perform link training at atomic_enable() at eDP case since we can
> assume link training will always success without link rate or link lane
> being reduced.
>
> However for external DP case, link training can not be guarantee always
> success without link rate or lane being reduced as Abhinav mentioned.
>
> In addition,  CTS (compliance test) it required to complete link
> training within 10ms after hpd asserted.

Is it possible to change that timeout? I have to look around for the CTS
parameters because I'm pretty confused how it can work. What do we do if
DP wakes the system from suspend and asserts HPD? We need resume time to
be < 10ms?  That's not realistic.

>
> I am not sure do link training at atomic_enable() can meet this timing
> requirement.
>

At least in the DP spec itself it doesn't require the link to be trained
within 10ms of HPD being asserted. Instead it simply recommends that the
OS start configuring the display promptly after HPD is asserted, e.g.
within 100ms. There's some strict timing on IRQ_HPD, so the driver must
read DPCD registers within 100ms of IRQ_HPD rising edge; maybe that is
what CTS is checking for?

TL;DR: I don't see why CTS should stop us from link training in
atomic_enable(). It would be beneficial to do so to make eDP and DP the
same. It would also help to report a drm connector being connected
_before_ link training so that userspace knows the link itself is the
bad part of the equation (and not that the DP connector looks
disconnected to userspace when in fact it really is connected and the
monitor is asserting HPD, just the link training failed).

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Dmitry Baryshkov]

On Thu, 28 Sept 2023 at 01:01, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Kuogee Hsieh (2023-09-25 09:07:18)
> >
> > On 9/22/2023 6:35 PM, Abhinav Kumar wrote:
> > >
> > > Doing link training when we get hpd instead of atomic_enable() is a
> > > design choice we have been following for a while because for the case
> > > when link training fails in atomic_enable() and setting the link
> > > status property as you mentioned, the compositor needs to be able to
> > > handle that and also needs to try with a different resolution or take
> > > some other corrective action. We have seen many compositors not able
> > > to handle this complexity. So the design sends the hotplug to usermode
> > > only after link training succeeds.
> > >
> > > I do not think we should change this design unless prototyped with an
> > > existing compositor such as chrome or android at this point.
> > >
> > > Thanks
> > >
> > > Abhinav
> >
> >
> > We did perform link training at atomic_enable() at eDP case since we can
> > assume link training will always success without link rate or link lane
> > being reduced.
> >
> > However for external DP case, link training can not be guarantee always
> > success without link rate or lane being reduced as Abhinav mentioned.
> >
> > In addition,  CTS (compliance test) it required to complete link
> > training within 10ms after hpd asserted.
>
> Is it possible to change that timeout? I have to look around for the CTS
> parameters because I'm pretty confused how it can work. What do we do if
> DP wakes the system from suspend and asserts HPD? We need resume time to
> be < 10ms?  That's not realistic.
>
> >
> > I am not sure do link training at atomic_enable() can meet this timing
> > requirement.
> >
>
> At least in the DP spec itself it doesn't require the link to be trained
> within 10ms of HPD being asserted. Instead it simply recommends that the
> OS start configuring the display promptly after HPD is asserted, e.g.
> within 100ms. There's some strict timing on IRQ_HPD, so the driver must
> read DPCD registers within 100ms of IRQ_HPD rising edge; maybe that is
> what CTS is checking for?
>
> TL;DR: I don't see why CTS should stop us from link training in
> atomic_enable(). It would be beneficial to do so to make eDP and DP the
> same. It would also help to report a drm connector being connected
> _before_ link training so that userspace knows the link itself is the
> bad part of the equation (and not that the DP connector looks
> disconnected to userspace when in fact it really is connected and the
> monitor is asserting HPD, just the link training failed).

Also this will move us closer to i915 user experience: failing link
training in the display enablement part. So that a part of xrandr
calls can retry link training.

-- 
With best wishes
Dmitry

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Abhinav Kumar]

On 9/27/2023 2:41 PM, Stephen Boyd wrote:
> Quoting Abhinav Kumar (2023-09-22 18:35:27)
>> On 9/22/2023 2:54 PM, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2023-09-19 02:50:12)
>>>>
>>>> This should be hpd_notify, who starts link training, not some event.
>>>
>>> I think this driver should train the link during atomic_enable(), not
>>> hpd_notify() (or directly from the irq handler). The drm_bridge_funcs
>>> talk a bit about when the clocks and timing signals are supposed to be
>>> enabled. For example, struct drm_bridge_funcs::atomic_pre_enable() says
>>> the "display pipe (i.e.  clocks and timing signals) feeding this bridge
>>> will not yet be running when this callback is called". And struct
>>> drm_bridge_funcs::atomic_enable() says "this callback must enable the
>>> display link feeding the next bridge in the chain if there is one."
>>>
>>> That looks to me like link training, i.e. the display link, should
>>> happen in the enable path and not hpd_notify. It looks like link
>>> training could fail, but when that happens I believe the driver should
>>> call drm_connector_set_link_status_property() with
>>> DRM_MODE_LINK_STATUS_BAD. The two callers of that which exist in the
>>> tree also call drm_kms_helper_hotplug_event() or
>>> drm_kms_helper_connector_hotplug_event() after updating the link so that
>>> userspace knows to try again.
>>>
>>> It would be nice if there was some drm_bridge_set_link_status_bad() API
>>> that bridge drivers could use to signal that the link status is bad and
>>> call the hotplug helper. Maybe it could also record some diagnostics
>>> about which bridge failed to setup the link and stop the atomic_enable()
>>> chain for that connector.
>>
>> Doing link training when we get hpd instead of atomic_enable() is a
>> design choice we have been following for a while because for the case
>> when link training fails in atomic_enable() and setting the link status
>> property as you mentioned, the compositor needs to be able to handle
>> that and also needs to try with a different resolution or take some
>> other corrective action. We have seen many compositors not able to
>> handle this complexity.
> 
> The chrome compositor handles this case[1]. If the link status is set to
> bad and there are non-zero number of modes on a connected connector it
> resets the status to good to try again.
> 

Thanks for the link. Just resetting the property alone and trying again 
is going to lead to the same failure again. So that alone is 
insufficient and doesn't sound right.

As documented here:

https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#standard-connector-properties

"When user-space receives the hotplug uevent and detects a "BAD" 
link-status, the sink doesn't receive pixels anymore (e.g. the screen 
becomes completely black). The list of available modes may have changed. 
User-space is expected to pick a new mode if the current one has 
disappeared and perform a new modeset with link-status set to "GOOD" to 
re-enable the connector."

Picking a different mode is a reasonable attempt to try again but even 
that might fail again if it picks a mode which falls in the same link rate.

Thats why, to re-iterate what i mentioned, we need to see if some sort 
of a handshake fallback exists or can be implemented. It will need 
compositor support as well as driver change to maybe remove that mode etc.

We prioritized user experience here to make sure display_enable() wont 
fail otherwise the user might keep waiting for the screen to come up 
forever. With the driver ensuring link is trained and then reporting to 
usermode, its a safer option as the driver will train with the highest 
link rate / lane combination supported and also remove modes which dont 
fall in this bucket in dp_bridge_mode_valid.

Till we validate this, I would prefer to keep this change out of this 
series.

>> So the design sends the hotplug to usermode only
>> after link training succeeds.
> 
> I suspect this is why my trogdor device turns off after rebooting when I
> apply a ChromeOS update with the lid closed and DP connected. Userspace
> wants to know that a display is connected, but this driver is still link
> training by the time userspace boots up so we don't see any drm
> connector indicating status is connected. No drm connectors connected
> means the OS should shutdown.
> 

Interesting use case but I am not sure if thats whats happening till we 
debug that. Why should OS shutdown if connectors are not in "connected" 
state? Agreed, display will be off. But shouldnt compositor be alive in 
case it receives hotplug? The user (in this case you) never turned the 
device off so why should the OS shutdown?

>>
>> I do not think we should change this design unless prototyped with an
>> existing compositor such as chrome or android at this point.
> 
> Is this driver used with android?
> 

There are some internal efforts ongoing with prototyping this but I 
cannot comment more on this right now.

> [1] https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc;l=114;drc=67520ac99db89395b10f2ab728b540eef0da8292

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Abhinav Kumar]

On 9/27/2023 3:01 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2023-09-25 09:07:18)
>>
>> On 9/22/2023 6:35 PM, Abhinav Kumar wrote:
>>>
>>> Doing link training when we get hpd instead of atomic_enable() is a
>>> design choice we have been following for a while because for the case
>>> when link training fails in atomic_enable() and setting the link
>>> status property as you mentioned, the compositor needs to be able to
>>> handle that and also needs to try with a different resolution or take
>>> some other corrective action. We have seen many compositors not able
>>> to handle this complexity. So the design sends the hotplug to usermode
>>> only after link training succeeds.
>>>
>>> I do not think we should change this design unless prototyped with an
>>> existing compositor such as chrome or android at this point.
>>>
>>> Thanks
>>>
>>> Abhinav
>>
>>
>> We did perform link training at atomic_enable() at eDP case since we can
>> assume link training will always success without link rate or link lane
>> being reduced.
>>
>> However for external DP case, link training can not be guarantee always
>> success without link rate or lane being reduced as Abhinav mentioned.
>>
>> In addition,  CTS (compliance test) it required to complete link
>> training within 10ms after hpd asserted.
> 
> Is it possible to change that timeout? I have to look around for the CTS
> parameters because I'm pretty confused how it can work. What do we do if
> DP wakes the system from suspend and asserts HPD? We need resume time to
> be < 10ms?  That's not realistic.
> 

No, the CTS doesnt say we need to finish link training within 10ms after 
HPD is asserted. It says it must be completed in 10ms after 
TRAINING_PATTERN_SET dpcd write.

"Wait until the Source DUT writes 00h to the TRAINING_PATTERN_SET byte 
of Reference Sink DPCD Link Configuration Field to indicate the end of 
the link training. Stop the link training timer. Verify that link 
training completed in 10ms or less"

That needs to be done independent of HPD so we can ignore the CTS point.

>>
>> I am not sure do link training at atomic_enable() can meet this timing
>> requirement.
>>
> 
> At least in the DP spec itself it doesn't require the link to be trained
> within 10ms of HPD being asserted. Instead it simply recommends that the
> OS start configuring the display promptly after HPD is asserted, e.g.
> within 100ms. There's some strict timing on IRQ_HPD, so the driver must
> read DPCD registers within 100ms of IRQ_HPD rising edge; maybe that is
> what CTS is checking for?
> 
> TL;DR: I don't see why CTS should stop us from link training in
> atomic_enable(). It would be beneficial to do so to make eDP and DP the
> same. It would also help to report a drm connector being connected
> _before_ link training so that userspace knows the link itself is the
> bad part of the equation (and not that the DP connector looks
> disconnected to userspace when in fact it really is connected and the
> monitor is asserting HPD, just the link training failed).

Its the corrective action of the userspace when it finds link is bad is 
the concern as I highlighted in the other response. Just reading and 
resetting link_status is not enough to recover.

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Stephen Boyd]

Quoting Abhinav Kumar (2023-09-28 17:46:11)
> On 9/27/2023 3:01 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2023-09-25 09:07:18)
> >>
> >> However for external DP case, link training can not be guarantee always
> >> success without link rate or lane being reduced as Abhinav mentioned.
> >>
> >> In addition,  CTS (compliance test) it required to complete link
> >> training within 10ms after hpd asserted.
> >
> > Is it possible to change that timeout? I have to look around for the CTS
> > parameters because I'm pretty confused how it can work. What do we do if
> > DP wakes the system from suspend and asserts HPD? We need resume time to
> > be < 10ms?  That's not realistic.
> >
>
> No, the CTS doesnt say we need to finish link training within 10ms after
> HPD is asserted. It says it must be completed in 10ms after
> TRAINING_PATTERN_SET dpcd write.
>
> "Wait until the Source DUT writes 00h to the TRAINING_PATTERN_SET byte
> of Reference Sink DPCD Link Configuration Field to indicate the end of
> the link training. Stop the link training timer. Verify that link
> training completed in 10ms or less"
>
> That needs to be done independent of HPD so we can ignore the CTS point.

Great!

>
> >>
> >> I am not sure do link training at atomic_enable() can meet this timing
> >> requirement.

Why? It's putting some time bound on link training in general to only
take 10ms, right?

> >>
> >
> > At least in the DP spec itself it doesn't require the link to be trained
> > within 10ms of HPD being asserted. Instead it simply recommends that the
> > OS start configuring the display promptly after HPD is asserted, e.g.
> > within 100ms. There's some strict timing on IRQ_HPD, so the driver must
> > read DPCD registers within 100ms of IRQ_HPD rising edge; maybe that is
> > what CTS is checking for?
> >
> > TL;DR: I don't see why CTS should stop us from link training in
> > atomic_enable(). It would be beneficial to do so to make eDP and DP the
> > same. It would also help to report a drm connector being connected
> > _before_ link training so that userspace knows the link itself is the
> > bad part of the equation (and not that the DP connector looks
> > disconnected to userspace when in fact it really is connected and the
> > monitor is asserting HPD, just the link training failed).
>
> Its the corrective action of the userspace when it finds link is bad is
> the concern as I highlighted in the other response. Just reading and
> resetting link_status is not enough to recover.

What needs to be done to recover? Userspace will try to set a mode on
the connector again if the link status is bad and there were some modes
available. If there are zero modes and the link is bad, then it ignores
the connector. I'm not sure what else could be done to recover besides
try again and stop trying if no modes exist.

Acting like the connector isn't connected makes the situation worse for
ChromeOS because userspace thinks there's nothing there so it can't try
to retrain the link again. Instead, userspace has to rely on the kernel
driver to train the link again. The kernel should just tell userspace
the link is bad so userspace can implement the policy to either ignore
the connector entirely or to consider it a display that is having link
training problems.

So again, I see no reason why the kernel driver thinks it can implement
a policy to train the link before indicating the drm connector is
connected. It should stop doing that. Instead it should tell userspace
that the connector is connected and then train the link when there's a
modeset. If the modeset fails then userspace can take action to either
figure out that the link is bad, or notify the user that the cable is
bad, or to try replugging or power cycle the monitor, etc. None of that
can be done if the kernel lies about the state of the connector because
the link training failed.

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Abhinav Kumar]

On 10/2/2023 3:58 PM, Stephen Boyd wrote:
> Quoting Abhinav Kumar (2023-09-28 17:46:11)
>> On 9/27/2023 3:01 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2023-09-25 09:07:18)
>>>>
>>>> However for external DP case, link training can not be guarantee always
>>>> success without link rate or lane being reduced as Abhinav mentioned.
>>>>
>>>> In addition,  CTS (compliance test) it required to complete link
>>>> training within 10ms after hpd asserted.
>>>
>>> Is it possible to change that timeout? I have to look around for the CTS
>>> parameters because I'm pretty confused how it can work. What do we do if
>>> DP wakes the system from suspend and asserts HPD? We need resume time to
>>> be < 10ms?  That's not realistic.
>>>
>>
>> No, the CTS doesnt say we need to finish link training within 10ms after
>> HPD is asserted. It says it must be completed in 10ms after
>> TRAINING_PATTERN_SET dpcd write.
>>
>> "Wait until the Source DUT writes 00h to the TRAINING_PATTERN_SET byte
>> of Reference Sink DPCD Link Configuration Field to indicate the end of
>> the link training. Stop the link training timer. Verify that link
>> training completed in 10ms or less"
>>
>> That needs to be done independent of HPD so we can ignore the CTS point.
> 
> Great!
> 
>>
>>>>
>>>> I am not sure do link training at atomic_enable() can meet this timing
>>>> requirement.
> 
> Why? It's putting some time bound on link training in general to only
> take 10ms, right?
> 

Like I said, CTS is mentioning 10ms to finish link training after the 
DUT writes 00h to the TRAINING_PATTERN_SET byte. So for this discussion 
lets leave out CTS for now.

>>>>
>>>
>>> At least in the DP spec itself it doesn't require the link to be trained
>>> within 10ms of HPD being asserted. Instead it simply recommends that the
>>> OS start configuring the display promptly after HPD is asserted, e.g.
>>> within 100ms. There's some strict timing on IRQ_HPD, so the driver must
>>> read DPCD registers within 100ms of IRQ_HPD rising edge; maybe that is
>>> what CTS is checking for?
>>>
>>> TL;DR: I don't see why CTS should stop us from link training in
>>> atomic_enable(). It would be beneficial to do so to make eDP and DP the
>>> same. It would also help to report a drm connector being connected
>>> _before_ link training so that userspace knows the link itself is the
>>> bad part of the equation (and not that the DP connector looks
>>> disconnected to userspace when in fact it really is connected and the
>>> monitor is asserting HPD, just the link training failed).
>>
>> Its the corrective action of the userspace when it finds link is bad is
>> the concern as I highlighted in the other response. Just reading and
>> resetting link_status is not enough to recover.
> 
> What needs to be done to recover? Userspace will try to set a mode on
> the connector again if the link status is bad and there were some modes
> available. If there are zero modes and the link is bad, then it ignores
> the connector. I'm not sure what else could be done to recover besides
> try again and stop trying if no modes exist.
> 

Let me re-explain if I didnt make this clear last time.

You are right. Thats all the "userspace" can do which is basically retry 
the mode. And like I said, its again only going to fail. All the 
corrective actions you mentioned below like ignoring the connector 
entirely or consider that the display has link training problems are not 
something we decided to go with on a commercial device where we expect 
things to be more reliable.

Let me re-explain what I explained in the prev response.

If driver issues hot-plug after link-training:

It would have implemented all the link training mechanisms such as 
trying lower rates/number of lanes and made sure that when the usermode 
queries the list of modes, only the modes which fit into the link rate 
which was link trained successfully will be exposed and the chances of a 
user ending up with a blank screen on connection are pretty high.

This reduces the dependency on usermodes to be smart enough to implement 
such policies and we would rather not depend on those unless we have 
some reference to a compositor which is more sturdy. I do not think the 
CrOS code you have pointed to is more sturdy than the driver mechanism 
explained above.

As opposed to this, if we just issue hotplug without any of this, 
usermode does not know which mode to retry as we do not remove or edit 
the mode list once link training fails.

> Acting like the connector isn't connected makes the situation worse for
> ChromeOS because userspace thinks there's nothing there so it can't try
> to retrain the link again. Instead, userspace has to rely on the kernel
> driver to train the link again. The kernel should just tell userspace
> the link is bad so userspace can implement the policy to either ignore
> the connector entirely or to consider it a display that is having link
> training problems.
> 

What gain will it give if it retries the same mode blindly as opposed to 
the safer option I have explained above. None of the policies you have 
highlighted seem like something an end user will be satisfied with.

> So again, I see no reason why the kernel driver thinks it can implement
> a policy to train the link before indicating the drm connector is
> connected. It should stop doing that. Instead it should tell userspace
> that the connector is connected and then train the link when there's a
> modeset. If the modeset fails then userspace can take action to either
> figure out that the link is bad, or notify the user that the cable is
> bad, or to try replugging or power cycle the monitor, etc. None of that
> can be done if the kernel lies about the state of the connector because
> the link training failed.

Usermode is unable to take the corrective action without proper support 
from the kernel like removing unsupported modes etc and I dont see other 
drivers taking an action like that. Kernel is not lying. Its delaying 
the status to a point where usermode can safely handle.

Please explain to me how any of the policies you have explained usermode 
can take are safer and have more chance of success than what we have now.

Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()

[Dmitry Baryshkov]

On Tue, 3 Oct 2023 at 04:33, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 10/2/2023 3:58 PM, Stephen Boyd wrote:
> > Quoting Abhinav Kumar (2023-09-28 17:46:11)
> >> On 9/27/2023 3:01 PM, Stephen Boyd wrote:
> >>> Quoting Kuogee Hsieh (2023-09-25 09:07:18)
> >>>>
> >>>> However for external DP case, link training can not be guarantee always
> >>>> success without link rate or lane being reduced as Abhinav mentioned.
> >>>>
> >>>> In addition,  CTS (compliance test) it required to complete link
> >>>> training within 10ms after hpd asserted.
> >>>
> >>> Is it possible to change that timeout? I have to look around for the CTS
> >>> parameters because I'm pretty confused how it can work. What do we do if
> >>> DP wakes the system from suspend and asserts HPD? We need resume time to
> >>> be < 10ms?  That's not realistic.
> >>>
> >>
> >> No, the CTS doesnt say we need to finish link training within 10ms after
> >> HPD is asserted. It says it must be completed in 10ms after
> >> TRAINING_PATTERN_SET dpcd write.
> >>
> >> "Wait until the Source DUT writes 00h to the TRAINING_PATTERN_SET byte
> >> of Reference Sink DPCD Link Configuration Field to indicate the end of
> >> the link training. Stop the link training timer. Verify that link
> >> training completed in 10ms or less"
> >>
> >> That needs to be done independent of HPD so we can ignore the CTS point.
> >
> > Great!
> >
> >>
> >>>>
> >>>> I am not sure do link training at atomic_enable() can meet this timing
> >>>> requirement.
> >
> > Why? It's putting some time bound on link training in general to only
> > take 10ms, right?
> >
>
> Like I said, CTS is mentioning 10ms to finish link training after the
> DUT writes 00h to the TRAINING_PATTERN_SET byte. So for this discussion
> lets leave out CTS for now.
>
> >>>>
> >>>
> >>> At least in the DP spec itself it doesn't require the link to be trained
> >>> within 10ms of HPD being asserted. Instead it simply recommends that the
> >>> OS start configuring the display promptly after HPD is asserted, e.g.
> >>> within 100ms. There's some strict timing on IRQ_HPD, so the driver must
> >>> read DPCD registers within 100ms of IRQ_HPD rising edge; maybe that is
> >>> what CTS is checking for?
> >>>
> >>> TL;DR: I don't see why CTS should stop us from link training in
> >>> atomic_enable(). It would be beneficial to do so to make eDP and DP the
> >>> same. It would also help to report a drm connector being connected
> >>> _before_ link training so that userspace knows the link itself is the
> >>> bad part of the equation (and not that the DP connector looks
> >>> disconnected to userspace when in fact it really is connected and the
> >>> monitor is asserting HPD, just the link training failed).
> >>
> >> Its the corrective action of the userspace when it finds link is bad is
> >> the concern as I highlighted in the other response. Just reading and
> >> resetting link_status is not enough to recover.
> >
> > What needs to be done to recover? Userspace will try to set a mode on
> > the connector again if the link status is bad and there were some modes
> > available. If there are zero modes and the link is bad, then it ignores
> > the connector. I'm not sure what else could be done to recover besides
> > try again and stop trying if no modes exist.
> >
>
> Let me re-explain if I didnt make this clear last time.
>
> You are right. Thats all the "userspace" can do which is basically retry
> the mode. And like I said, its again only going to fail. All the
> corrective actions you mentioned below like ignoring the connector
> entirely or consider that the display has link training problems are not
> something we decided to go with on a commercial device where we expect
> things to be more reliable.

I have had link training issues with one of my laptops (x86) and USB-C
dock. Usually switching to lower resolution works in such cases.
Moreover, in some cases after switching to low res, I can successfully
switch to high res.

>
> Let me re-explain what I explained in the prev response.
>
> If driver issues hot-plug after link-training:
>
> It would have implemented all the link training mechanisms such as
> trying lower rates/number of lanes and made sure that when the usermode
> queries the list of modes, only the modes which fit into the link rate
> which was link trained successfully will be exposed and the chances of a
> user ending up with a blank screen on connection are pretty high.
>
> This reduces the dependency on usermodes to be smart enough to implement
> such policies and we would rather not depend on those unless we have
> some reference to a compositor which is more sturdy. I do not think the
> CrOS code you have pointed to is more sturdy than the driver mechanism
> explained above.
>
> As opposed to this, if we just issue hotplug without any of this,
> usermode does not know which mode to retry as we do not remove or edit
> the mode list once link training fails.

I think we are trying to be overprotective here. From my point of
view, there are two kinds of issues:
1) We know that some modes can not be supported (e.g. because of the
amount of lanes available or because of the board link rate
limitations).
Of course the kernel should not present these modes to userspace

2) Modes that pass known limitations, but can not be set e.g. because
of the bad cable or dirty connector.
Neither kernel nor userspace have control here. Judging from my
experience with x86, we should pass all these modes to userspace. Then
the user can select what seems to be working.

>
> > Acting like the connector isn't connected makes the situation worse for
> > ChromeOS because userspace thinks there's nothing there so it can't try
> > to retrain the link again. Instead, userspace has to rely on the kernel
> > driver to train the link again. The kernel should just tell userspace
> > the link is bad so userspace can implement the policy to either ignore
> > the connector entirely or to consider it a display that is having link
> > training problems.
> >
>
> What gain will it give if it retries the same mode blindly as opposed to
> the safer option I have explained above. None of the policies you have
> highlighted seem like something an end user will be satisfied with.
>
> > So again, I see no reason why the kernel driver thinks it can implement
> > a policy to train the link before indicating the drm connector is
> > connected. It should stop doing that. Instead it should tell userspace
> > that the connector is connected and then train the link when there's a
> > modeset. If the modeset fails then userspace can take action to either
> > figure out that the link is bad, or notify the user that the cable is
> > bad, or to try replugging or power cycle the monitor, etc. None of that
> > can be done if the kernel lies about the state of the connector because
> > the link training failed.
>
> Usermode is unable to take the corrective action without proper support
> from the kernel like removing unsupported modes etc and I dont see other
> drivers taking an action like that. Kernel is not lying. Its delaying
> the status to a point where usermode can safely handle.
>
> Please explain to me how any of the policies you have explained usermode
> can take are safer and have more chance of success than what we have now.



-- 
With best wishes
Dmitry