[PATCH v1 0/5] incorporate pm runtime framework and eDP clean up

[PATCH v1 0/5] incorporate pm runtime framework and eDP clean up

[Kuogee Hsieh]

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

Kuogee Hsieh (5):
  drm/msm/dp: remove pm_runtime_xxx() from dp_power.c
  drm/msm/dp: incorporate pm_runtime framework into DP driver
  drm/msm/dp: delete EV_HPD_INIT_SETUP
  drm/msm/dp: move relevant dp initialization code from bind() to
    probe()
  drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP

 drivers/gpu/drm/msm/dp/dp_aux.c     |  28 +++++
 drivers/gpu/drm/msm/dp/dp_display.c | 204 +++++++++++++++++++++---------------
 drivers/gpu/drm/msm/dp/dp_display.h |   1 -
 drivers/gpu/drm/msm/dp/dp_power.c   |   9 --
 4 files changed, 145 insertions(+), 97 deletions(-)

-- 
2.7.4

[PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Kuogee Hsieh]

Since both pm_runtime_resume() and pm_runtime_suspend() are not
populated at dp_pm_ops. Those pm_runtime_get/put() functions within
dp_power.c will not have any effects in addition to increase/decrease
power counter. Also pm_runtime_xxx() should be executed at top
layer.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_power.c | 9 ---------
 1 file changed, 9 deletions(-)

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

[Kuogee Hsieh]

Incorporating pm runtime framework into DP driver so that power
and clock resource handling can be centralized allowing easier
control of these resources in preparation of registering aux bus
uring probe.

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

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 8e3b677..c592064 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		return -EINVAL;
 	}
 
+	pm_runtime_get_sync(dp_aux->dev);
 	mutex_lock(&aux->mutex);
 	if (!aux->initted) {
 		ret = -EIO;
@@ -364,6 +365,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 76f1395..2c5706a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, struct device *master,
 		goto end;
 	}
 
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+
 	return 0;
 end:
 	return rc;
@@ -320,9 +324,8 @@ 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);
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_disable(dev);
 
 	kthread_stop(dp->ev_tsk);
 
@@ -466,10 +469,12 @@ static void dp_display_host_init(struct dp_display_private *dp)
 		dp->dp_display.connector_type, dp->core_initialized,
 		dp->phy_initialized);
 
-	dp_power_init(dp->power);
-	dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
-	dp_aux_init(dp->aux);
-	dp->core_initialized = true;
+	if (!dp->core_initialized) {
+		dp_power_init(dp->power);
+		dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
+		dp_aux_init(dp->aux);
+		dp->core_initialized = true;
+	}
 }
 
 static void dp_display_host_deinit(struct dp_display_private *dp)
@@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
 		dp->dp_display.connector_type, dp->core_initialized,
 		dp->phy_initialized);
 
-	dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
-	dp_aux_deinit(dp->aux);
-	dp_power_deinit(dp->power);
-	dp->core_initialized = false;
+	if (dp->core_initialized) {
+		dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+		dp_aux_deinit(dp->aux);
+		dp_power_deinit(dp->power);
+		dp->core_initialized = false;
+	}
 }
 
 static int dp_display_usbpd_configure_cb(struct device *dev)
@@ -1304,6 +1311,39 @@ static int dp_display_remove(struct platform_device *pdev)
 	dp_display_deinit_sub_modules(dp);
 
 	platform_set_drvdata(pdev, NULL);
+	pm_runtime_put_sync_suspend(&pdev->dev);
+
+	return 0;
+}
+
+static int dp_pm_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct msm_dp *dp_display = platform_get_drvdata(pdev);
+	struct dp_display_private *dp;
+
+	dp = container_of(dp_display, struct dp_display_private, dp_display);
+
+	dp_display_host_phy_exit(dp);
+	dp_catalog_ctrl_hpd_enable(dp->catalog);
+	dp_display_host_deinit(dp);
+
+	return 0;
+}
+
+static int dp_pm_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct msm_dp *dp_display = platform_get_drvdata(pdev);
+	struct dp_display_private *dp;
+
+	dp = container_of(dp_display, struct dp_display_private, dp_display);
+
+	dp_display_host_init(dp);
+	if (dp_display->is_edp) {
+		dp_catalog_ctrl_hpd_enable(dp->catalog);
+		dp_display_host_phy_init(dp);
+	}
 
 	return 0;
 }
@@ -1409,6 +1449,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,
 };
@@ -1493,10 +1534,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.
@@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 		dp_hpd_plug_handle(dp_display, 0);
 
 	mutex_lock(&dp_display->event_mutex);
+	pm_runtime_get_sync(&dp_display->pdev->dev);
 
 	state = dp_display->hpd_state;
 	if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
@@ -1684,6 +1722,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);
 }
 
@@ -1723,6 +1763,8 @@ 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);
+	pm_runtime_get_sync(&dp->pdev->dev);
+
 	dp_catalog_ctrl_hpd_enable(dp->catalog);
 
 	/* enable HDP interrupts */
@@ -1744,6 +1786,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);
 }
 
-- 
2.7.4

[PATCH v1 3/5] 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.

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

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 2c5706a..44580c2 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,
@@ -1119,9 +1118,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:
-			dp_display_host_init(dp_priv);
-			break;
 		case EV_HPD_PLUG_INT:
 			dp_hpd_plug_handle(dp_priv, todo->data);
 			break;
@@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void)
 
 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)
-- 
2.7.4

[PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Kuogee Hsieh]

In preparation of moving edp of_dp_aux_populate_bus() to
dp_display_probe(), move dp_display_request_irq(),
dp->parser->parse() and dp_power_client_init() to dp_display_probe()
too.

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

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 44580c2..185f1eb 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -290,12 +290,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");
@@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
+	rc = dp->parser->parse(dp->parser);
+	if (rc) {
+		DRM_ERROR("device tree parsing failed\n");
+		goto error;
+	}
+
 	dp->catalog = dp_catalog_get(dev, &dp->parser->io);
 	if (IS_ERR(dp->catalog)) {
 		rc = PTR_ERR(dp->catalog);
@@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
+	rc = dp_power_client_init(dp->power);
+	if (rc) {
+		DRM_ERROR("Power client create failed\n");
+		goto error;
+	}
+
 	dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
 	if (IS_ERR(dp->aux)) {
 		rc = PTR_ERR(dp->aux);
@@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 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",
@@ -1290,6 +1290,8 @@ static int dp_display_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dp->dp_display);
 
+	dp_display_request_irq(dp);
+
 	rc = component_add(&pdev->dev, &dp_display_comp_ops);
 	if (rc) {
 		DRM_ERROR("component add failed, rc=%d\n", rc);
@@ -1574,12 +1576,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 v1 5/5] drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP

[Kuogee Hsieh]

Move of_dp_aux_populate_bus() to dp_display_probe() for eDP
from dp_display_bind() so that probe deferral cases can be
handled effectively

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, 65 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index c592064..c1baffb 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -505,6 +505,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)
 {
@@ -528,6 +543,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 185f1eb..7ed4bea 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
 		goto end;
 	}
 
-	pm_runtime_enable(dev);
-	pm_runtime_set_autosuspend_delay(dev, 1000);
-	pm_runtime_use_autosuspend(dev);
-
 	return 0;
 end:
 	return rc;
@@ -322,8 +318,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);
@@ -1245,6 +1239,29 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde
 	return NULL;
 }
 
+static void of_dp_aux_depopulate_bus_void(void *data)
+{
+	of_dp_aux_depopulate_bus(data);
+}
+
+static int dp_display_auxbus_emulation(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, NULL);
+
+		devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void,
+					 dp->aux);
+	}
+
+	return ret;
+}
+
 static int dp_display_probe(struct platform_device *pdev)
 {
 	int rc = 0;
@@ -1290,8 +1307,18 @@ static int dp_display_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dp->dp_display);
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+
 	dp_display_request_irq(dp);
 
+	if (dp->dp_display.is_edp) {
+		rc = dp_display_auxbus_emulation(dp);
+		if (rc)
+			DRM_ERROR("eDP aux-bus emulation failed, rc=%d\n", rc);
+	}
+
 	rc = component_add(&pdev->dev, &dp_display_comp_ops);
 	if (rc) {
 		DRM_ERROR("component add failed, rc=%d\n", rc);
@@ -1306,11 +1333,14 @@ 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_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	pm_runtime_put_sync_suspend(&pdev->dev);
 
+	dp_display_deinit_sub_modules(dp);
+
 	return 0;
 }
 
@@ -1514,31 +1544,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
@@ -1551,17 +1560,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 v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Dmitry Baryshkov]

On 08/07/2023 02:52, Kuogee Hsieh wrote:
> Incorporating pm runtime framework into DP driver so that power
> and clock resource handling can be centralized allowing easier
> control of these resources in preparation of registering aux bus
> uring probe.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>   drivers/gpu/drm/msm/dp/dp_display.c | 75 +++++++++++++++++++++++++++++--------
>   2 files changed, 63 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 8e3b677..c592064 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>   		return -EINVAL;
>   	}
>   
> +	pm_runtime_get_sync(dp_aux->dev);

Let me quote the function's documentation:
Consider using pm_runtime_resume_and_get() instead of it, especially if 
its return value is checked by the caller, as this is likely to result 
in cleaner code.

So two notes concerning the whole patch:
- error checking is missing
- please use pm_runtime_resume_and_get() instead.

>   	mutex_lock(&aux->mutex);
>   	if (!aux->initted) {
>   		ret = -EIO;
> @@ -364,6 +365,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 76f1395..2c5706a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, struct device *master,
>   		goto end;
>   	}
>   
> +	pm_runtime_enable(dev);

devm_pm_runtime_enable() removes need for a cleanup.

> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);

Why do you want to use autosuspend here?

> +
>   	return 0;
>   end:
>   	return rc;
> @@ -320,9 +324,8 @@ 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);
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
>   
>   	kthread_stop(dp->ev_tsk);
>   
> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct dp_display_private *dp)
>   		dp->dp_display.connector_type, dp->core_initialized,
>   		dp->phy_initialized);
>   
> -	dp_power_init(dp->power);
> -	dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
> -	dp_aux_init(dp->aux);
> -	dp->core_initialized = true;
> +	if (!dp->core_initialized) {
> +		dp_power_init(dp->power);
> +		dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
> +		dp_aux_init(dp->aux);
> +		dp->core_initialized = true;
> +	}

Is this relevant to PM runtime? I don't think so.

>   }
>   
>   static void dp_display_host_deinit(struct dp_display_private *dp)
> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>   		dp->dp_display.connector_type, dp->core_initialized,
>   		dp->phy_initialized);
>   
> -	dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
> -	dp_aux_deinit(dp->aux);
> -	dp_power_deinit(dp->power);
> -	dp->core_initialized = false;
> +	if (dp->core_initialized) {
> +		dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
> +		dp_aux_deinit(dp->aux);
> +		dp_power_deinit(dp->power);
> +		dp->core_initialized = false;
> +	}
>   }
>   
>   static int dp_display_usbpd_configure_cb(struct device *dev)
> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct platform_device *pdev)
>   	dp_display_deinit_sub_modules(dp);
>   
>   	platform_set_drvdata(pdev, NULL);
> +	pm_runtime_put_sync_suspend(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static int dp_pm_runtime_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);
> +	struct dp_display_private *dp;
> +
> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> +	dp_display_host_phy_exit(dp);
> +	dp_catalog_ctrl_hpd_enable(dp->catalog);

What? NO!

> +	dp_display_host_deinit(dp);
> +
> +	return 0;
> +}
> +
> +static int dp_pm_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);
> +	struct dp_display_private *dp;
> +
> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> +	dp_display_host_init(dp);
> +	if (dp_display->is_edp) {
> +		dp_catalog_ctrl_hpd_enable(dp->catalog);
> +		dp_display_host_phy_init(dp);
> +	}
>   
>   	return 0;
>   }
> @@ -1409,6 +1449,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,

With the runtime PM in place, can we change suspend/resume to use 
pm_runtime_force_suspend() and pm_runtime_force_resume() ?


>   };
> @@ -1493,10 +1534,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);

Are you going to populate the AUX bus (which can cause AUX bus access) 
without waking up the device?

> -
>   		/*
>   		 * The code below assumes that the panel will finish probing
>   		 * by the time devm_of_dp_aux_populate_ep_devices() returns.
> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>   		dp_hpd_plug_handle(dp_display, 0);

Nearly the same question. Resume device before accessing registers.

>   
>   	mutex_lock(&dp_display->event_mutex);
> +	pm_runtime_get_sync(&dp_display->pdev->dev);
>   
>   	state = dp_display->hpd_state;
>   	if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
> @@ -1684,6 +1722,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);
>   }
>   
> @@ -1723,6 +1763,8 @@ 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);
> +	pm_runtime_get_sync(&dp->pdev->dev);
> +
>   	dp_catalog_ctrl_hpd_enable(dp->catalog);
>   
>   	/* enable HDP interrupts */
> @@ -1744,6 +1786,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);
>   }
>   

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Dmitry Baryshkov]

On 08/07/2023 02:52, Kuogee Hsieh wrote:
> Since both pm_runtime_resume() and pm_runtime_suspend() are not
> populated at dp_pm_ops. Those pm_runtime_get/put() functions within
> dp_power.c will not have any effects in addition to increase/decrease
> power counter.

Lie.

> Also pm_runtime_xxx() should be executed at top
> layer.

Why?

> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_power.c | 9 ---------
>   1 file changed, 9 deletions(-)
> 
> 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;
>   }
>   

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/5] drm/msm/dp: delete EV_HPD_INIT_SETUP

[Dmitry Baryshkov]

On 08/07/2023 02:52, Kuogee Hsieh 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.
> Lets get rid of it.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 12 ------------
>   1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 2c5706a..44580c2 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,
> @@ -1119,9 +1118,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:
> -			dp_display_host_init(dp_priv);
> -			break;
>   		case EV_HPD_PLUG_INT:
>   			dp_hpd_plug_handle(dp_priv, todo->data);
>   			break;
> @@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void)
>   
>   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);
>   }

Why do you keep an empty function?

>   
>   bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)

-- 
With best wishes
Dmitry

Re: [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Dmitry Baryshkov]

On 08/07/2023 02:52, Kuogee Hsieh wrote:
> In preparation of moving edp of_dp_aux_populate_bus() to
> dp_display_probe(), move dp_display_request_irq(),
> dp->parser->parse() and dp_power_client_init() to dp_display_probe()
> too.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++--------------------
>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>   2 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 44580c2..185f1eb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -290,12 +290,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");
> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>   		goto error;
>   	}
>   
> +	rc = dp->parser->parse(dp->parser);
> +	if (rc) {
> +		DRM_ERROR("device tree parsing failed\n");
> +		goto error;
> +	}
> +
>   	dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>   	if (IS_ERR(dp->catalog)) {
>   		rc = PTR_ERR(dp->catalog);
> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>   		goto error;
>   	}
>   
> +	rc = dp_power_client_init(dp->power);
> +	if (rc) {
> +		DRM_ERROR("Power client create failed\n");
> +		goto error;
> +	}
> +
>   	dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>   	if (IS_ERR(dp->aux)) {
>   		rc = PTR_ERR(dp->aux);
> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
> +		if (!dp->irq) {
> +			DRM_ERROR("failed to get irq\n");
> +			return -EINVAL;
> +		}
>   	}

