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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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;
>> }
>
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 <[email protected]>
>> ---
>> 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
>>
On Sun, 9 Jul 2023 at 20:22, Abhinav Kumar <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
On 7/9/2023 11:00 AM, Dmitry Baryshkov wrote:
> On Sun, 9 Jul 2023 at 20:22, Abhinav Kumar <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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;
>>>> }
>>>
>
>
>
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 <[email protected]>
>> ---
>> 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
>>
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 <[email protected]>
>> ---
>> 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);
>> }
>
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 <[email protected]>
>> ---
>> 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)
>
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 <[email protected]>
>>> ---
>>> 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
[Restored CC list]
On Mon, 10 Jul 2023 at 20:08, Kuogee Hsieh <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
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
>> <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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;
>>>>> }
>>>>
>>
>>
>>
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>> ---
>> 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);
>> }
>
On 7/10/2023 11:24 AM, Dmitry Baryshkov wrote:
> [Restored CC list]
>
> On Mon, 10 Jul 2023 at 20:08, Kuogee Hsieh <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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;
>>>> }
>
>
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 <[email protected]>
>> 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 <[email protected]>
>>>>> ---
>>>>> 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
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 <[email protected]>
>>> ---
>>> 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
>>>