Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
On 26/07/2023 02:26, Kuogee Hsieh wrote:
On 7/25/2023 3:33 PM, Dmitry Baryshkov wrote:
On 26/07/2023 01:25, Kuogee Hsieh wrote:
On 7/10/2023 9:22 AM, Kuogee Hsieh wrote:
On 7/8/2023 7:52 PM, Bjorn Andersson wrote:
On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:
Incorporating pm runtime framework into DP driver so that power
and clock resource handling can be centralized allowing easier
control of these resources in preparation of registering aux bus
uring probe.
Signed-off-by: Kuogee Hsieh
---
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(>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(>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(>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(>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);
+
Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
On 7/25/2023 3:33 PM, Dmitry Baryshkov wrote:
On 26/07/2023 01:25, Kuogee Hsieh wrote:
On 7/10/2023 9:22 AM, Kuogee Hsieh wrote:
On 7/8/2023 7:52 PM, Bjorn Andersson wrote:
On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:
Incorporating pm runtime framework into DP driver so that power
and clock resource handling can be centralized allowing easier
control of these resources in preparation of registering aux bus
uring probe.
Signed-off-by: Kuogee Hsieh
---
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(>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(>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(>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(>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);
+ }
Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
On 26/07/2023 01:25, Kuogee Hsieh wrote:
On 7/10/2023 9:22 AM, Kuogee Hsieh wrote:
On 7/8/2023 7:52 PM, Bjorn Andersson wrote:
On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:
Incorporating pm runtime framework into DP driver so that power
and clock resource handling can be centralized allowing easier
control of these resources in preparation of registering aux bus
uring probe.
Signed-off-by: Kuogee Hsieh
---
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(>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(>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(>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(>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
Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
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
---
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(>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(>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(>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(>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)
}
Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
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
---
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(>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(>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(>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
Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
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
---
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(>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(>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(>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);
+
+
Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
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
---
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(>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(>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(>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(>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) {
+
Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
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
---
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(>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(>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(>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;
+
+
Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
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
> ---
> 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(>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(>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(>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(>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);
> +
Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
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
---
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(>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(>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(>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) {
+
[Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver
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
---
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(>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(>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(>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 =