Use platform_get_irq() from probe() function.

>   
> -	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",
> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, &dp->dp_display);
>   
> +	dp_display_request_irq(dp);
> +

Error checking?
Are we completely ready to handle interrupts at this point?

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

-- 
With best wishes
Dmitry

Re: [PATCH v1 5/5] drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP

[Dmitry Baryshkov]

On 08/07/2023 02:52, Kuogee Hsieh wrote:
> Move of_dp_aux_populate_bus() to dp_display_probe() for eDP
> from dp_display_bind() so that probe deferral cases can be
> handled effectively
> 
> 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, 65 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index c592064..c1baffb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -505,6 +505,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)
>   {
> @@ -528,6 +543,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 185f1eb..7ed4bea 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>   		goto end;
>   	}
>   
> -	pm_runtime_enable(dev);
> -	pm_runtime_set_autosuspend_delay(dev, 1000);
> -	pm_runtime_use_autosuspend(dev);
> -
>   	return 0;
>   end:
>   	return rc;
> @@ -322,8 +318,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);
> @@ -1245,6 +1239,29 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde
>   	return NULL;
>   }
>   
> +static void of_dp_aux_depopulate_bus_void(void *data)
> +{
> +	of_dp_aux_depopulate_bus(data);
> +}
> +
> +static int dp_display_auxbus_emulation(struct dp_display_private *dp)

Why is it called emulation?

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

And here you missed the whole point of why we have been asking for. 
Please add a sensible `done_probing' callback, which will call 
component_add(). This way the DP component will only be registered when 
the panel has been probed. Keeping us from the component binding retries 
and corresponding side effects.

> +
> +		devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void,
> +					 dp->aux);

Useless, it's already handled by the devm_ part of the 
devm_of_dp_aux_populate_bus().

> +	}
> +
> +	return ret;
> +}
> +
>   static int dp_display_probe(struct platform_device *pdev)
>   {
>   	int rc = 0;
> @@ -1290,8 +1307,18 @@ static int dp_display_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, &dp->dp_display);
>   
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> +	pm_runtime_use_autosuspend(&pdev->dev);

Can we have this in probe right from the patch #2?

> +
>   	dp_display_request_irq(dp);
>   
> +	if (dp->dp_display.is_edp) {
> +		rc = dp_display_auxbus_emulation(dp);
> +		if (rc)
> +			DRM_ERROR("eDP aux-bus emulation failed, rc=%d\n", rc);
> +	}
> +
>   	rc = component_add(&pdev->dev, &dp_display_comp_ops);
>   	if (rc) {
>   		DRM_ERROR("component add failed, rc=%d\n", rc);
> @@ -1306,11 +1333,14 @@ 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_dont_use_autosuspend(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
>   	pm_runtime_put_sync_suspend(&pdev->dev);
>   
> +	dp_display_deinit_sub_modules(dp);
> +
>   	return 0;
>   }
>   
> @@ -1514,31 +1544,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
> @@ -1551,17 +1560,9 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>   	if (!dp->is_edp && rc == -ENODEV)
>   		return 0;
>   
> -	if (!rc) {
> +	if (!rc)
>   		dp->next_bridge = dp_priv->parser->next_bridge;
> -		return 0;
> -	}
>   
> -error:
> -	if (dp->is_edp) {
> -		of_dp_aux_depopulate_bus(dp_priv->aux);
> -		dp_display_host_phy_exit(dp_priv);
> -		dp_display_host_deinit(dp_priv);
> -	}
>   	return rc;
>   }
>   

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/5] drm/msm/dp: delete EV_HPD_INIT_SETUP

[Dmitry Baryshkov]

On 08/07/2023 02:52, Kuogee Hsieh 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.
> Lets get rid of it.

And another question. Between patches #2 and #3 we have both INIT_SETUP 
event and the PM doing dp_display_host_init(). Will it work correctly?

> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 12 ------------
>   1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 2c5706a..44580c2 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,
> @@ -1119,9 +1118,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:
> -			dp_display_host_init(dp_priv);
> -			break;
>   		case EV_HPD_PLUG_INT:
>   			dp_hpd_plug_handle(dp_priv, todo->data);
>   			break;
> @@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void)
>   
>   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)

-- 
With best wishes
Dmitry

Re: [PATCH v1 0/5] incorporate pm runtime framework and eDP clean up

[Dmitry Baryshkov]

On 08/07/2023 02:52, Kuogee Hsieh wrote:
> Incorporate pm runtime framework into DP driver and clean up eDP
> by moving of_dp_aux_populate_bus() to probe()

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

> 
> Kuogee Hsieh (5):
>    drm/msm/dp: remove pm_runtime_xxx() from dp_power.c
>    drm/msm/dp: incorporate pm_runtime framework into DP driver
>    drm/msm/dp: delete EV_HPD_INIT_SETUP
>    drm/msm/dp: move relevant dp initialization code from bind() to
>      probe()
>    drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP
> 
>   drivers/gpu/drm/msm/dp/dp_aux.c     |  28 +++++
>   drivers/gpu/drm/msm/dp/dp_display.c | 204 +++++++++++++++++++++---------------
>   drivers/gpu/drm/msm/dp/dp_display.h |   1 -
>   drivers/gpu/drm/msm/dp/dp_power.c   |   9 --
>   4 files changed, 145 insertions(+), 97 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Bjorn Andersson]

On Fri, Jul 07, 2023 at 04:52:19PM -0700, Kuogee Hsieh wrote:
> Since both pm_runtime_resume() and pm_runtime_suspend() are not
> populated at dp_pm_ops. Those pm_runtime_get/put() functions within
> dp_power.c will not have any effects in addition to increase/decrease
> power counter. Also pm_runtime_xxx() should be executed at top
> layer.
> 

Getting/putting the runtime PM state affects the vote for the GDSC. So I
would suggest that you move this after patch 2, to not create a gap in
the git history of lacking GDSC votes.

Regards,
Bjorn

> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_power.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> 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 v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Bjorn Andersson]

On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:
> Incorporating pm runtime framework into DP driver so that power
> and clock resource handling can be centralized allowing easier
> control of these resources in preparation of registering aux bus
> uring probe.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>  drivers/gpu/drm/msm/dp/dp_display.c | 75 +++++++++++++++++++++++++++++--------
>  2 files changed, 63 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 8e3b677..c592064 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>  		return -EINVAL;
>  	}
>  
> +	pm_runtime_get_sync(dp_aux->dev);
>  	mutex_lock(&aux->mutex);
>  	if (!aux->initted) {
>  		ret = -EIO;
> @@ -364,6 +365,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 76f1395..2c5706a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, struct device *master,
>  		goto end;
>  	}
>  
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
>  	return 0;
>  end:
>  	return rc;
> @@ -320,9 +324,8 @@ 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);
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
>  
>  	kthread_stop(dp->ev_tsk);
>  
> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct dp_display_private *dp)
>  		dp->dp_display.connector_type, dp->core_initialized,
>  		dp->phy_initialized);
>  
> -	dp_power_init(dp->power);
> -	dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
> -	dp_aux_init(dp->aux);
> -	dp->core_initialized = true;
> +	if (!dp->core_initialized) {
> +		dp_power_init(dp->power);
> +		dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
> +		dp_aux_init(dp->aux);
> +		dp->core_initialized = true;

There are two cases that queries core_initialized, both of those are
done to avoid accessing the DP block without it first being powered up.
With the introduction of runtime PM, it seems reasonable to just power
up the block in those two code paths (and remove the variable).

> +	}
>  }
>  
>  static void dp_display_host_deinit(struct dp_display_private *dp)
> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>  		dp->dp_display.connector_type, dp->core_initialized,
>  		dp->phy_initialized);
>  
> -	dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
> -	dp_aux_deinit(dp->aux);
> -	dp_power_deinit(dp->power);
> -	dp->core_initialized = false;
> +	if (dp->core_initialized) {
> +		dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
> +		dp_aux_deinit(dp->aux);
> +		dp_power_deinit(dp->power);
> +		dp->core_initialized = false;
> +	}
>  }
>  
>  static int dp_display_usbpd_configure_cb(struct device *dev)
> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct platform_device *pdev)
>  	dp_display_deinit_sub_modules(dp);
>  
>  	platform_set_drvdata(pdev, NULL);
> +	pm_runtime_put_sync_suspend(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static int dp_pm_runtime_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);

platform_get_drvdata() is a wrapper for dev_get_drvdata(&pdev->dev), so
there's no need to resolve the platform_device first...

> +	struct dp_display_private *dp;
> +
> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> +	dp_display_host_phy_exit(dp);
> +	dp_catalog_ctrl_hpd_enable(dp->catalog);
> +	dp_display_host_deinit(dp);
> +
> +	return 0;
> +}
> +
> +static int dp_pm_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);
> +	struct dp_display_private *dp;
> +
> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> +	dp_display_host_init(dp);
> +	if (dp_display->is_edp) {
> +		dp_catalog_ctrl_hpd_enable(dp->catalog);
> +		dp_display_host_phy_init(dp);
> +	}
>  
>  	return 0;
>  }
> @@ -1409,6 +1449,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,
>  };
> @@ -1493,10 +1534,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);

I'm probably just missing it, but how do we get here with the host
powered up and the phy initialized?

> -
>  		/*
>  		 * The code below assumes that the panel will finish probing
>  		 * by the time devm_of_dp_aux_populate_ep_devices() returns.
> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>  		dp_hpd_plug_handle(dp_display, 0);
>  
>  	mutex_lock(&dp_display->event_mutex);
> +	pm_runtime_get_sync(&dp_display->pdev->dev);
>  
>  	state = dp_display->hpd_state;
>  	if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
> @@ -1684,6 +1722,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);
>  }
>  
> @@ -1723,6 +1763,8 @@ 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);
> +	pm_runtime_get_sync(&dp->pdev->dev);
> +
>  	dp_catalog_ctrl_hpd_enable(dp->catalog);
>  
>  	/* enable HDP interrupts */
> @@ -1744,6 +1786,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);
>  }

The runtime_get/put in dp_bridge_hpd_enable() and disable matches my
expectations. But in the case that we have an external HPD source, where
will the power be turned on?

Note that you can test this on your device by routing the HPD GPIO to a
display-connector instance and wiring this to the DP node. In the same
way it's done here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28

Regards,
Bjorn

>  
> -- 
> 2.7.4
> 

Re: [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Bjorn Andersson]

On Fri, Jul 07, 2023 at 04:52:22PM -0700, Kuogee Hsieh wrote:
> In preparation of moving edp of_dp_aux_populate_bus() to
> dp_display_probe(), move dp_display_request_irq(),
> dp->parser->parse() and dp_power_client_init() to dp_display_probe()
> too.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++--------------------
>  drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>  2 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 44580c2..185f1eb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -290,12 +290,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");
> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>  		goto error;
>  	}
>  
> +	rc = dp->parser->parse(dp->parser);

Today dp_init_sub_modules() just allocates memory for all the modules
and ties them together. While I don't fancy this way of structuring
device drivers in Linux, I think it's reasonable to retain that design
for now, and perform the parsing and power initialization in
dp_display_probe().

> +	if (rc) {
> +		DRM_ERROR("device tree parsing failed\n");
> +		goto error;
> +	}
> +
>  	dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>  	if (IS_ERR(dp->catalog)) {
>  		rc = PTR_ERR(dp->catalog);
> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>  		goto error;
>  	}
>  
> +	rc = dp_power_client_init(dp->power);
> +	if (rc) {
> +		DRM_ERROR("Power client create failed\n");
> +		goto error;
> +	}
> +
>  	dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>  	if (IS_ERR(dp->aux)) {
>  		rc = PTR_ERR(dp->aux);
> @@ -1196,26 +1202,20 @@ 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;
> -	}

Love this, but it's unrelated to the rest of the patch.

> -
> -	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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 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,

This is fixing a bug where currently the dp_display_irq_handler()
registration is tied to the DPU device's life cycle, while depending on
resources that are released as the DP device is torn down.

It would be nice if this was not hidden in a patch that claims to just
move calls around.

Regards,
Bjorn

>  			IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
>  	if (rc < 0) {
>  		DRM_ERROR("failed to request IRQ%u: %d\n",
> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, &dp->dp_display);
>  
> +	dp_display_request_irq(dp);
> +
>  	rc = component_add(&pdev->dev, &dp_display_comp_ops);
>  	if (rc) {
>  		DRM_ERROR("component add failed, rc=%d\n", rc);
> @@ -1574,12 +1576,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: [Freedreno] [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Abhinav Kumar]

On 7/7/2023 5:06 PM, Dmitry Baryshkov wrote:
> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>> Since both pm_runtime_resume() and pm_runtime_suspend() are not
>> populated at dp_pm_ops. Those pm_runtime_get/put() functions within
>> dp_power.c will not have any effects in addition to increase/decrease
>> power counter.
> 
> Lie.
> 

Even if the commit text is incorrect, review comments like this are not 
helping the patch nor the author and will just get ignored anyway.

>> Also pm_runtime_xxx() should be executed at top
>> layer.
> 
> Why?
> 

I guess he meant to centralize this around dp_display.c. Will elaborate 
while posting the next rev.

>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_power.c | 9 ---------
>>   1 file changed, 9 deletions(-)
>>
>> 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;
>>   }
> 

Re: [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Abhinav Kumar]

On 7/8/2023 7:34 PM, Bjorn Andersson wrote:
> On Fri, Jul 07, 2023 at 04:52:19PM -0700, Kuogee Hsieh wrote:
>> Since both pm_runtime_resume() and pm_runtime_suspend() are not
>> populated at dp_pm_ops. Those pm_runtime_get/put() functions within
>> dp_power.c will not have any effects in addition to increase/decrease
>> power counter. Also pm_runtime_xxx() should be executed at top
>> layer.
>>
> 
> Getting/putting the runtime PM state affects the vote for the GDSC. So I
> would suggest that you move this after patch 2, to not create a gap in
> the git history of lacking GDSC votes.
> 
> Regards,
> Bjorn
> 

the mdss_dp node has rpmhpd SC7180_CX in its power domains. the parent 
device has the GDSC.

So just so that I understand this right,  the DP's vote on this will 
still count because the parent's power domain wont get collapsed till 
the child PM votes are removed too?

If so, I see your point and yes it will make sense to move this later to 
avoid the gap.

>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_power.c | 9 ---------
>>   1 file changed, 9 deletions(-)
>>
>> 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: [Freedreno] [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Dmitry Baryshkov]

On Sun, 9 Jul 2023 at 20:22, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 7/7/2023 5:06 PM, Dmitry Baryshkov wrote:
> > On 08/07/2023 02:52, Kuogee Hsieh wrote:
> >> Since both pm_runtime_resume() and pm_runtime_suspend() are not
> >> populated at dp_pm_ops. Those pm_runtime_get/put() functions within
> >> dp_power.c will not have any effects in addition to increase/decrease
> >> power counter.
> >
> > Lie.
> >
>
> Even if the commit text is incorrect, review comments like this are not
> helping the patch nor the author and will just get ignored anyway.

The review comment might be overreacting, excuse me. I was really
impressed by the commit message, which contradicts the basic source
code. pm_runtime_get() does a lot more than just increasing the power
counter.

> >> Also pm_runtime_xxx() should be executed at top
> >> layer.
> >
> > Why?
> >
>
> I guess he meant to centralize this around dp_display.c. Will elaborate
> while posting the next rev.
>
> >>
> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_power.c | 9 ---------
> >>   1 file changed, 9 deletions(-)
> >>
> >> 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;
> >>   }
> >



-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Abhinav Kumar]

On 7/9/2023 11:00 AM, Dmitry Baryshkov wrote:
> On Sun, 9 Jul 2023 at 20:22, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 7/7/2023 5:06 PM, Dmitry Baryshkov wrote:
>>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>>> Since both pm_runtime_resume() and pm_runtime_suspend() are not
>>>> populated at dp_pm_ops. Those pm_runtime_get/put() functions within
>>>> dp_power.c will not have any effects in addition to increase/decrease
>>>> power counter.
>>>
>>> Lie.
>>>
>>
>> Even if the commit text is incorrect, review comments like this are not
>> helping the patch nor the author and will just get ignored anyway.
> 
> The review comment might be overreacting, excuse me. I was really
> impressed by the commit message, which contradicts the basic source
> code. pm_runtime_get() does a lot more than just increasing the power
> counter.
> 

It says within dp_power.c. Nonetheless, please let us know what is 
missing in the context of this patch like Bjorn did to make it an 
effective review and we can correct it. In its current form, the review 
comment is adding no value.

>>>> Also pm_runtime_xxx() should be executed at top
>>>> layer.
>>>
>>> Why?
>>>
>>
>> I guess he meant to centralize this around dp_display.c. Will elaborate
>> while posting the next rev.
>>
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/dp/dp_power.c | 9 ---------
>>>>    1 file changed, 9 deletions(-)
>>>>
>>>> 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;
>>>>    }
>>>
> 
> 
> 

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

[Kuogee Hsieh]

On 7/7/2023 5:04 PM, Dmitry Baryshkov wrote:
> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>> Incorporating pm runtime framework into DP driver so that power
>> and clock resource handling can be centralized allowing easier
>> control of these resources in preparation of registering aux bus
>> uring probe.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>>   drivers/gpu/drm/msm/dp/dp_display.c | 75 
>> +++++++++++++++++++++++++++++--------
>>   2 files changed, 63 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 8e3b677..c592064 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>> *dp_aux,
>>           return -EINVAL;
>>       }
>>   +    pm_runtime_get_sync(dp_aux->dev);
>
> Let me quote the function's documentation:
> Consider using pm_runtime_resume_and_get() instead of it, especially 
> if its return value is checked by the caller, as this is likely to 
> result in cleaner code.

pm_runtime_resume_and_get() will call pm_runtime_resume()  every time.

Since aux_transfer is called very frequently, is it just simple to call 
pm_runtiem_get_sync() which will call pm_runtime_reusme() if power 
counter is 0 before increased it.

otherwise it just increase power counter?


>
> So two notes concerning the whole patch:
> - error checking is missing
> - please use pm_runtime_resume_and_get() instead.
>
>>       mutex_lock(&aux->mutex);
>>       if (!aux->initted) {
>>           ret = -EIO;
>> @@ -364,6 +365,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 76f1395..2c5706a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, 
>> struct device *master,
>>           goto end;
>>       }
>>   +    pm_runtime_enable(dev);
>
> devm_pm_runtime_enable() removes need for a cleanup.
>
>> +    pm_runtime_set_autosuspend_delay(dev, 1000);
>> +    pm_runtime_use_autosuspend(dev);
>
> Why do you want to use autosuspend here?
>
>> +
>>       return 0;
>>   end:
>>       return rc;
>> @@ -320,9 +324,8 @@ 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);
>> +    pm_runtime_dont_use_autosuspend(dev);
>> +    pm_runtime_disable(dev);
>>         kthread_stop(dp->ev_tsk);
>>   @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
>> dp_display_private *dp)
>>           dp->dp_display.connector_type, dp->core_initialized,
>>           dp->phy_initialized);
>>   -    dp_power_init(dp->power);
>> -    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>> -    dp_aux_init(dp->aux);
>> -    dp->core_initialized = true;
>> +    if (!dp->core_initialized) {
>> +        dp_power_init(dp->power);
>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>> +        dp_aux_init(dp->aux);
>> +        dp->core_initialized = true;
>> +    }
>
> Is this relevant to PM runtime? I don't think so.
>
>>   }
>>     static void dp_display_host_deinit(struct dp_display_private *dp)
>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
>> dp_display_private *dp)
>>           dp->dp_display.connector_type, dp->core_initialized,
>>           dp->phy_initialized);
>>   -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>> -    dp_aux_deinit(dp->aux);
>> -    dp_power_deinit(dp->power);
>> -    dp->core_initialized = false;
>> +    if (dp->core_initialized) {
>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>> +        dp_aux_deinit(dp->aux);
>> +        dp_power_deinit(dp->power);
>> +        dp->core_initialized = false;
>> +    }
>>   }
>>     static int dp_display_usbpd_configure_cb(struct device *dev)
>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
>> platform_device *pdev)
>>       dp_display_deinit_sub_modules(dp);
>>         platform_set_drvdata(pdev, NULL);
>> +    pm_runtime_put_sync_suspend(&pdev->dev);
>> +
>> +    return 0;
>> +}
>> +
>> +static int dp_pm_runtime_suspend(struct device *dev)
>> +{
>> +    struct platform_device *pdev = to_platform_device(dev);
>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> +    struct dp_display_private *dp;
>> +
>> +    dp = container_of(dp_display, struct dp_display_private, 
>> dp_display);
>> +
>> +    dp_display_host_phy_exit(dp);
>> +    dp_catalog_ctrl_hpd_enable(dp->catalog);
>
> What? NO!
>
>> +    dp_display_host_deinit(dp);
>> +
>> +    return 0;
>> +}
>> +
>> +static int dp_pm_runtime_resume(struct device *dev)
>> +{
>> +    struct platform_device *pdev = to_platform_device(dev);
>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> +    struct dp_display_private *dp;
>> +
>> +    dp = container_of(dp_display, struct dp_display_private, 
>> dp_display);
>> +
>> +    dp_display_host_init(dp);
>> +    if (dp_display->is_edp) {
>> +        dp_catalog_ctrl_hpd_enable(dp->catalog);
>> +        dp_display_host_phy_init(dp);
>> +    }
>>         return 0;
>>   }
>> @@ -1409,6 +1449,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,
>
> With the runtime PM in place, can we change suspend/resume to use 
> pm_runtime_force_suspend() and pm_runtime_force_resume() ?

Let em try if i can move checking device connection status out of 
dp_pm_resume(). it handles external dp panel plugin/unplug during 
suspend cases.

>
>
>>   };
>> @@ -1493,10 +1534,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);
>
> Are you going to populate the AUX bus (which can cause AUX bus access) 
> without waking up the device?

> devm_of_dp_aux_populate_ep_devices() ==>  will call 
> pm_runtiemget_sync() internally which will call pm_runtime_resume() to 
> wake dp driver
>> -
>>           /*
>>            * The code below assumes that the panel will finish probing
>>            * by the time devm_of_dp_aux_populate_ep_devices() returns.
>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge 
>> *drm_bridge,
>>           dp_hpd_plug_handle(dp_display, 0);
>
> Nearly the same question. Resume device before accessing registers.
>
>> mutex_lock(&dp_display->event_mutex);
>> +    pm_runtime_get_sync(&dp_display->pdev->dev);
>>         state = dp_display->hpd_state;
>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>> @@ -1684,6 +1722,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);
>>   }
>>   @@ -1723,6 +1763,8 @@ 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);
>> +    pm_runtime_get_sync(&dp->pdev->dev);
>> +
>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>         /* enable HDP interrupts */
>> @@ -1744,6 +1786,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);
>>   }
>

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

[Kuogee Hsieh]

On 7/8/2023 7:52 PM, Bjorn Andersson wrote:
> On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:
>> Incorporating pm runtime framework into DP driver so that power
>> and clock resource handling can be centralized allowing easier
>> control of these resources in preparation of registering aux bus
>> uring probe.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>>   drivers/gpu/drm/msm/dp/dp_display.c | 75 +++++++++++++++++++++++++++++--------
>>   2 files changed, 63 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 8e3b677..c592064 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>>   		return -EINVAL;
>>   	}
>>   
>> +	pm_runtime_get_sync(dp_aux->dev);
>>   	mutex_lock(&aux->mutex);
>>   	if (!aux->initted) {
>>   		ret = -EIO;
>> @@ -364,6 +365,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 76f1395..2c5706a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, struct device *master,
>>   		goto end;
>>   	}
>>   
>> +	pm_runtime_enable(dev);
>> +	pm_runtime_set_autosuspend_delay(dev, 1000);
>> +	pm_runtime_use_autosuspend(dev);
>> +
>>   	return 0;
>>   end:
>>   	return rc;
>> @@ -320,9 +324,8 @@ 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);
>> +	pm_runtime_dont_use_autosuspend(dev);
>> +	pm_runtime_disable(dev);
>>   
>>   	kthread_stop(dp->ev_tsk);
>>   
>> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct dp_display_private *dp)
>>   		dp->dp_display.connector_type, dp->core_initialized,
>>   		dp->phy_initialized);
>>   
>> -	dp_power_init(dp->power);
>> -	dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>> -	dp_aux_init(dp->aux);
>> -	dp->core_initialized = true;
>> +	if (!dp->core_initialized) {
>> +		dp_power_init(dp->power);
>> +		dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>> +		dp_aux_init(dp->aux);
>> +		dp->core_initialized = true;
> There are two cases that queries core_initialized, both of those are
> done to avoid accessing the DP block without it first being powered up.
> With the introduction of runtime PM, it seems reasonable to just power
> up the block in those two code paths (and remove the variable).
>
>> +	}
>>   }
>>   
>>   static void dp_display_host_deinit(struct dp_display_private *dp)
>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>>   		dp->dp_display.connector_type, dp->core_initialized,
>>   		dp->phy_initialized);
>>   
>> -	dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>> -	dp_aux_deinit(dp->aux);
>> -	dp_power_deinit(dp->power);
>> -	dp->core_initialized = false;
>> +	if (dp->core_initialized) {
>> +		dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>> +		dp_aux_deinit(dp->aux);
>> +		dp_power_deinit(dp->power);
>> +		dp->core_initialized = false;
>> +	}
>>   }
>>   
>>   static int dp_display_usbpd_configure_cb(struct device *dev)
>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct platform_device *pdev)
>>   	dp_display_deinit_sub_modules(dp);
>>   
>>   	platform_set_drvdata(pdev, NULL);
>> +	pm_runtime_put_sync_suspend(&pdev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dp_pm_runtime_suspend(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);
> platform_get_drvdata() is a wrapper for dev_get_drvdata(&pdev->dev), so
> there's no need to resolve the platform_device first...
>
>> +	struct dp_display_private *dp;
>> +
>> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
>> +
>> +	dp_display_host_phy_exit(dp);
>> +	dp_catalog_ctrl_hpd_enable(dp->catalog);
>> +	dp_display_host_deinit(dp);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dp_pm_runtime_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> +	struct dp_display_private *dp;
>> +
>> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
>> +
>> +	dp_display_host_init(dp);
>> +	if (dp_display->is_edp) {
>> +		dp_catalog_ctrl_hpd_enable(dp->catalog);
>> +		dp_display_host_phy_init(dp);
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -1409,6 +1449,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,
>>   };
>> @@ -1493,10 +1534,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);
> I'm probably just missing it, but how do we get here with the host
> powered up and the phy initialized?

if (!dp->core_initialized)  is at dp_display_host_init()

>
>> -
>>   		/*
>>   		 * The code below assumes that the panel will finish probing
>>   		 * by the time devm_of_dp_aux_populate_ep_devices() returns.
>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>>   		dp_hpd_plug_handle(dp_display, 0);
>>   
>>   	mutex_lock(&dp_display->event_mutex);
>> +	pm_runtime_get_sync(&dp_display->pdev->dev);
>>   
>>   	state = dp_display->hpd_state;
>>   	if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>> @@ -1684,6 +1722,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);
>>   }
>>   
>> @@ -1723,6 +1763,8 @@ 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);
>> +	pm_runtime_get_sync(&dp->pdev->dev);
>> +
>>   	dp_catalog_ctrl_hpd_enable(dp->catalog);
>>   
>>   	/* enable HDP interrupts */
>> @@ -1744,6 +1786,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);
>>   }
> The runtime_get/put in dp_bridge_hpd_enable() and disable matches my
> expectations. But in the case that we have an external HPD source, where
> will the power be turned on?
>
> Note that you can test this on your device by routing the HPD GPIO to a
> display-connector instance and wiring this to the DP node. In the same
> way it's done here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28
>
> Regards,
> Bjorn
>
>>   
>> -- 
>> 2.7.4
>>

Re: [PATCH v1 3/5] drm/msm/dp: delete EV_HPD_INIT_SETUP

[Kuogee Hsieh]

On 7/7/2023 5:34 PM, Dmitry Baryshkov wrote:
> On 08/07/2023 02:52, Kuogee Hsieh 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.
>> Lets get rid of it.
>
> And another question. Between patches #2 and #3 we have both 
> INIT_SETUP event and the PM doing dp_display_host_init(). Will it work 
> correctly?

yes,  i had added  if (!dp->core_initialized)  into dp_display_host_init().

should I merge this patch into patch #2?

>
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 12 ------------
>>   1 file changed, 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 2c5706a..44580c2 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,
>> @@ -1119,9 +1118,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:
>> -            dp_display_host_init(dp_priv);
>> -            break;
>>           case EV_HPD_PLUG_INT:
>>               dp_hpd_plug_handle(dp_priv, todo->data);
>>               break;
>> @@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void)
>>     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)
>

Re: [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Kuogee Hsieh]

On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote:
> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>> In preparation of moving edp of_dp_aux_populate_bus() to
>> dp_display_probe(), move dp_display_request_irq(),
>> dp->parser->parse() and dp_power_client_init() to dp_display_probe()
>> too.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 48 
>> +++++++++++++++++--------------------
>>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>>   2 files changed, 22 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 44580c2..185f1eb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -290,12 +290,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");
>> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct 
>> dp_display_private *dp)
>>           goto error;
>>       }
>>   +    rc = dp->parser->parse(dp->parser);
>> +    if (rc) {
>> +        DRM_ERROR("device tree parsing failed\n");
>> +        goto error;
>> +    }
>> +
>>       dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>       if (IS_ERR(dp->catalog)) {
>>           rc = PTR_ERR(dp->catalog);
>> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct 
>> dp_display_private *dp)
>>           goto error;
>>       }
>>   +    rc = dp_power_client_init(dp->power);
>> +    if (rc) {
>> +        DRM_ERROR("Power client create failed\n");
>> +        goto error;
>> +    }
>> +
>>       dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>       if (IS_ERR(dp->aux)) {
>>           rc = PTR_ERR(dp->aux);
>> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
>> +        if (!dp->irq) {
>> +            DRM_ERROR("failed to get irq\n");
>> +            return -EINVAL;
>> +        }
>>       }
>
> Use platform_get_irq() from probe() function.
>
>>   -    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",
>> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct 
>> platform_device *pdev)
>>         platform_set_drvdata(pdev, &dp->dp_display);
>>   +    dp_display_request_irq(dp);
>> +
>
> Error checking?
> Are we completely ready to handle interrupts at this point?
not until dp_display_host_init() is called which will be called from 
pm_runtime_resume() later.
>
>>       rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>       if (rc) {
>>           DRM_ERROR("component add failed, rc=%d\n", rc);
>> @@ -1574,12 +1576,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);
>

Re: [Freedreno] [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Kuogee Hsieh]

On 7/9/2023 1:32 PM, Abhinav Kumar wrote:
>
>
> On 7/9/2023 11:00 AM, Dmitry Baryshkov wrote:
>> On Sun, 9 Jul 2023 at 20:22, Abhinav Kumar 
>> <quic_abhinavk@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 7/7/2023 5:06 PM, Dmitry Baryshkov wrote:
>>>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>>>> Since both pm_runtime_resume() and pm_runtime_suspend() are not
>>>>> populated at dp_pm_ops. Those pm_runtime_get/put() functions within
>>>>> dp_power.c will not have any effects in addition to increase/decrease
>>>>> power counter.
>>>>
>>>> Lie.
>>>>
>>>
>>> Even if the commit text is incorrect, review comments like this are not
>>> helping the patch nor the author and will just get ignored anyway.
>>
>> The review comment might be overreacting, excuse me. I was really
>> impressed by the commit message, which contradicts the basic source
>> code. pm_runtime_get() does a lot more than just increasing the power
>> counter.
>>
>
> It says within dp_power.c. Nonetheless, please let us know what is 
> missing in the context of this patch like Bjorn did to make it an 
> effective review and we can correct it. In its current form, the 
> review comment is adding no value.
>
I am new in pm.

Any recommendation to revise this commit test?

>>>>> Also pm_runtime_xxx() should be executed at top
>>>>> layer.
>>>>
>>>> Why?
>>>>
>>>
>>> I guess he meant to centralize this around dp_display.c. Will elaborate
>>> while posting the next rev.
>>>
>>>>>
>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/dp/dp_power.c | 9 ---------
>>>>>    1 file changed, 9 deletions(-)
>>>>>
>>>>> 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;
>>>>>    }
>>>>
>>
>>
>>

Re: [Freedreno] [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Dmitry Baryshkov]

On 10/07/2023 20:25, Kuogee Hsieh wrote:
> 
> On 7/9/2023 1:32 PM, Abhinav Kumar wrote:
>>
>>
>> On 7/9/2023 11:00 AM, Dmitry Baryshkov wrote:
>>> On Sun, 9 Jul 2023 at 20:22, Abhinav Kumar 
>>> <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 7/7/2023 5:06 PM, Dmitry Baryshkov wrote:
>>>>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>>>>> Since both pm_runtime_resume() and pm_runtime_suspend() are not
>>>>>> populated at dp_pm_ops. Those pm_runtime_get/put() functions within
>>>>>> dp_power.c will not have any effects in addition to increase/decrease
>>>>>> power counter.
>>>>>
>>>>> Lie.
>>>>>
>>>>
>>>> Even if the commit text is incorrect, review comments like this are not
>>>> helping the patch nor the author and will just get ignored anyway.
>>>
>>> The review comment might be overreacting, excuse me. I was really
>>> impressed by the commit message, which contradicts the basic source
>>> code. pm_runtime_get() does a lot more than just increasing the power
>>> counter.
>>>
>>
>> It says within dp_power.c. Nonetheless, please let us know what is 
>> missing in the context of this patch like Bjorn did to make it an 
>> effective review and we can correct it. In its current form, the 
>> review comment is adding no value.
>>
> I am new in pm.
> 
> Any recommendation to revise this commit test?

I'd say, squash patches 1 and 2 and then state in the commit message 
that you are changing pm_runtime code paths because you want to power up 
the device from the runtime callbacks rather than just waking up the 
device in the power up path.

Generally it is much easier to justify changing from A to B rather than 
just dropping A and then adding B.

> 
>>>>>> Also pm_runtime_xxx() should be executed at top
>>>>>> layer.
>>>>>
>>>>> Why?
>>>>>
>>>>
>>>> I guess he meant to centralize this around dp_display.c. Will elaborate
>>>> while posting the next rev.
>>>>
>>>>>>
>>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/msm/dp/dp_power.c | 9 ---------
>>>>>>    1 file changed, 9 deletions(-)
>>>>>>
>>>>>> 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;
>>>>>>    }
>>>>>
>>>
>>>
>>>

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 10/07/2023 19:18, Kuogee Hsieh wrote:
> 
> On 7/7/2023 5:04 PM, Dmitry Baryshkov wrote:
>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>> Incorporating pm runtime framework into DP driver so that power
>>> and clock resource handling can be centralized allowing easier
>>> control of these resources in preparation of registering aux bus
>>> uring probe.
>>>
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>>>   drivers/gpu/drm/msm/dp/dp_display.c | 75 
>>> +++++++++++++++++++++++++++++--------
>>>   2 files changed, 63 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> index 8e3b677..c592064 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>>> *dp_aux,
>>>           return -EINVAL;
>>>       }
>>>   +    pm_runtime_get_sync(dp_aux->dev);
>>
>> Let me quote the function's documentation:
>> Consider using pm_runtime_resume_and_get() instead of it, especially 
>> if its return value is checked by the caller, as this is likely to 
>> result in cleaner code.
> 
> pm_runtime_resume_and_get() will call pm_runtime_resume()  every time.
> 
> Since aux_transfer is called very frequently, is it just simple to call 
> pm_runtiem_get_sync() which will call pm_runtime_reusme() if power 
> counter is 0 before increased it.
> 
> otherwise it just increase power counter?

As you are adding meaningful runtime PM calls, you have to add error 
checking to these calls. Just calling pm_runtime_get_sync() is not enough.

And once you add error handling, you will see what is the difference 
between two mentioned functions and why one is suggested to be used 
instead of the other one.

> 
> 
>>
>> So two notes concerning the whole patch:
>> - error checking is missing
>> - please use pm_runtime_resume_and_get() instead.
>>
>>>       mutex_lock(&aux->mutex);
>>>       if (!aux->initted) {
>>>           ret = -EIO;
>>> @@ -364,6 +365,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 76f1395..2c5706a 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, 
>>> struct device *master,
>>>           goto end;
>>>       }
>>>   +    pm_runtime_enable(dev);
>>
>> devm_pm_runtime_enable() removes need for a cleanup.
>>
>>> +    pm_runtime_set_autosuspend_delay(dev, 1000);
>>> +    pm_runtime_use_autosuspend(dev);
>>
>> Why do you want to use autosuspend here?
>>
>>> +
>>>       return 0;
>>>   end:
>>>       return rc;
>>> @@ -320,9 +324,8 @@ 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);
>>> +    pm_runtime_dont_use_autosuspend(dev);
>>> +    pm_runtime_disable(dev);
>>>         kthread_stop(dp->ev_tsk);
>>>   @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
>>> dp_display_private *dp)
>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>           dp->phy_initialized);
>>>   -    dp_power_init(dp->power);
>>> -    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>> -    dp_aux_init(dp->aux);
>>> -    dp->core_initialized = true;
>>> +    if (!dp->core_initialized) {
>>> +        dp_power_init(dp->power);
>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>> +        dp_aux_init(dp->aux);
>>> +        dp->core_initialized = true;
>>> +    }
>>
>> Is this relevant to PM runtime? I don't think so.
>>
>>>   }
>>>     static void dp_display_host_deinit(struct dp_display_private *dp)
>>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
>>> dp_display_private *dp)
>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>           dp->phy_initialized);
>>>   -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>> -    dp_aux_deinit(dp->aux);
>>> -    dp_power_deinit(dp->power);
>>> -    dp->core_initialized = false;
>>> +    if (dp->core_initialized) {
>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>> +        dp_aux_deinit(dp->aux);
>>> +        dp_power_deinit(dp->power);
>>> +        dp->core_initialized = false;
>>> +    }
>>>   }
>>>     static int dp_display_usbpd_configure_cb(struct device *dev)
>>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
>>> platform_device *pdev)
>>>       dp_display_deinit_sub_modules(dp);
>>>         platform_set_drvdata(pdev, NULL);
>>> +    pm_runtime_put_sync_suspend(&pdev->dev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int dp_pm_runtime_suspend(struct device *dev)
>>> +{
>>> +    struct platform_device *pdev = to_platform_device(dev);
>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>> +    struct dp_display_private *dp;
>>> +
>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>> dp_display);
>>> +
>>> +    dp_display_host_phy_exit(dp);
>>> +    dp_catalog_ctrl_hpd_enable(dp->catalog);
>>
>> What? NO!
>>
>>> +    dp_display_host_deinit(dp);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int dp_pm_runtime_resume(struct device *dev)
>>> +{
>>> +    struct platform_device *pdev = to_platform_device(dev);
>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>> +    struct dp_display_private *dp;
>>> +
>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>> dp_display);
>>> +
>>> +    dp_display_host_init(dp);
>>> +    if (dp_display->is_edp) {
>>> +        dp_catalog_ctrl_hpd_enable(dp->catalog);
>>> +        dp_display_host_phy_init(dp);
>>> +    }
>>>         return 0;
>>>   }
>>> @@ -1409,6 +1449,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,
>>
>> With the runtime PM in place, can we change suspend/resume to use 
>> pm_runtime_force_suspend() and pm_runtime_force_resume() ?
> 
> Let em try if i can move checking device connection status out of 
> dp_pm_resume(). it handles external dp panel plugin/unplug during 
> suspend cases.

As we discussed during the telco, it does it in a very strange way, 
which is not compatible with the external HPD support. Thus it should be 
rewritten anyway. Yes, this change can be handled in a separate patch,

What bugs me here is that I don't see runtime PM calls in the device's 
suspend/resume path. Shouldn't you put the runtime PM status in suspend 
path?

> 
>>
>>
>>>   };
>>> @@ -1493,10 +1534,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);
>>
>> Are you going to populate the AUX bus (which can cause AUX bus access) 
>> without waking up the device?
> 
>> devm_of_dp_aux_populate_ep_devices() ==>  will call 
>> pm_runtiemget_sync() internally which will call pm_runtime_resume() to 
>> wake dp driver
>>> -
>>>           /*
>>>            * The code below assumes that the panel will finish probing
>>>            * by the time devm_of_dp_aux_populate_ep_devices() returns.
>>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge 
>>> *drm_bridge,
>>>           dp_hpd_plug_handle(dp_display, 0);
>>
>> Nearly the same question. Resume device before accessing registers.
>>
>>> mutex_lock(&dp_display->event_mutex);
>>> +    pm_runtime_get_sync(&dp_display->pdev->dev);
>>>         state = dp_display->hpd_state;
>>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>>> @@ -1684,6 +1722,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);
>>>   }
>>>   @@ -1723,6 +1763,8 @@ 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);
>>> +    pm_runtime_get_sync(&dp->pdev->dev);
>>> +
>>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>         /* enable HDP interrupts */
>>> @@ -1744,6 +1786,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);
>>>   }
>>

-- 
With best wishes
Dmitry

Re: [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Dmitry Baryshkov]

On 10/07/2023 19:57, Kuogee Hsieh wrote:
> 
> On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote:
>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>> In preparation of moving edp of_dp_aux_populate_bus() to
>>> dp_display_probe(), move dp_display_request_irq(),
>>> dp->parser->parse() and dp_power_client_init() to dp_display_probe()
>>> too.
>>>
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_display.c | 48 
>>> +++++++++++++++++--------------------
>>>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>>>   2 files changed, 22 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index 44580c2..185f1eb 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -290,12 +290,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");
>>> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct 
>>> dp_display_private *dp)
>>>           goto error;
>>>       }
>>>   +    rc = dp->parser->parse(dp->parser);
>>> +    if (rc) {
>>> +        DRM_ERROR("device tree parsing failed\n");
>>> +        goto error;
>>> +    }
>>> +
>>>       dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>>       if (IS_ERR(dp->catalog)) {
>>>           rc = PTR_ERR(dp->catalog);
>>> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct 
>>> dp_display_private *dp)
>>>           goto error;
>>>       }
>>>   +    rc = dp_power_client_init(dp->power);
>>> +    if (rc) {
>>> +        DRM_ERROR("Power client create failed\n");
>>> +        goto error;
>>> +    }
>>> +
>>>       dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>>       if (IS_ERR(dp->aux)) {
>>>           rc = PTR_ERR(dp->aux);
>>> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
>>> +        if (!dp->irq) {
>>> +            DRM_ERROR("failed to get irq\n");
>>> +            return -EINVAL;
>>> +        }
>>>       }
>>
>> Use platform_get_irq() from probe() function.
>>
>>>   -    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",
>>> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct 
>>> platform_device *pdev)
>>>         platform_set_drvdata(pdev, &dp->dp_display);
>>>   +    dp_display_request_irq(dp);
>>> +
>>
>> Error checking?
>> Are we completely ready to handle interrupts at this point?
> not until dp_display_host_init() is called which will be called from 
> pm_runtime_resume() later.

But once you request_irq(), you should be ready for the IRQs to be 
delivered right away.

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

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/5] drm/msm/dp: delete EV_HPD_INIT_SETUP

[Dmitry Baryshkov]

On 10/07/2023 19:52, Kuogee Hsieh wrote:
> 
> On 7/7/2023 5:34 PM, Dmitry Baryshkov wrote:
>> On 08/07/2023 02:52, Kuogee Hsieh 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.
>>> Lets get rid of it.
>>
>> And another question. Between patches #2 and #3 we have both 
>> INIT_SETUP event and the PM doing dp_display_host_init(). Will it work 
>> correctly?
> 
> yes,  i had added  if (!dp->core_initialized)  into dp_display_host_init().
> 
> should I merge this patch into patch #2?

You can remove a call to dp_display_host_init() in patch #2 and then 
drop EV_HOST_INIT / msm_dp_irq_postinstall() here.

> 
>>
>>>
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_display.c | 12 ------------
>>>   1 file changed, 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index 2c5706a..44580c2 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,
>>> @@ -1119,9 +1118,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:
>>> -            dp_display_host_init(dp_priv);
>>> -            break;
>>>           case EV_HPD_PLUG_INT:
>>>               dp_hpd_plug_handle(dp_priv, todo->data);
>>>               break;
>>> @@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void)
>>>     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)
>>

-- 
With best wishes
Dmitry

Re: [PATCH v1 5/5] drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP

[Dmitry Baryshkov]

[Restored CC list]

On Mon, 10 Jul 2023 at 20:08, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 7/7/2023 5:32 PM, Dmitry Baryshkov wrote:
> > On 08/07/2023 02:52, Kuogee Hsieh wrote:
> >> Move of_dp_aux_populate_bus() to dp_display_probe() for eDP
> >> from dp_display_bind() so that probe deferral cases can be
> >> handled effectively
> >>
> >> 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, 65 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> >> b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> index c592064..c1baffb 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> @@ -505,6 +505,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)
> >>   {
> >> @@ -528,6 +543,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 185f1eb..7ed4bea 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev,
> >> struct device *master,
> >>           goto end;
> >>       }
> >>   -    pm_runtime_enable(dev);
> >> -    pm_runtime_set_autosuspend_delay(dev, 1000);
> >> -    pm_runtime_use_autosuspend(dev);
> >> -
> >>       return 0;
> >>   end:
> >>       return rc;
> >> @@ -322,8 +318,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);
> >> @@ -1245,6 +1239,29 @@ static const struct msm_dp_desc
> >> *dp_display_get_desc(struct platform_device *pde
> >>       return NULL;
> >>   }
> >>   +static void of_dp_aux_depopulate_bus_void(void *data)
> >> +{
> >> +    of_dp_aux_depopulate_bus(data);
> >> +}
> >> +
> >> +static int dp_display_auxbus_emulation(struct dp_display_private *dp)
> >
> > Why is it called emulation?
> >
> >> +{
> >> +    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, NULL);
> >
> > And here you missed the whole point of why we have been asking for.
> > Please add a sensible `done_probing' callback, which will call
> > component_add(). This way the DP component will only be registered
> > when the panel has been probed. Keeping us from the component binding
> > retries and corresponding side effects.
> >
> >> +
> >> +        devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void,
> >> +                     dp->aux);
> >
> > Useless, it's already handled by the devm_ part of the
> > devm_of_dp_aux_populate_bus().
> >
> >> +    }
> >> +
> >> +    return ret;
> >> +}
> >> +
> >>   static int dp_display_probe(struct platform_device *pdev)
> >>   {
> >>       int rc = 0;
> >> @@ -1290,8 +1307,18 @@ static int dp_display_probe(struct
> >> platform_device *pdev)
> >>         platform_set_drvdata(pdev, &dp->dp_display);
> >>   +    pm_runtime_enable(&pdev->dev);
> >> +    pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >> +    pm_runtime_use_autosuspend(&pdev->dev);
> >
> > Can we have this in probe right from the patch #2?
>
> no, at patch#2, devm_of_dp_aux_populate_bus() is done ta bind timing.
>
> The device used by pm_runtime_get_sync() of generic_edp_panel_probe()
> which is derived from devm_of_dp_aux_populate_bus() is different the
> &pdev->dev here.

Excuse me, I don't get your answer. In patch #2 you have added
pm_runtime_enable() / etc to dp_display_bind().
In this patch you are moving these calls to dp_display_probe(). I
think that the latter is a better place for enabling runtime PM and as
such I've asked you to squash this chunk into patch #2.
Why isn't that going to work?

If I'm not mistaken here, the panel's call to pm_runtime_get_sync()
will wake up the panel and all the parent devices, including the DP.
That's what I meant in my comment regarding PM calls in the patch #1.
pm_runtime_get_sync() / resume() / etc. do not only increase the
runtime PM count. They do other things to parent devices, linked
devices, etc.

>
> >
> >> +
> >>       dp_display_request_irq(dp);
> >>   +    if (dp->dp_display.is_edp) {
> >> +        rc = dp_display_auxbus_emulation(dp);
> >> +        if (rc)
> >> +            DRM_ERROR("eDP aux-bus emulation failed, rc=%d\n", rc);
> >> +    }
> >> +
> >>       rc = component_add(&pdev->dev, &dp_display_comp_ops);
> >>       if (rc) {
> >>           DRM_ERROR("component add failed, rc=%d\n", rc);
> >> @@ -1306,11 +1333,14 @@ 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_dont_use_autosuspend(&pdev->dev);
> >> +    pm_runtime_disable(&pdev->dev);
> >>       pm_runtime_put_sync_suspend(&pdev->dev);
> >>   +    dp_display_deinit_sub_modules(dp);
> >> +
> >>       return 0;
> >>   }
> >>   @@ -1514,31 +1544,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
> >> @@ -1551,17 +1560,9 @@ static int dp_display_get_next_bridge(struct
> >> msm_dp *dp)
> >>       if (!dp->is_edp && rc == -ENODEV)
> >>           return 0;
> >>   -    if (!rc) {
> >> +    if (!rc)
> >>           dp->next_bridge = dp_priv->parser->next_bridge;
> >> -        return 0;
> >> -    }
> >>   -error:
> >> -    if (dp->is_edp) {
> >> -        of_dp_aux_depopulate_bus(dp_priv->aux);
> >> -        dp_display_host_phy_exit(dp_priv);
> >> -        dp_display_host_deinit(dp_priv);
> >> -    }
> >>       return rc;
> >>   }
> >



-- 
With best wishes
Dmitry

Re: [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Kuogee Hsieh]

On 7/10/2023 11:13 AM, Dmitry Baryshkov wrote:
> On 10/07/2023 19:57, Kuogee Hsieh wrote:
>>
>> On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote:
>>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>>> In preparation of moving edp of_dp_aux_populate_bus() to
>>>> dp_display_probe(), move dp_display_request_irq(),
>>>> dp->parser->parse() and dp_power_client_init() to dp_display_probe()
>>>> too.
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dp/dp_display.c | 48 
>>>> +++++++++++++++++--------------------
>>>>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>>>>   2 files changed, 22 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index 44580c2..185f1eb 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -290,12 +290,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");
>>>> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct 
>>>> dp_display_private *dp)
>>>>           goto error;
>>>>       }
>>>>   +    rc = dp->parser->parse(dp->parser);
>>>> +    if (rc) {
>>>> +        DRM_ERROR("device tree parsing failed\n");
>>>> +        goto error;
>>>> +    }
>>>> +
>>>>       dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>>>       if (IS_ERR(dp->catalog)) {
>>>>           rc = PTR_ERR(dp->catalog);
>>>> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct 
>>>> dp_display_private *dp)
>>>>           goto error;
>>>>       }
>>>>   +    rc = dp_power_client_init(dp->power);
>>>> +    if (rc) {
>>>> +        DRM_ERROR("Power client create failed\n");
>>>> +        goto error;
>>>> +    }
>>>> +
>>>>       dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>>>       if (IS_ERR(dp->aux)) {
>>>>           rc = PTR_ERR(dp->aux);
>>>> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
>>>> +        if (!dp->irq) {
>>>> +            DRM_ERROR("failed to get irq\n");
>>>> +            return -EINVAL;
>>>> +        }
>>>>       }
>>>
>>> Use platform_get_irq() from probe() function.
>>>
>>>>   -    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",
>>>> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct 
>>>> platform_device *pdev)
>>>>         platform_set_drvdata(pdev, &dp->dp_display);
>>>>   +    dp_display_request_irq(dp);
>>>> +
>>>
>>> Error checking?
>>> Are we completely ready to handle interrupts at this point?
>> not until dp_display_host_init() is called which will be called from 
>> pm_runtime_resume() later.
>
> But once you request_irq(), you should be ready for the IRQs to be 
> delivered right away.

At this point, the DP controller interrupts mask bit is not enabled yet.

Therefore interrupts will not happen until dp_bridge_hpd_enable() is 
called to initialize dp host  controller and then enabled mask bits.

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

Re: [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Dmitry Baryshkov]

On 17/07/2023 20:16, Kuogee Hsieh wrote:
> 
> On 7/10/2023 11:13 AM, Dmitry Baryshkov wrote:
>> On 10/07/2023 19:57, Kuogee Hsieh wrote:
>>>
>>> On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote:
>>>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>>>> In preparation of moving edp of_dp_aux_populate_bus() to
>>>>> dp_display_probe(), move dp_display_request_irq(),
>>>>> dp->parser->parse() and dp_power_client_init() to dp_display_probe()
>>>>> too.
>>>>>
>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/dp/dp_display.c | 48 
>>>>> +++++++++++++++++--------------------
>>>>>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>>>>>   2 files changed, 22 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> index 44580c2..185f1eb 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> @@ -290,12 +290,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");
>>>>> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct 
>>>>> dp_display_private *dp)
>>>>>           goto error;
>>>>>       }
>>>>>   +    rc = dp->parser->parse(dp->parser);
>>>>> +    if (rc) {
>>>>> +        DRM_ERROR("device tree parsing failed\n");
>>>>> +        goto error;
>>>>> +    }
>>>>> +
>>>>>       dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>>>>       if (IS_ERR(dp->catalog)) {
>>>>>           rc = PTR_ERR(dp->catalog);
>>>>> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct 
>>>>> dp_display_private *dp)
>>>>>           goto error;
>>>>>       }
>>>>>   +    rc = dp_power_client_init(dp->power);
>>>>> +    if (rc) {
>>>>> +        DRM_ERROR("Power client create failed\n");
>>>>> +        goto error;
>>>>> +    }
>>>>> +
>>>>>       dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>>>>       if (IS_ERR(dp->aux)) {
>>>>>           rc = PTR_ERR(dp->aux);
>>>>> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
>>>>> +        if (!dp->irq) {
>>>>> +            DRM_ERROR("failed to get irq\n");
>>>>> +            return -EINVAL;
>>>>> +        }
>>>>>       }
>>>>
>>>> Use platform_get_irq() from probe() function.
>>>>
>>>>>   -    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",
>>>>> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct 
>>>>> platform_device *pdev)
>>>>>         platform_set_drvdata(pdev, &dp->dp_display);
>>>>>   +    dp_display_request_irq(dp);
>>>>> +
>>>>
>>>> Error checking?
>>>> Are we completely ready to handle interrupts at this point?
>>> not until dp_display_host_init() is called which will be called from 
>>> pm_runtime_resume() later.
>>
>> But once you request_irq(), you should be ready for the IRQs to be 
>> delivered right away.
> 
> At this point, the DP controller interrupts mask bit is not enabled yet.
> 
> Therefore interrupts will not happen until dp_bridge_hpd_enable() is 
> called to initialize dp host  controller and then enabled mask bits.

Are AUX and CTRL interrupts also disabled? What about any stray/pending 
interrupts? Just take it as a rule of thumb. Once request_irq() has been 
called without the IRQ_NOAUTOEN flag, the driver should be prepared to 
handle the incoming interrupt requests.

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

-- 
With best wishes
Dmitry

Re: [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Kuogee Hsieh]

On 7/17/2023 10:22 AM, Dmitry Baryshkov wrote:
> On 17/07/2023 20:16, Kuogee Hsieh wrote:
>>
>> On 7/10/2023 11:13 AM, Dmitry Baryshkov wrote:
>>> On 10/07/2023 19:57, Kuogee Hsieh wrote:
>>>>
>>>> On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote:
>>>>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>>>>> In preparation of moving edp of_dp_aux_populate_bus() to
>>>>>> dp_display_probe(), move dp_display_request_irq(),
>>>>>> dp->parser->parse() and dp_power_client_init() to dp_display_probe()
>>>>>> too.
>>>>>>
>>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/msm/dp/dp_display.c | 48 
>>>>>> +++++++++++++++++--------------------
>>>>>>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>>>>>>   2 files changed, 22 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> index 44580c2..185f1eb 100644
>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> @@ -290,12 +290,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");
>>>>>> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct 
>>>>>> dp_display_private *dp)
>>>>>>           goto error;
>>>>>>       }
>>>>>>   +    rc = dp->parser->parse(dp->parser);
>>>>>> +    if (rc) {
>>>>>> +        DRM_ERROR("device tree parsing failed\n");
>>>>>> +        goto error;
>>>>>> +    }
>>>>>> +
>>>>>>       dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>>>>>       if (IS_ERR(dp->catalog)) {
>>>>>>           rc = PTR_ERR(dp->catalog);
>>>>>> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct 
>>>>>> dp_display_private *dp)
>>>>>>           goto error;
>>>>>>       }
>>>>>>   +    rc = dp_power_client_init(dp->power);
>>>>>> +    if (rc) {
>>>>>> +        DRM_ERROR("Power client create failed\n");
>>>>>> +        goto error;
>>>>>> +    }
>>>>>> +
>>>>>>       dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>>>>>       if (IS_ERR(dp->aux)) {
>>>>>>           rc = PTR_ERR(dp->aux);
>>>>>> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
>>>>>> +        if (!dp->irq) {
>>>>>> +            DRM_ERROR("failed to get irq\n");
>>>>>> +            return -EINVAL;
>>>>>> +        }
>>>>>>       }
>>>>>
>>>>> Use platform_get_irq() from probe() function.
>>>>>
>>>>>>   -    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",
>>>>>> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct 
>>>>>> platform_device *pdev)
>>>>>>         platform_set_drvdata(pdev, &dp->dp_display);
>>>>>>   +    dp_display_request_irq(dp);
>>>>>> +
>>>>>
>>>>> Error checking?
>>>>> Are we completely ready to handle interrupts at this point?
>>>> not until dp_display_host_init() is called which will be called 
>>>> from pm_runtime_resume() later.
>>>
>>> But once you request_irq(), you should be ready for the IRQs to be 
>>> delivered right away.
>>
>> At this point, the DP controller interrupts mask bit is not enabled yet.
>>
>> Therefore interrupts will not happen until dp_bridge_hpd_enable() is 
>> called to initialize dp host controller and then enabled mask bits.
>
> Are AUX and CTRL interrupts also disabled? What about any 
> stray/pending interrupts? Just take it as a rule of thumb. Once 
> request_irq() has been called without the IRQ_NOAUTOEN flag, the 
> driver should be prepared to handle the incoming interrupt requests.

yes, both AUX and CTRL are disabled.

edp population do need irq to handle aux transfer during probe.

it should work by checking core_initialized flag at irq handle to filter 
out stray/pending interrupts.

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

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

[Kuogee Hsieh]

On 7/7/2023 5:04 PM, Dmitry Baryshkov wrote:
> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>> Incorporating pm runtime framework into DP driver so that power
>> and clock resource handling can be centralized allowing easier
>> control of these resources in preparation of registering aux bus
>> uring probe.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>>   drivers/gpu/drm/msm/dp/dp_display.c | 75 
>> +++++++++++++++++++++++++++++--------
>>   2 files changed, 63 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 8e3b677..c592064 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>> *dp_aux,
>>           return -EINVAL;
>>       }
>>   +    pm_runtime_get_sync(dp_aux->dev);
>
> Let me quote the function's documentation:
> Consider using pm_runtime_resume_and_get() instead of it, especially 
> if its return value is checked by the caller, as this is likely to 
> result in cleaner code.
>
> So two notes concerning the whole patch:
> - error checking is missing
> - please use pm_runtime_resume_and_get() instead.
>
>>       mutex_lock(&aux->mutex);
>>       if (!aux->initted) {
>>           ret = -EIO;
>> @@ -364,6 +365,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 76f1395..2c5706a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, 
>> struct device *master,
>>           goto end;
>>       }
>>   +    pm_runtime_enable(dev);
>
> devm_pm_runtime_enable() removes need for a cleanup.
>
>> +    pm_runtime_set_autosuspend_delay(dev, 1000);
>> +    pm_runtime_use_autosuspend(dev);
>
> Why do you want to use autosuspend here?
>
>> +
>>       return 0;
>>   end:
>>       return rc;
>> @@ -320,9 +324,8 @@ 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);
>> +    pm_runtime_dont_use_autosuspend(dev);
>> +    pm_runtime_disable(dev);
>>         kthread_stop(dp->ev_tsk);
>>   @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
>> dp_display_private *dp)
>>           dp->dp_display.connector_type, dp->core_initialized,
>>           dp->phy_initialized);
>>   -    dp_power_init(dp->power);
>> -    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>> -    dp_aux_init(dp->aux);
>> -    dp->core_initialized = true;
>> +    if (!dp->core_initialized) {
>> +        dp_power_init(dp->power);
>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>> +        dp_aux_init(dp->aux);
>> +        dp->core_initialized = true;
>> +    }
>
> Is this relevant to PM runtime? I don't think so.
>
>>   }
>>     static void dp_display_host_deinit(struct dp_display_private *dp)
>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
>> dp_display_private *dp)
>>           dp->dp_display.connector_type, dp->core_initialized,
>>           dp->phy_initialized);
>>   -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>> -    dp_aux_deinit(dp->aux);
>> -    dp_power_deinit(dp->power);
>> -    dp->core_initialized = false;
>> +    if (dp->core_initialized) {
>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>> +        dp_aux_deinit(dp->aux);
>> +        dp_power_deinit(dp->power);
>> +        dp->core_initialized = false;
>> +    }
>>   }
>>     static int dp_display_usbpd_configure_cb(struct device *dev)
>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
>> platform_device *pdev)
>>       dp_display_deinit_sub_modules(dp);
>>         platform_set_drvdata(pdev, NULL);
>> +    pm_runtime_put_sync_suspend(&pdev->dev);
>> +
>> +    return 0;
>> +}
>> +
>> +static int dp_pm_runtime_suspend(struct device *dev)
>> +{
>> +    struct platform_device *pdev = to_platform_device(dev);
>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> +    struct dp_display_private *dp;
>> +
>> +    dp = container_of(dp_display, struct dp_display_private, 
>> dp_display);
>> +
>> +    dp_display_host_phy_exit(dp);
>> +    dp_catalog_ctrl_hpd_enable(dp->catalog);
>
> What? NO!
>
>> +    dp_display_host_deinit(dp);
>> +
>> +    return 0;
>> +}
>> +
>> +static int dp_pm_runtime_resume(struct device *dev)
>> +{
>> +    struct platform_device *pdev = to_platform_device(dev);
>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> +    struct dp_display_private *dp;
>> +
>> +    dp = container_of(dp_display, struct dp_display_private, 
>> dp_display);
>> +
>> +    dp_display_host_init(dp);
>> +    if (dp_display->is_edp) {
>> +        dp_catalog_ctrl_hpd_enable(dp->catalog);
>> +        dp_display_host_phy_init(dp);
>> +    }
>>         return 0;
>>   }
>> @@ -1409,6 +1449,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,
>
> With the runtime PM in place, can we change suspend/resume to use 
> pm_runtime_force_suspend() and pm_runtime_force_resume() ?
>
>
>>   };
>> @@ -1493,10 +1534,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);
>
> Are you going to populate the AUX bus (which can cause AUX bus access) 
> without waking up the device?

no,  pm_runtime_get_sync() will be called inside 
generic_edp_panel_probe() which will trigger dp_pm_runtime_resume() be 
called to wake up device (initialize dp host) before retrieve

panel_id from edp panel through aux bus.

>
>> -
>>           /*
>>            * The code below assumes that the panel will finish probing
>>            * by the time devm_of_dp_aux_populate_ep_devices() returns.
>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge 
>> *drm_bridge,
>>           dp_hpd_plug_handle(dp_display, 0);
>
> Nearly the same question. Resume device before accessing registers.
>
>> mutex_lock(&dp_display->event_mutex);
>> +    pm_runtime_get_sync(&dp_display->pdev->dev);
>>         state = dp_display->hpd_state;
>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>> @@ -1684,6 +1722,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);
>>   }
>>   @@ -1723,6 +1763,8 @@ 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);
>> +    pm_runtime_get_sync(&dp->pdev->dev);
>> +
>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>         /* enable HDP interrupts */
>> @@ -1744,6 +1786,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);
>>   }
>

Re: [PATCH v1 5/5] drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP

[Kuogee Hsieh]

On 7/10/2023 11:24 AM, Dmitry Baryshkov wrote:
> [Restored CC list]
>
> On Mon, 10 Jul 2023 at 20:08, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 7/7/2023 5:32 PM, Dmitry Baryshkov wrote:
>>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>>> Move of_dp_aux_populate_bus() to dp_display_probe() for eDP
>>>> from dp_display_bind() so that probe deferral cases can be
>>>> handled effectively
>>>>
>>>> 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, 65 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> index c592064..c1baffb 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> @@ -505,6 +505,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)
>>>>    {
>>>> @@ -528,6 +543,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 185f1eb..7ed4bea 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev,
>>>> struct device *master,
>>>>            goto end;
>>>>        }
>>>>    -    pm_runtime_enable(dev);
>>>> -    pm_runtime_set_autosuspend_delay(dev, 1000);
>>>> -    pm_runtime_use_autosuspend(dev);
>>>> -
>>>>        return 0;
>>>>    end:
>>>>        return rc;
>>>> @@ -322,8 +318,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);
>>>> @@ -1245,6 +1239,29 @@ static const struct msm_dp_desc
>>>> *dp_display_get_desc(struct platform_device *pde
>>>>        return NULL;
>>>>    }
>>>>    +static void of_dp_aux_depopulate_bus_void(void *data)
>>>> +{
>>>> +    of_dp_aux_depopulate_bus(data);
>>>> +}
>>>> +
>>>> +static int dp_display_auxbus_emulation(struct dp_display_private *dp)
>>> Why is it called emulation?
>>>
>>>> +{
>>>> +    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, NULL);
>>> And here you missed the whole point of why we have been asking for.
>>> Please add a sensible `done_probing' callback, which will call
>>> component_add(). This way the DP component will only be registered
>>> when the panel has been probed. Keeping us from the component binding
>>> retries and corresponding side effects.
>>>
>>>> +
>>>> +        devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void,
>>>> +                     dp->aux);
>>> Useless, it's already handled by the devm_ part of the
>>> devm_of_dp_aux_populate_bus().
>>>
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>    static int dp_display_probe(struct platform_device *pdev)
>>>>    {
>>>>        int rc = 0;
>>>> @@ -1290,8 +1307,18 @@ static int dp_display_probe(struct
>>>> platform_device *pdev)
>>>>          platform_set_drvdata(pdev, &dp->dp_display);
>>>>    +    pm_runtime_enable(&pdev->dev);
>>>> +    pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>>>> +    pm_runtime_use_autosuspend(&pdev->dev);
>>> Can we have this in probe right from the patch #2?
>> no, at patch#2, devm_of_dp_aux_populate_bus() is done ta bind timing.
>>
>> The device used by pm_runtime_get_sync() of generic_edp_panel_probe()
>> which is derived from devm_of_dp_aux_populate_bus() is different the
>> &pdev->dev here.
> Excuse me, I don't get your answer. In patch #2 you have added
> pm_runtime_enable() / etc to dp_display_bind().
> In this patch you are moving these calls to dp_display_probe(). I
> think that the latter is a better place for enabling runtime PM and as
> such I've asked you to squash this chunk into patch #2.
> Why isn't that going to work?
>
> If I'm not mistaken here, the panel's call to pm_runtime_get_sync()
> will wake up the panel and all the parent devices, including the DP.
> That's what I meant in my comment regarding PM calls in the patch #1.
> pm_runtime_get_sync() / resume() / etc. do not only increase the
> runtime PM count. They do other things to parent devices, linked
> devices, etc.

sorry for late response,

yes, pm_runtime_enable() at probe() is better and i did that original. 
but it is not work.

I found that,

1) at dp_display_bind(), dev is mdss

2) at probe() dev is dp

3) pm_runtime_enable(dp's dev) and generic_edp_panel_probe() --> 
pm_runtime_get_sync(mdss's dev)



>>>> +
>>>>        dp_display_request_irq(dp);
>>>>    +    if (dp->dp_display.is_edp) {
>>>> +        rc = dp_display_auxbus_emulation(dp);
>>>> +        if (rc)
>>>> +            DRM_ERROR("eDP aux-bus emulation failed, rc=%d\n", rc);
>>>> +    }
>>>> +
>>>>        rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>>>        if (rc) {
>>>>            DRM_ERROR("component add failed, rc=%d\n", rc);
>>>> @@ -1306,11 +1333,14 @@ 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_dont_use_autosuspend(&pdev->dev);
>>>> +    pm_runtime_disable(&pdev->dev);
>>>>        pm_runtime_put_sync_suspend(&pdev->dev);
>>>>    +    dp_display_deinit_sub_modules(dp);
>>>> +
>>>>        return 0;
>>>>    }
>>>>    @@ -1514,31 +1544,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
>>>> @@ -1551,17 +1560,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;
>>>>    }
>
>

Re: [PATCH v1 5/5] drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP

[Dmitry Baryshkov]

On 20/07/2023 23:27, Kuogee Hsieh wrote:
> 
> On 7/10/2023 11:24 AM, Dmitry Baryshkov wrote:
>> [Restored CC list]
>>
>> On Mon, 10 Jul 2023 at 20:08, Kuogee Hsieh <quic_khsieh@quicinc.com> 
>> wrote:
>>>
>>> On 7/7/2023 5:32 PM, Dmitry Baryshkov wrote:
>>>> On 08/07/2023 02:52, Kuogee Hsieh wrote:
>>>>> Move of_dp_aux_populate_bus() to dp_display_probe() for eDP
>>>>> from dp_display_bind() so that probe deferral cases can be
>>>>> handled effectively
>>>>>
>>>>> 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, 65 insertions(+), 39 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>> index c592064..c1baffb 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>> @@ -505,6 +505,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)
>>>>>    {
>>>>> @@ -528,6 +543,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 185f1eb..7ed4bea 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> @@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev,
>>>>> struct device *master,
>>>>>            goto end;
>>>>>        }
>>>>>    -    pm_runtime_enable(dev);
>>>>> -    pm_runtime_set_autosuspend_delay(dev, 1000);
>>>>> -    pm_runtime_use_autosuspend(dev);
>>>>> -
>>>>>        return 0;
>>>>>    end:
>>>>>        return rc;
>>>>> @@ -322,8 +318,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);
>>>>> @@ -1245,6 +1239,29 @@ static const struct msm_dp_desc
>>>>> *dp_display_get_desc(struct platform_device *pde
>>>>>        return NULL;
>>>>>    }
>>>>>    +static void of_dp_aux_depopulate_bus_void(void *data)
>>>>> +{
>>>>> +    of_dp_aux_depopulate_bus(data);
>>>>> +}
>>>>> +
>>>>> +static int dp_display_auxbus_emulation(struct dp_display_private *dp)
>>>> Why is it called emulation?
>>>>
>>>>> +{
>>>>> +    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, NULL);
>>>> And here you missed the whole point of why we have been asking for.
>>>> Please add a sensible `done_probing' callback, which will call
>>>> component_add(). This way the DP component will only be registered
>>>> when the panel has been probed. Keeping us from the component binding
>>>> retries and corresponding side effects.
>>>>
>>>>> +
>>>>> +        devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void,
>>>>> +                     dp->aux);
>>>> Useless, it's already handled by the devm_ part of the
>>>> devm_of_dp_aux_populate_bus().
>>>>
>>>>> +    }
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>>    static int dp_display_probe(struct platform_device *pdev)
>>>>>    {
>>>>>        int rc = 0;
>>>>> @@ -1290,8 +1307,18 @@ static int dp_display_probe(struct
>>>>> platform_device *pdev)
>>>>>          platform_set_drvdata(pdev, &dp->dp_display);
>>>>>    +    pm_runtime_enable(&pdev->dev);
>>>>> +    pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>>>>> +    pm_runtime_use_autosuspend(&pdev->dev);
>>>> Can we have this in probe right from the patch #2?
>>> no, at patch#2, devm_of_dp_aux_populate_bus() is done ta bind timing.
>>>
>>> The device used by pm_runtime_get_sync() of generic_edp_panel_probe()
>>> which is derived from devm_of_dp_aux_populate_bus() is different the
>>> &pdev->dev here.
>> Excuse me, I don't get your answer. In patch #2 you have added
>> pm_runtime_enable() / etc to dp_display_bind().
>> In this patch you are moving these calls to dp_display_probe(). I
>> think that the latter is a better place for enabling runtime PM and as
>> such I've asked you to squash this chunk into patch #2.
>> Why isn't that going to work?
>>
>> If I'm not mistaken here, the panel's call to pm_runtime_get_sync()
>> will wake up the panel and all the parent devices, including the DP.
>> That's what I meant in my comment regarding PM calls in the patch #1.
>> pm_runtime_get_sync() / resume() / etc. do not only increase the
>> runtime PM count. They do other things to parent devices, linked
>> devices, etc.
> 
> sorry for late response,
> 
> yes, pm_runtime_enable() at probe() is better and i did that original. 
> but it is not work.
> 
> I found that,
> 
> 1) at dp_display_bind(), dev is mdss

If the 'dev' is the issue, you can always use dp_display_private::pdev.

> 
> 2) at probe() dev is dp
> 
> 3) pm_runtime_enable(dp's dev) and generic_edp_panel_probe() --> 
> pm_runtime_get_sync(mdss's dev)

I might be missing something. Please describe, what exactly doesn't work.

> 
> 
> 
>>>>> +
>>>>>        dp_display_request_irq(dp);
>>>>>    +    if (dp->dp_display.is_edp) {
>>>>> +        rc = dp_display_auxbus_emulation(dp);
>>>>> +        if (rc)
>>>>> +            DRM_ERROR("eDP aux-bus emulation failed, rc=%d\n", rc);
>>>>> +    }
>>>>> +
>>>>>        rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>>>>        if (rc) {
>>>>>            DRM_ERROR("component add failed, rc=%d\n", rc);
>>>>> @@ -1306,11 +1333,14 @@ 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_dont_use_autosuspend(&pdev->dev);
>>>>> +    pm_runtime_disable(&pdev->dev);
>>>>>        pm_runtime_put_sync_suspend(&pdev->dev);
>>>>>    +    dp_display_deinit_sub_modules(dp);
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>>    @@ -1514,31 +1544,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
>>>>> @@ -1551,17 +1560,9 @@ static int dp_display_get_next_bridge(struct
>>>>> msm_dp *dp)
>>>>>        if (!dp->is_edp && rc == -ENODEV)
>>>>>            return 0;
>>>>>    -    if (!rc) {
>>>>> +    if (!rc)
>>>>>            dp->next_bridge = dp_priv->parser->next_bridge;
>>>>> -        return 0;
>>>>> -    }
>>>>>    -error:
>>>>> -    if (dp->is_edp) {
>>>>> -        of_dp_aux_depopulate_bus(dp_priv->aux);
>>>>> -        dp_display_host_phy_exit(dp_priv);
>>>>> -        dp_display_host_deinit(dp_priv);
>>>>> -    }
>>>>>        return rc;
>>>>>    }
>>
>>

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Kuogee Hsieh]

On 7/10/2023 9:22 AM, Kuogee Hsieh wrote:
>
> On 7/8/2023 7:52 PM, Bjorn Andersson wrote:
>> On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:
>>> Incorporating pm runtime framework into DP driver so that power
>>> and clock resource handling can be centralized allowing easier
>>> control of these resources in preparation of registering aux bus
>>> uring probe.
>>>
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>>>   drivers/gpu/drm/msm/dp/dp_display.c | 75 
>>> +++++++++++++++++++++++++++++--------
>>>   2 files changed, 63 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> index 8e3b677..c592064 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>>> *dp_aux,
>>>           return -EINVAL;
>>>       }
>>>   +    pm_runtime_get_sync(dp_aux->dev);
>>>       mutex_lock(&aux->mutex);
>>>       if (!aux->initted) {
>>>           ret = -EIO;
>>> @@ -364,6 +365,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 76f1395..2c5706a 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, 
>>> struct device *master,
>>>           goto end;
>>>       }
>>>   +    pm_runtime_enable(dev);
>>> +    pm_runtime_set_autosuspend_delay(dev, 1000);
>>> +    pm_runtime_use_autosuspend(dev);
>>> +
>>>       return 0;
>>>   end:
>>>       return rc;
>>> @@ -320,9 +324,8 @@ 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);
>>> +    pm_runtime_dont_use_autosuspend(dev);
>>> +    pm_runtime_disable(dev);
>>>         kthread_stop(dp->ev_tsk);
>>>   @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
>>> dp_display_private *dp)
>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>           dp->phy_initialized);
>>>   -    dp_power_init(dp->power);
>>> -    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>> -    dp_aux_init(dp->aux);
>>> -    dp->core_initialized = true;
>>> +    if (!dp->core_initialized) {
>>> +        dp_power_init(dp->power);
>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>> +        dp_aux_init(dp->aux);
>>> +        dp->core_initialized = true;
>> There are two cases that queries core_initialized, both of those are
>> done to avoid accessing the DP block without it first being powered up.
>> With the introduction of runtime PM, it seems reasonable to just power
>> up the block in those two code paths (and remove the variable).
>>
>>> +    }
>>>   }
>>>     static void dp_display_host_deinit(struct dp_display_private *dp)
>>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
>>> dp_display_private *dp)
>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>           dp->phy_initialized);
>>>   -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>> -    dp_aux_deinit(dp->aux);
>>> -    dp_power_deinit(dp->power);
>>> -    dp->core_initialized = false;
>>> +    if (dp->core_initialized) {
>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>> +        dp_aux_deinit(dp->aux);
>>> +        dp_power_deinit(dp->power);
>>> +        dp->core_initialized = false;
>>> +    }
>>>   }
>>>     static int dp_display_usbpd_configure_cb(struct device *dev)
>>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
>>> platform_device *pdev)
>>>       dp_display_deinit_sub_modules(dp);
>>>         platform_set_drvdata(pdev, NULL);
>>> +    pm_runtime_put_sync_suspend(&pdev->dev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int dp_pm_runtime_suspend(struct device *dev)
>>> +{
>>> +    struct platform_device *pdev = to_platform_device(dev);
>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> platform_get_drvdata() is a wrapper for dev_get_drvdata(&pdev->dev), so
>> there's no need to resolve the platform_device first...
>>
>>> +    struct dp_display_private *dp;
>>> +
>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>> dp_display);
>>> +
>>> +    dp_display_host_phy_exit(dp);
>>> +    dp_catalog_ctrl_hpd_enable(dp->catalog);
>>> +    dp_display_host_deinit(dp);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int dp_pm_runtime_resume(struct device *dev)
>>> +{
>>> +    struct platform_device *pdev = to_platform_device(dev);
>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>> +    struct dp_display_private *dp;
>>> +
>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>> dp_display);
>>> +
>>> +    dp_display_host_init(dp);
>>> +    if (dp_display->is_edp) {
>>> +        dp_catalog_ctrl_hpd_enable(dp->catalog);
>>> +        dp_display_host_phy_init(dp);
>>> +    }
>>>         return 0;
>>>   }
>>> @@ -1409,6 +1449,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,
>>>   };
>>> @@ -1493,10 +1534,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);
>> I'm probably just missing it, but how do we get here with the host
>> powered up and the phy initialized?
>
> if (!dp->core_initialized)  is at dp_display_host_init()
>
>>
>>> -
>>>           /*
>>>            * The code below assumes that the panel will finish probing
>>>            * by the time devm_of_dp_aux_populate_ep_devices() returns.
>>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge 
>>> *drm_bridge,
>>>           dp_hpd_plug_handle(dp_display, 0);
>>>         mutex_lock(&dp_display->event_mutex);
>>> +    pm_runtime_get_sync(&dp_display->pdev->dev);
>>>         state = dp_display->hpd_state;
>>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>>> @@ -1684,6 +1722,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);
>>>   }
>>>   @@ -1723,6 +1763,8 @@ 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);
>>> +    pm_runtime_get_sync(&dp->pdev->dev);
>>> +
>>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>         /* enable HDP interrupts */
>>> @@ -1744,6 +1786,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);
>>>   }
>> The runtime_get/put in dp_bridge_hpd_enable() and disable matches my
>> expectations. But in the case that we have an external HPD source, where
>> will the power be turned on?
>>
>> Note that you can test this on your device by routing the HPD GPIO to a
>> display-connector instance and wiring this to the DP node. In the same
>> way it's done here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28 
>>

at sc7280, gpio-47 has function 2 as dp-hot-plug pin. but it does not 
has function for general purpose pin.

Just curious,  to work with external HPD source,

1) which DRM_BRIDGE_OP_xxx should be used for bridge->ops?

2) are both .hpd_enable and .hpd_disable  have to be populated?

>>
>> Regards,
>> Bjorn
>>
>>>   --
>>> 2.7.4
>>>

Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Dmitry Baryshkov]

On 26/07/2023 01:25, Kuogee Hsieh wrote:
> 
> On 7/10/2023 9:22 AM, Kuogee Hsieh wrote:
>>
>> On 7/8/2023 7:52 PM, Bjorn Andersson wrote:
>>> On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:
>>>> Incorporating pm runtime framework into DP driver so that power
>>>> and clock resource handling can be centralized allowing easier
>>>> control of these resources in preparation of registering aux bus
>>>> uring probe.
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>>>>   drivers/gpu/drm/msm/dp/dp_display.c | 75 
>>>> +++++++++++++++++++++++++++++--------
>>>>   2 files changed, 63 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> index 8e3b677..c592064 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>>>> *dp_aux,
>>>>           return -EINVAL;
>>>>       }
>>>>   +    pm_runtime_get_sync(dp_aux->dev);
>>>>       mutex_lock(&aux->mutex);
>>>>       if (!aux->initted) {
>>>>           ret = -EIO;
>>>> @@ -364,6 +365,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 76f1395..2c5706a 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, 
>>>> struct device *master,
>>>>           goto end;
>>>>       }
>>>>   +    pm_runtime_enable(dev);
>>>> +    pm_runtime_set_autosuspend_delay(dev, 1000);
>>>> +    pm_runtime_use_autosuspend(dev);
>>>> +
>>>>       return 0;
>>>>   end:
>>>>       return rc;
>>>> @@ -320,9 +324,8 @@ 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);
>>>> +    pm_runtime_dont_use_autosuspend(dev);
>>>> +    pm_runtime_disable(dev);
>>>>         kthread_stop(dp->ev_tsk);
>>>>   @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
>>>> dp_display_private *dp)
>>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>>           dp->phy_initialized);
>>>>   -    dp_power_init(dp->power);
>>>> -    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>>> -    dp_aux_init(dp->aux);
>>>> -    dp->core_initialized = true;
>>>> +    if (!dp->core_initialized) {
>>>> +        dp_power_init(dp->power);
>>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>>> +        dp_aux_init(dp->aux);
>>>> +        dp->core_initialized = true;
>>> There are two cases that queries core_initialized, both of those are
>>> done to avoid accessing the DP block without it first being powered up.
>>> With the introduction of runtime PM, it seems reasonable to just power
>>> up the block in those two code paths (and remove the variable).
>>>
>>>> +    }
>>>>   }
>>>>     static void dp_display_host_deinit(struct dp_display_private *dp)
>>>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
>>>> dp_display_private *dp)
>>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>>           dp->phy_initialized);
>>>>   -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>>> -    dp_aux_deinit(dp->aux);
>>>> -    dp_power_deinit(dp->power);
>>>> -    dp->core_initialized = false;
>>>> +    if (dp->core_initialized) {
>>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>>> +        dp_aux_deinit(dp->aux);
>>>> +        dp_power_deinit(dp->power);
>>>> +        dp->core_initialized = false;
>>>> +    }
>>>>   }
>>>>     static int dp_display_usbpd_configure_cb(struct device *dev)
>>>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
>>>> platform_device *pdev)
>>>>       dp_display_deinit_sub_modules(dp);
>>>>         platform_set_drvdata(pdev, NULL);
>>>> +    pm_runtime_put_sync_suspend(&pdev->dev);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int dp_pm_runtime_suspend(struct device *dev)
>>>> +{
>>>> +    struct platform_device *pdev = to_platform_device(dev);
>>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>> platform_get_drvdata() is a wrapper for dev_get_drvdata(&pdev->dev), so
>>> there's no need to resolve the platform_device first...
>>>
>>>> +    struct dp_display_private *dp;
>>>> +
>>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>>> dp_display);
>>>> +
>>>> +    dp_display_host_phy_exit(dp);
>>>> +    dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>> +    dp_display_host_deinit(dp);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int dp_pm_runtime_resume(struct device *dev)
>>>> +{
>>>> +    struct platform_device *pdev = to_platform_device(dev);
>>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>>> +    struct dp_display_private *dp;
>>>> +
>>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>>> dp_display);
>>>> +
>>>> +    dp_display_host_init(dp);
>>>> +    if (dp_display->is_edp) {
>>>> +        dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>> +        dp_display_host_phy_init(dp);
>>>> +    }
>>>>         return 0;
>>>>   }
>>>> @@ -1409,6 +1449,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,
>>>>   };
>>>> @@ -1493,10 +1534,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);
>>> I'm probably just missing it, but how do we get here with the host
>>> powered up and the phy initialized?
>>
>> if (!dp->core_initialized)  is at dp_display_host_init()
>>
>>>
>>>> -
>>>>           /*
>>>>            * The code below assumes that the panel will finish probing
>>>>            * by the time devm_of_dp_aux_populate_ep_devices() returns.
>>>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge 
>>>> *drm_bridge,
>>>>           dp_hpd_plug_handle(dp_display, 0);
>>>>         mutex_lock(&dp_display->event_mutex);
>>>> +    pm_runtime_get_sync(&dp_display->pdev->dev);
>>>>         state = dp_display->hpd_state;
>>>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>>>> @@ -1684,6 +1722,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);
>>>>   }
>>>>   @@ -1723,6 +1763,8 @@ 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);
>>>> +    pm_runtime_get_sync(&dp->pdev->dev);
>>>> +
>>>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>>         /* enable HDP interrupts */
>>>> @@ -1744,6 +1786,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);
>>>>   }
>>> The runtime_get/put in dp_bridge_hpd_enable() and disable matches my
>>> expectations. But in the case that we have an external HPD source, where
>>> will the power be turned on?
>>>
>>> Note that you can test this on your device by routing the HPD GPIO to a
>>> display-connector instance and wiring this to the DP node. In the same
>>> way it's done here:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28
> 
> at sc7280, gpio-47 has function 2 as dp-hot-plug pin. but it does not 
> has function for general purpose pin.

It has a 'gpio' function, so that the pin can be used as a generic GPIO 
with a dp-connector device.

> 
> Just curious,  to work with external HPD source,
> 
> 1) which DRM_BRIDGE_OP_xxx should be used for bridge->ops?

There is no difference. The drm_bridge_connector will select the bridges 
according to the needs. E.g. the dp-connector can provide 
DRM_BRIDGE_OP_DETECT / OP_HPD (if the hpd-gpios property is configured). 
If it does, it will be selected for detection/HPD handling. If not, the 
main dp bridge will handle these operations.

> 
> 2) are both .hpd_enable and .hpd_disable  have to be populated?

No, they are both optional.

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Kuogee Hsieh]

On 7/25/2023 3:33 PM, Dmitry Baryshkov wrote:
> On 26/07/2023 01:25, Kuogee Hsieh wrote:
>>
>> On 7/10/2023 9:22 AM, Kuogee Hsieh wrote:
>>>
>>> On 7/8/2023 7:52 PM, Bjorn Andersson wrote:
>>>> On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:
>>>>> Incorporating pm runtime framework into DP driver so that power
>>>>> and clock resource handling can be centralized allowing easier
>>>>> control of these resources in preparation of registering aux bus
>>>>> uring probe.
>>>>>
>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>>>>>   drivers/gpu/drm/msm/dp/dp_display.c | 75 
>>>>> +++++++++++++++++++++++++++++--------
>>>>>   2 files changed, 63 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>> index 8e3b677..c592064 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct 
>>>>> drm_dp_aux *dp_aux,
>>>>>           return -EINVAL;
>>>>>       }
>>>>>   +    pm_runtime_get_sync(dp_aux->dev);
>>>>>       mutex_lock(&aux->mutex);
>>>>>       if (!aux->initted) {
>>>>>           ret = -EIO;
>>>>> @@ -364,6 +365,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 76f1395..2c5706a 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device 
>>>>> *dev, struct device *master,
>>>>>           goto end;
>>>>>       }
>>>>>   +    pm_runtime_enable(dev);
>>>>> +    pm_runtime_set_autosuspend_delay(dev, 1000);
>>>>> +    pm_runtime_use_autosuspend(dev);
>>>>> +
>>>>>       return 0;
>>>>>   end:
>>>>>       return rc;
>>>>> @@ -320,9 +324,8 @@ 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);
>>>>> +    pm_runtime_dont_use_autosuspend(dev);
>>>>> +    pm_runtime_disable(dev);
>>>>>         kthread_stop(dp->ev_tsk);
>>>>>   @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
>>>>> dp_display_private *dp)
>>>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>>>           dp->phy_initialized);
>>>>>   -    dp_power_init(dp->power);
>>>>> -    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>>>> -    dp_aux_init(dp->aux);
>>>>> -    dp->core_initialized = true;
>>>>> +    if (!dp->core_initialized) {
>>>>> +        dp_power_init(dp->power);
>>>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>>>> +        dp_aux_init(dp->aux);
>>>>> +        dp->core_initialized = true;
>>>> There are two cases that queries core_initialized, both of those are
>>>> done to avoid accessing the DP block without it first being powered 
>>>> up.
>>>> With the introduction of runtime PM, it seems reasonable to just power
>>>> up the block in those two code paths (and remove the variable).
>>>>
>>>>> +    }
>>>>>   }
>>>>>     static void dp_display_host_deinit(struct dp_display_private *dp)
>>>>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
>>>>> dp_display_private *dp)
>>>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>>>           dp->phy_initialized);
>>>>>   -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>>>> -    dp_aux_deinit(dp->aux);
>>>>> -    dp_power_deinit(dp->power);
>>>>> -    dp->core_initialized = false;
>>>>> +    if (dp->core_initialized) {
>>>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>>>> +        dp_aux_deinit(dp->aux);
>>>>> +        dp_power_deinit(dp->power);
>>>>> +        dp->core_initialized = false;
>>>>> +    }
>>>>>   }
>>>>>     static int dp_display_usbpd_configure_cb(struct device *dev)
>>>>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
>>>>> platform_device *pdev)
>>>>>       dp_display_deinit_sub_modules(dp);
>>>>>         platform_set_drvdata(pdev, NULL);
>>>>> +    pm_runtime_put_sync_suspend(&pdev->dev);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int dp_pm_runtime_suspend(struct device *dev)
>>>>> +{
>>>>> +    struct platform_device *pdev = to_platform_device(dev);
>>>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>>> platform_get_drvdata() is a wrapper for 
>>>> dev_get_drvdata(&pdev->dev), so
>>>> there's no need to resolve the platform_device first...
>>>>
>>>>> +    struct dp_display_private *dp;
>>>>> +
>>>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>>>> dp_display);
>>>>> +
>>>>> +    dp_display_host_phy_exit(dp);
>>>>> +    dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>>> +    dp_display_host_deinit(dp);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int dp_pm_runtime_resume(struct device *dev)
>>>>> +{
>>>>> +    struct platform_device *pdev = to_platform_device(dev);
>>>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>>>> +    struct dp_display_private *dp;
>>>>> +
>>>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>>>> dp_display);
>>>>> +
>>>>> +    dp_display_host_init(dp);
>>>>> +    if (dp_display->is_edp) {
>>>>> +        dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>>> +        dp_display_host_phy_init(dp);
>>>>> +    }
>>>>>         return 0;
>>>>>   }
>>>>> @@ -1409,6 +1449,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,
>>>>>   };
>>>>> @@ -1493,10 +1534,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);
>>>> I'm probably just missing it, but how do we get here with the host
>>>> powered up and the phy initialized?
>>>
>>> if (!dp->core_initialized)  is at dp_display_host_init()
>>>
>>>>
>>>>> -
>>>>>           /*
>>>>>            * The code below assumes that the panel will finish 
>>>>> probing
>>>>>            * by the time devm_of_dp_aux_populate_ep_devices() 
>>>>> returns.
>>>>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct 
>>>>> drm_bridge *drm_bridge,
>>>>>           dp_hpd_plug_handle(dp_display, 0);
>>>>>         mutex_lock(&dp_display->event_mutex);
>>>>> + pm_runtime_get_sync(&dp_display->pdev->dev);
>>>>>         state = dp_display->hpd_state;
>>>>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>>>>> @@ -1684,6 +1722,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);
>>>>>   }
>>>>>   @@ -1723,6 +1763,8 @@ 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);
>>>>> +    pm_runtime_get_sync(&dp->pdev->dev);
>>>>> +
>>>>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>>>         /* enable HDP interrupts */
>>>>> @@ -1744,6 +1786,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);
>>>>>   }
>>>> The runtime_get/put in dp_bridge_hpd_enable() and disable matches my
>>>> expectations. But in the case that we have an external HPD source, 
>>>> where
>>>> will the power be turned on?
>>>>
>>>> Note that you can test this on your device by routing the HPD GPIO 
>>>> to a
>>>> display-connector instance and wiring this to the DP node. In the same
>>>> way it's done here:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28 
>>>>
>>
>> at sc7280, gpio-47 has function 2 as dp-hot-plug pin. but it does not 
>> has function for general purpose pin.
>
> It has a 'gpio' function, so that the pin can be used as a generic 
> GPIO with a dp-connector device.
function 0 is for PBL_DEBUG2,  which function # should I use?
>
>>
>> Just curious,  to work with external HPD source,
>>
>> 1) which DRM_BRIDGE_OP_xxx should be used for bridge->ops?
>
> There is no difference. The drm_bridge_connector will select the 
> bridges according to the needs. E.g. the dp-connector can provide 
> DRM_BRIDGE_OP_DETECT / OP_HPD (if the hpd-gpios property is 
> configured). If it does, it will be selected for detection/HPD 
> handling. If not, the main dp bridge will handle these operations.

can you please point me out where  "hpd-gpios" info get parsed and 
during polling where the gpio status get read from?

Thanks,

>
>>
>> 2) are both .hpd_enable and .hpd_disable  have to be populated?
>
> No, they are both optional.
>

Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Dmitry Baryshkov]

On 26/07/2023 02:26, Kuogee Hsieh wrote:
> 
> On 7/25/2023 3:33 PM, Dmitry Baryshkov wrote:
>> On 26/07/2023 01:25, Kuogee Hsieh wrote:
>>>
>>> On 7/10/2023 9:22 AM, Kuogee Hsieh wrote:
>>>>
>>>> On 7/8/2023 7:52 PM, Bjorn Andersson wrote:
>>>>> On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:
>>>>>> Incorporating pm runtime framework into DP driver so that power
>>>>>> and clock resource handling can be centralized allowing easier
>>>>>> control of these resources in preparation of registering aux bus
>>>>>> uring probe.
>>>>>>
>>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++
>>>>>>   drivers/gpu/drm/msm/dp/dp_display.c | 75 
>>>>>> +++++++++++++++++++++++++++++--------
>>>>>>   2 files changed, 63 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>>>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>>> index 8e3b677..c592064 100644
>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>>>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct 
>>>>>> drm_dp_aux *dp_aux,
>>>>>>           return -EINVAL;
>>>>>>       }
>>>>>>   +    pm_runtime_get_sync(dp_aux->dev);
>>>>>>       mutex_lock(&aux->mutex);
>>>>>>       if (!aux->initted) {
>>>>>>           ret = -EIO;
>>>>>> @@ -364,6 +365,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 76f1395..2c5706a 100644
>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device 
>>>>>> *dev, struct device *master,
>>>>>>           goto end;
>>>>>>       }
>>>>>>   +    pm_runtime_enable(dev);
>>>>>> +    pm_runtime_set_autosuspend_delay(dev, 1000);
>>>>>> +    pm_runtime_use_autosuspend(dev);
>>>>>> +
>>>>>>       return 0;
>>>>>>   end:
>>>>>>       return rc;
>>>>>> @@ -320,9 +324,8 @@ 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);
>>>>>> +    pm_runtime_dont_use_autosuspend(dev);
>>>>>> +    pm_runtime_disable(dev);
>>>>>>         kthread_stop(dp->ev_tsk);
>>>>>>   @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
>>>>>> dp_display_private *dp)
>>>>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>>>>           dp->phy_initialized);
>>>>>>   -    dp_power_init(dp->power);
>>>>>> -    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>>>>> -    dp_aux_init(dp->aux);
>>>>>> -    dp->core_initialized = true;
>>>>>> +    if (!dp->core_initialized) {
>>>>>> +        dp_power_init(dp->power);
>>>>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>>>>> +        dp_aux_init(dp->aux);
>>>>>> +        dp->core_initialized = true;
>>>>> There are two cases that queries core_initialized, both of those are
>>>>> done to avoid accessing the DP block without it first being powered 
>>>>> up.
>>>>> With the introduction of runtime PM, it seems reasonable to just power
>>>>> up the block in those two code paths (and remove the variable).
>>>>>
>>>>>> +    }
>>>>>>   }
>>>>>>     static void dp_display_host_deinit(struct dp_display_private *dp)
>>>>>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
>>>>>> dp_display_private *dp)
>>>>>>           dp->dp_display.connector_type, dp->core_initialized,
>>>>>>           dp->phy_initialized);
>>>>>>   -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>>>>> -    dp_aux_deinit(dp->aux);
>>>>>> -    dp_power_deinit(dp->power);
>>>>>> -    dp->core_initialized = false;
>>>>>> +    if (dp->core_initialized) {
>>>>>> +        dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>>>>> +        dp_aux_deinit(dp->aux);
>>>>>> +        dp_power_deinit(dp->power);
>>>>>> +        dp->core_initialized = false;
>>>>>> +    }
>>>>>>   }
>>>>>>     static int dp_display_usbpd_configure_cb(struct device *dev)
>>>>>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
>>>>>> platform_device *pdev)
>>>>>>       dp_display_deinit_sub_modules(dp);
>>>>>>         platform_set_drvdata(pdev, NULL);
>>>>>> +    pm_runtime_put_sync_suspend(&pdev->dev);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dp_pm_runtime_suspend(struct device *dev)
>>>>>> +{
>>>>>> +    struct platform_device *pdev = to_platform_device(dev);
>>>>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>>>> platform_get_drvdata() is a wrapper for 
>>>>> dev_get_drvdata(&pdev->dev), so
>>>>> there's no need to resolve the platform_device first...
>>>>>
>>>>>> +    struct dp_display_private *dp;
>>>>>> +
>>>>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>>>>> dp_display);
>>>>>> +
>>>>>> +    dp_display_host_phy_exit(dp);
>>>>>> +    dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>>>> +    dp_display_host_deinit(dp);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dp_pm_runtime_resume(struct device *dev)
>>>>>> +{
>>>>>> +    struct platform_device *pdev = to_platform_device(dev);
>>>>>> +    struct msm_dp *dp_display = platform_get_drvdata(pdev);
>>>>>> +    struct dp_display_private *dp;
>>>>>> +
>>>>>> +    dp = container_of(dp_display, struct dp_display_private, 
>>>>>> dp_display);
>>>>>> +
>>>>>> +    dp_display_host_init(dp);
>>>>>> +    if (dp_display->is_edp) {
>>>>>> +        dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>>>> +        dp_display_host_phy_init(dp);
>>>>>> +    }
>>>>>>         return 0;
>>>>>>   }
>>>>>> @@ -1409,6 +1449,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,
>>>>>>   };
>>>>>> @@ -1493,10 +1534,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);
>>>>> I'm probably just missing it, but how do we get here with the host
>>>>> powered up and the phy initialized?
>>>>
>>>> if (!dp->core_initialized)  is at dp_display_host_init()
>>>>
>>>>>
>>>>>> -
>>>>>>           /*
>>>>>>            * The code below assumes that the panel will finish 
>>>>>> probing
>>>>>>            * by the time devm_of_dp_aux_populate_ep_devices() 
>>>>>> returns.
>>>>>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct 
>>>>>> drm_bridge *drm_bridge,
>>>>>>           dp_hpd_plug_handle(dp_display, 0);
>>>>>>         mutex_lock(&dp_display->event_mutex);
>>>>>> + pm_runtime_get_sync(&dp_display->pdev->dev);
>>>>>>         state = dp_display->hpd_state;
>>>>>>       if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {
>>>>>> @@ -1684,6 +1722,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);
>>>>>>   }
>>>>>>   @@ -1723,6 +1763,8 @@ 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);
>>>>>> +    pm_runtime_get_sync(&dp->pdev->dev);
>>>>>> +
>>>>>>       dp_catalog_ctrl_hpd_enable(dp->catalog);
>>>>>>         /* enable HDP interrupts */
>>>>>> @@ -1744,6 +1786,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);
>>>>>>   }
>>>>> The runtime_get/put in dp_bridge_hpd_enable() and disable matches my
>>>>> expectations. But in the case that we have an external HPD source, 
>>>>> where
>>>>> will the power be turned on?
>>>>>
>>>>> Note that you can test this on your device by routing the HPD GPIO 
>>>>> to a
>>>>> display-connector instance and wiring this to the DP node. In the same
>>>>> way it's done here:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28
>>>
>>> at sc7280, gpio-47 has function 2 as dp-hot-plug pin. but it does not 
>>> has function for general purpose pin.
>>
>> It has a 'gpio' function, so that the pin can be used as a generic 
>> GPIO with a dp-connector device.
> function 0 is for PBL_DEBUG2,  which function # should I use?

does `function = "gpio"` work?

>>
>>>
>>> Just curious,  to work with external HPD source,
>>>
>>> 1) which DRM_BRIDGE_OP_xxx should be used for bridge->ops?
>>
>> There is no difference. The drm_bridge_connector will select the 
>> bridges according to the needs. E.g. the dp-connector can provide 
>> DRM_BRIDGE_OP_DETECT / OP_HPD (if the hpd-gpios property is 
>> configured). If it does, it will be selected for detection/HPD 
>> handling. If not, the main dp bridge will handle these operations.
> 
> can you please point me out where  "hpd-gpios" info get parsed and 
> during polling where the gpio status get read from?

Can you please take a look at the display-controller.c driver?

> 
> Thanks,
> 
>>
>>>
>>> 2) are both .hpd_enable and .hpd_disable  have to be populated?
>>
>> No, they are both optional.
>>

-- 
With best wishes
Dmitry