| Message ID | 1688773943-3887-3-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | incorporate pm runtime framework and eDP clean up | expand |
On 08/07/2023 02:52, Kuogee Hsieh wrote: > Incorporating pm runtime framework into DP driver so that power > and clock resource handling can be centralized allowing easier > control of these resources in preparation of registering aux bus > uring probe. > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ > drivers/gpu/drm/msm/dp/dp_display.c | 75 +++++++++++++++++++++++++++++-------- > 2 files changed, 63 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index 8e3b677..c592064 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > return -EINVAL; > } > > + pm_runtime_get_sync(dp_aux->dev); Let me quote the function's documentation: Consider using pm_runtime_resume_and_get() instead of it, especially if its return value is checked by the caller, as this is likely to result in cleaner code. So two notes concerning the whole patch: - error checking is missing - please use pm_runtime_resume_and_get() instead. > mutex_lock(&aux->mutex); > if (!aux->initted) { > ret = -EIO; > @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > > exit: > mutex_unlock(&aux->mutex); > + pm_runtime_mark_last_busy(dp_aux->dev); > + pm_runtime_put_autosuspend(dp_aux->dev); > > return ret; > } > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 76f1395..2c5706a 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, struct device *master, > goto end; > } > > + pm_runtime_enable(dev); devm_pm_runtime_enable() removes need for a cleanup. > + pm_runtime_set_autosuspend_delay(dev, 1000); > + pm_runtime_use_autosuspend(dev); Why do you want to use autosuspend here? > + > return 0; > end: > return rc; > @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, struct device *master, > struct dp_display_private *dp = dev_get_dp_display_private(dev); > struct msm_drm_private *priv = dev_get_drvdata(master); > > - /* disable all HPD interrupts */ > - if (dp->core_initialized) > - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false); > + pm_runtime_dont_use_autosuspend(dev); > + pm_runtime_disable(dev); > > kthread_stop(dp->ev_tsk); > > @@ -466,10 +469,12 @@ static void dp_display_host_init(struct dp_display_private *dp) > dp->dp_display.connector_type, dp->core_initialized, > dp->phy_initialized); > > - dp_power_init(dp->power); > - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); > - dp_aux_init(dp->aux); > - dp->core_initialized = true; > + if (!dp->core_initialized) { > + dp_power_init(dp->power); > + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); > + dp_aux_init(dp->aux); > + dp->core_initialized = true; > + } Is this relevant to PM runtime? I don't think so. > } > > static void dp_display_host_deinit(struct dp_display_private *dp) > @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct dp_display_private *dp) > dp->dp_display.connector_type, dp->core_initialized, > dp->phy_initialized); > > - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); > - dp_aux_deinit(dp->aux); > - dp_power_deinit(dp->power); > - dp->core_initialized = false; > + if (dp->core_initialized) { > + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); > + dp_aux_deinit(dp->aux); > + dp_power_deinit(dp->power); > + dp->core_initialized = false; > + } > } > > static int dp_display_usbpd_configure_cb(struct device *dev) > @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct platform_device *pdev) > dp_display_deinit_sub_modules(dp); > > platform_set_drvdata(pdev, NULL); > + pm_runtime_put_sync_suspend(&pdev->dev); > + > + return 0; > +} > + > +static int dp_pm_runtime_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct msm_dp *dp_display = platform_get_drvdata(pdev); > + struct dp_display_private *dp; > + > + dp = container_of(dp_display, struct dp_display_private, dp_display); > + > + dp_display_host_phy_exit(dp); > + dp_catalog_ctrl_hpd_enable(dp->catalog); What? NO! > + dp_display_host_deinit(dp); > + > + return 0; > +} > + > +static int dp_pm_runtime_resume(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct msm_dp *dp_display = platform_get_drvdata(pdev); > + struct dp_display_private *dp; > + > + dp = container_of(dp_display, struct dp_display_private, dp_display); > + > + dp_display_host_init(dp); > + if (dp_display->is_edp) { > + dp_catalog_ctrl_hpd_enable(dp->catalog); > + dp_display_host_phy_init(dp); > + } > > return 0; > } > @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) > } > > static const struct dev_pm_ops dp_pm_ops = { > + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL) > .suspend = dp_pm_suspend, > .resume = dp_pm_resume, With the runtime PM in place, can we change suspend/resume to use pm_runtime_force_suspend() and pm_runtime_force_resume() ? > }; > @@ -1493,10 +1534,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) > aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); > > if (aux_bus && dp->is_edp) { > - dp_display_host_init(dp_priv); > - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); > - dp_display_host_phy_init(dp_priv); Are you going to populate the AUX bus (which can cause AUX bus access) without waking up the device? > - > /* > * The code below assumes that the panel will finish probing > * by the time devm_of_dp_aux_populate_ep_devices() returns. > @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge, > dp_hpd_plug_handle(dp_display, 0); Nearly the same question. Resume device before accessing registers. > > mutex_lock(&dp_display->event_mutex); > + pm_runtime_get_sync(&dp_display->pdev->dev); > > state = dp_display->hpd_state; > if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { > @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge, > } > > drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type); > + > + pm_runtime_put_sync(&dp_display->pdev->dev); > mutex_unlock(&dp_display->event_mutex); > } > > @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge) > struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display); > > mutex_lock(&dp->event_mutex); > + pm_runtime_get_sync(&dp->pdev->dev); > + > dp_catalog_ctrl_hpd_enable(dp->catalog); > > /* enable HDP interrupts */ > @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge) > dp_catalog_ctrl_hpd_disable(dp->catalog); > > dp_display->internal_hpd = false; > + > + pm_runtime_mark_last_busy(&dp->pdev->dev); > + pm_runtime_put_autosuspend(&dp->pdev->dev); > mutex_unlock(&dp->event_mutex); > } >
On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote: > Incorporating pm runtime framework into DP driver so that power > and clock resource handling can be centralized allowing easier > control of these resources in preparation of registering aux bus > uring probe. > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ > drivers/gpu/drm/msm/dp/dp_display.c | 75 +++++++++++++++++++++++++++++-------- > 2 files changed, 63 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index 8e3b677..c592064 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > return -EINVAL; > } > > + pm_runtime_get_sync(dp_aux->dev); > mutex_lock(&aux->mutex); > if (!aux->initted) { > ret = -EIO; > @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > > exit: > mutex_unlock(&aux->mutex); > + pm_runtime_mark_last_busy(dp_aux->dev); > + pm_runtime_put_autosuspend(dp_aux->dev); > > return ret; > } > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 76f1395..2c5706a 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, struct device *master, > goto end; > } > > + pm_runtime_enable(dev); > + pm_runtime_set_autosuspend_delay(dev, 1000); > + pm_runtime_use_autosuspend(dev); > + > return 0; > end: > return rc; > @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, struct device *master, > struct dp_display_private *dp = dev_get_dp_display_private(dev); > struct msm_drm_private *priv = dev_get_drvdata(master); > > - /* disable all HPD interrupts */ > - if (dp->core_initialized) > - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false); > + pm_runtime_dont_use_autosuspend(dev); > + pm_runtime_disable(dev); > > kthread_stop(dp->ev_tsk); > > @@ -466,10 +469,12 @@ static void dp_display_host_init(struct dp_display_private *dp) > dp->dp_display.connector_type, dp->core_initialized, > dp->phy_initialized); > > - dp_power_init(dp->power); > - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); > - dp_aux_init(dp->aux); > - dp->core_initialized = true; > + if (!dp->core_initialized) { > + dp_power_init(dp->power); > + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); > + dp_aux_init(dp->aux); > + dp->core_initialized = true; There are two cases that queries core_initialized, both of those are done to avoid accessing the DP block without it first being powered up. With the introduction of runtime PM, it seems reasonable to just power up the block in those two code paths (and remove the variable). > + } > } > > static void dp_display_host_deinit(struct dp_display_private *dp) > @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct dp_display_private *dp) > dp->dp_display.connector_type, dp->core_initialized, > dp->phy_initialized); > > - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); > - dp_aux_deinit(dp->aux); > - dp_power_deinit(dp->power); > - dp->core_initialized = false; > + if (dp->core_initialized) { > + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); > + dp_aux_deinit(dp->aux); > + dp_power_deinit(dp->power); > + dp->core_initialized = false; > + } > } > > static int dp_display_usbpd_configure_cb(struct device *dev) > @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct platform_device *pdev) > dp_display_deinit_sub_modules(dp); > > platform_set_drvdata(pdev, NULL); > + pm_runtime_put_sync_suspend(&pdev->dev); > + > + return 0; > +} > + > +static int dp_pm_runtime_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct msm_dp *dp_display = platform_get_drvdata(pdev); platform_get_drvdata() is a wrapper for dev_get_drvdata(&pdev->dev), so there's no need to resolve the platform_device first... > + struct dp_display_private *dp; > + > + dp = container_of(dp_display, struct dp_display_private, dp_display); > + > + dp_display_host_phy_exit(dp); > + dp_catalog_ctrl_hpd_enable(dp->catalog); > + dp_display_host_deinit(dp); > + > + return 0; > +} > + > +static int dp_pm_runtime_resume(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct msm_dp *dp_display = platform_get_drvdata(pdev); > + struct dp_display_private *dp; > + > + dp = container_of(dp_display, struct dp_display_private, dp_display); > + > + dp_display_host_init(dp); > + if (dp_display->is_edp) { > + dp_catalog_ctrl_hpd_enable(dp->catalog); > + dp_display_host_phy_init(dp); > + } > > return 0; > } > @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) > } > > static const struct dev_pm_ops dp_pm_ops = { > + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL) > .suspend = dp_pm_suspend, > .resume = dp_pm_resume, > }; > @@ -1493,10 +1534,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) > aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); > > if (aux_bus && dp->is_edp) { > - dp_display_host_init(dp_priv); > - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); > - dp_display_host_phy_init(dp_priv); I'm probably just missing it, but how do we get here with the host powered up and the phy initialized? > - > /* > * The code below assumes that the panel will finish probing > * by the time devm_of_dp_aux_populate_ep_devices() returns. > @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge, > dp_hpd_plug_handle(dp_display, 0); > > mutex_lock(&dp_display->event_mutex); > + pm_runtime_get_sync(&dp_display->pdev->dev); > > state = dp_display->hpd_state; > if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { > @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge, > } > > drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type); > + > + pm_runtime_put_sync(&dp_display->pdev->dev); > mutex_unlock(&dp_display->event_mutex); > } > > @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge) > struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display); > > mutex_lock(&dp->event_mutex); > + pm_runtime_get_sync(&dp->pdev->dev); > + > dp_catalog_ctrl_hpd_enable(dp->catalog); > > /* enable HDP interrupts */ > @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge) > dp_catalog_ctrl_hpd_disable(dp->catalog); > > dp_display->internal_hpd = false; > + > + pm_runtime_mark_last_busy(&dp->pdev->dev); > + pm_runtime_put_autosuspend(&dp->pdev->dev); > mutex_unlock(&dp->event_mutex); > } The runtime_get/put in dp_bridge_hpd_enable() and disable matches my expectations. But in the case that we have an external HPD source, where will the power be turned on? Note that you can test this on your device by routing the HPD GPIO to a display-connector instance and wiring this to the DP node. In the same way it's done here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28 Regards, Bjorn > > -- > 2.7.4 >
On 7/7/2023 5:04 PM, Dmitry Baryshkov wrote: > On 08/07/2023 02:52, Kuogee Hsieh wrote: >> Incorporating pm runtime framework into DP driver so that power >> and clock resource handling can be centralized allowing easier >> control of these resources in preparation of registering aux bus >> uring probe. >> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ >> drivers/gpu/drm/msm/dp/dp_display.c | 75 >> +++++++++++++++++++++++++++++-------- >> 2 files changed, 63 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c >> b/drivers/gpu/drm/msm/dp/dp_aux.c >> index 8e3b677..c592064 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >> *dp_aux, >> return -EINVAL; >> } >> + pm_runtime_get_sync(dp_aux->dev); > > Let me quote the function's documentation: > Consider using pm_runtime_resume_and_get() instead of it, especially > if its return value is checked by the caller, as this is likely to > result in cleaner code. pm_runtime_resume_and_get() will call pm_runtime_resume() every time. Since aux_transfer is called very frequently, is it just simple to call pm_runtiem_get_sync() which will call pm_runtime_reusme() if power counter is 0 before increased it. otherwise it just increase power counter? > > So two notes concerning the whole patch: > - error checking is missing > - please use pm_runtime_resume_and_get() instead. > >> mutex_lock(&aux->mutex); >> if (!aux->initted) { >> ret = -EIO; >> @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >> *dp_aux, >> exit: >> mutex_unlock(&aux->mutex); >> + pm_runtime_mark_last_busy(dp_aux->dev); >> + pm_runtime_put_autosuspend(dp_aux->dev); >> return ret; >> } >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >> b/drivers/gpu/drm/msm/dp/dp_display.c >> index 76f1395..2c5706a 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, >> struct device *master, >> goto end; >> } >> + pm_runtime_enable(dev); > > devm_pm_runtime_enable() removes need for a cleanup. > >> + pm_runtime_set_autosuspend_delay(dev, 1000); >> + pm_runtime_use_autosuspend(dev); > > Why do you want to use autosuspend here? > >> + >> return 0; >> end: >> return rc; >> @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, >> struct device *master, >> struct dp_display_private *dp = dev_get_dp_display_private(dev); >> struct msm_drm_private *priv = dev_get_drvdata(master); >> - /* disable all HPD interrupts */ >> - if (dp->core_initialized) >> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, >> false); >> + pm_runtime_dont_use_autosuspend(dev); >> + pm_runtime_disable(dev); >> kthread_stop(dp->ev_tsk); >> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct >> dp_display_private *dp) >> dp->dp_display.connector_type, dp->core_initialized, >> dp->phy_initialized); >> - dp_power_init(dp->power); >> - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >> - dp_aux_init(dp->aux); >> - dp->core_initialized = true; >> + if (!dp->core_initialized) { >> + dp_power_init(dp->power); >> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >> + dp_aux_init(dp->aux); >> + dp->core_initialized = true; >> + } > > Is this relevant to PM runtime? I don't think so. > >> } >> static void dp_display_host_deinit(struct dp_display_private *dp) >> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct >> dp_display_private *dp) >> dp->dp_display.connector_type, dp->core_initialized, >> dp->phy_initialized); >> - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >> - dp_aux_deinit(dp->aux); >> - dp_power_deinit(dp->power); >> - dp->core_initialized = false; >> + if (dp->core_initialized) { >> + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >> + dp_aux_deinit(dp->aux); >> + dp_power_deinit(dp->power); >> + dp->core_initialized = false; >> + } >> } >> static int dp_display_usbpd_configure_cb(struct device *dev) >> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct >> platform_device *pdev) >> dp_display_deinit_sub_modules(dp); >> platform_set_drvdata(pdev, NULL); >> + pm_runtime_put_sync_suspend(&pdev->dev); >> + >> + return 0; >> +} >> + >> +static int dp_pm_runtime_suspend(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >> + struct dp_display_private *dp; >> + >> + dp = container_of(dp_display, struct dp_display_private, >> dp_display); >> + >> + dp_display_host_phy_exit(dp); >> + dp_catalog_ctrl_hpd_enable(dp->catalog); > > What? NO! > >> + dp_display_host_deinit(dp); >> + >> + return 0; >> +} >> + >> +static int dp_pm_runtime_resume(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >> + struct dp_display_private *dp; >> + >> + dp = container_of(dp_display, struct dp_display_private, >> dp_display); >> + >> + dp_display_host_init(dp); >> + if (dp_display->is_edp) { >> + dp_catalog_ctrl_hpd_enable(dp->catalog); >> + dp_display_host_phy_init(dp); >> + } >> return 0; >> } >> @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) >> } >> static const struct dev_pm_ops dp_pm_ops = { >> + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, >> NULL) >> .suspend = dp_pm_suspend, >> .resume = dp_pm_resume, > > With the runtime PM in place, can we change suspend/resume to use > pm_runtime_force_suspend() and pm_runtime_force_resume() ? Let em try if i can move checking device connection status out of dp_pm_resume(). it handles external dp panel plugin/unplug during suspend cases. > > >> }; >> @@ -1493,10 +1534,6 @@ static int dp_display_get_next_bridge(struct >> msm_dp *dp) >> aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >> if (aux_bus && dp->is_edp) { >> - dp_display_host_init(dp_priv); >> - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); >> - dp_display_host_phy_init(dp_priv); > > Are you going to populate the AUX bus (which can cause AUX bus access) > without waking up the device? > devm_of_dp_aux_populate_ep_devices() ==> will call > pm_runtiemget_sync() internally which will call pm_runtime_resume() to > wake dp driver >> - >> /* >> * The code below assumes that the panel will finish probing >> * by the time devm_of_dp_aux_populate_ep_devices() returns. >> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge >> *drm_bridge, >> dp_hpd_plug_handle(dp_display, 0); > > Nearly the same question. Resume device before accessing registers. > >> mutex_lock(&dp_display->event_mutex); >> + pm_runtime_get_sync(&dp_display->pdev->dev); >> state = dp_display->hpd_state; >> if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { >> @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct >> drm_bridge *drm_bridge, >> } >> drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type); >> + >> + pm_runtime_put_sync(&dp_display->pdev->dev); >> mutex_unlock(&dp_display->event_mutex); >> } >> @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct drm_bridge >> *bridge) >> struct dp_display_private *dp = container_of(dp_display, struct >> dp_display_private, dp_display); >> mutex_lock(&dp->event_mutex); >> + pm_runtime_get_sync(&dp->pdev->dev); >> + >> dp_catalog_ctrl_hpd_enable(dp->catalog); >> /* enable HDP interrupts */ >> @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge >> *bridge) >> dp_catalog_ctrl_hpd_disable(dp->catalog); >> dp_display->internal_hpd = false; >> + >> + pm_runtime_mark_last_busy(&dp->pdev->dev); >> + pm_runtime_put_autosuspend(&dp->pdev->dev); >> mutex_unlock(&dp->event_mutex); >> } >
On 7/8/2023 7:52 PM, Bjorn Andersson wrote: > On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote: >> Incorporating pm runtime framework into DP driver so that power >> and clock resource handling can be centralized allowing easier >> control of these resources in preparation of registering aux bus >> uring probe. >> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ >> drivers/gpu/drm/msm/dp/dp_display.c | 75 +++++++++++++++++++++++++++++-------- >> 2 files changed, 63 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c >> index 8e3b677..c592064 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, >> return -EINVAL; >> } >> >> + pm_runtime_get_sync(dp_aux->dev); >> mutex_lock(&aux->mutex); >> if (!aux->initted) { >> ret = -EIO; >> @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, >> >> exit: >> mutex_unlock(&aux->mutex); >> + pm_runtime_mark_last_busy(dp_aux->dev); >> + pm_runtime_put_autosuspend(dp_aux->dev); >> >> return ret; >> } >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index 76f1395..2c5706a 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, struct device *master, >> goto end; >> } >> >> + pm_runtime_enable(dev); >> + pm_runtime_set_autosuspend_delay(dev, 1000); >> + pm_runtime_use_autosuspend(dev); >> + >> return 0; >> end: >> return rc; >> @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, struct device *master, >> struct dp_display_private *dp = dev_get_dp_display_private(dev); >> struct msm_drm_private *priv = dev_get_drvdata(master); >> >> - /* disable all HPD interrupts */ >> - if (dp->core_initialized) >> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false); >> + pm_runtime_dont_use_autosuspend(dev); >> + pm_runtime_disable(dev); >> >> kthread_stop(dp->ev_tsk); >> >> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct dp_display_private *dp) >> dp->dp_display.connector_type, dp->core_initialized, >> dp->phy_initialized); >> >> - dp_power_init(dp->power); >> - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >> - dp_aux_init(dp->aux); >> - dp->core_initialized = true; >> + if (!dp->core_initialized) { >> + dp_power_init(dp->power); >> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >> + dp_aux_init(dp->aux); >> + dp->core_initialized = true; > There are two cases that queries core_initialized, both of those are > done to avoid accessing the DP block without it first being powered up. > With the introduction of runtime PM, it seems reasonable to just power > up the block in those two code paths (and remove the variable). > >> + } >> } >> >> static void dp_display_host_deinit(struct dp_display_private *dp) >> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct dp_display_private *dp) >> dp->dp_display.connector_type, dp->core_initialized, >> dp->phy_initialized); >> >> - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >> - dp_aux_deinit(dp->aux); >> - dp_power_deinit(dp->power); >> - dp->core_initialized = false; >> + if (dp->core_initialized) { >> + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >> + dp_aux_deinit(dp->aux); >> + dp_power_deinit(dp->power); >> + dp->core_initialized = false; >> + } >> } >> >> static int dp_display_usbpd_configure_cb(struct device *dev) >> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct platform_device *pdev) >> dp_display_deinit_sub_modules(dp); >> >> platform_set_drvdata(pdev, NULL); >> + pm_runtime_put_sync_suspend(&pdev->dev); >> + >> + return 0; >> +} >> + >> +static int dp_pm_runtime_suspend(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct msm_dp *dp_display = platform_get_drvdata(pdev); > platform_get_drvdata() is a wrapper for dev_get_drvdata(&pdev->dev), so > there's no need to resolve the platform_device first... > >> + struct dp_display_private *dp; >> + >> + dp = container_of(dp_display, struct dp_display_private, dp_display); >> + >> + dp_display_host_phy_exit(dp); >> + dp_catalog_ctrl_hpd_enable(dp->catalog); >> + dp_display_host_deinit(dp); >> + >> + return 0; >> +} >> + >> +static int dp_pm_runtime_resume(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >> + struct dp_display_private *dp; >> + >> + dp = container_of(dp_display, struct dp_display_private, dp_display); >> + >> + dp_display_host_init(dp); >> + if (dp_display->is_edp) { >> + dp_catalog_ctrl_hpd_enable(dp->catalog); >> + dp_display_host_phy_init(dp); >> + } >> >> return 0; >> } >> @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) >> } >> >> static const struct dev_pm_ops dp_pm_ops = { >> + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, NULL) >> .suspend = dp_pm_suspend, >> .resume = dp_pm_resume, >> }; >> @@ -1493,10 +1534,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) >> aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >> >> if (aux_bus && dp->is_edp) { >> - dp_display_host_init(dp_priv); >> - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); >> - dp_display_host_phy_init(dp_priv); > I'm probably just missing it, but how do we get here with the host > powered up and the phy initialized? if (!dp->core_initialized) is at dp_display_host_init() > >> - >> /* >> * The code below assumes that the panel will finish probing >> * by the time devm_of_dp_aux_populate_ep_devices() returns. >> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge, >> dp_hpd_plug_handle(dp_display, 0); >> >> mutex_lock(&dp_display->event_mutex); >> + pm_runtime_get_sync(&dp_display->pdev->dev); >> >> state = dp_display->hpd_state; >> if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { >> @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge, >> } >> >> drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type); >> + >> + pm_runtime_put_sync(&dp_display->pdev->dev); >> mutex_unlock(&dp_display->event_mutex); >> } >> >> @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge) >> struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display); >> >> mutex_lock(&dp->event_mutex); >> + pm_runtime_get_sync(&dp->pdev->dev); >> + >> dp_catalog_ctrl_hpd_enable(dp->catalog); >> >> /* enable HDP interrupts */ >> @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge *bridge) >> dp_catalog_ctrl_hpd_disable(dp->catalog); >> >> dp_display->internal_hpd = false; >> + >> + pm_runtime_mark_last_busy(&dp->pdev->dev); >> + pm_runtime_put_autosuspend(&dp->pdev->dev); >> mutex_unlock(&dp->event_mutex); >> } > The runtime_get/put in dp_bridge_hpd_enable() and disable matches my > expectations. But in the case that we have an external HPD source, where > will the power be turned on? > > Note that you can test this on your device by routing the HPD GPIO to a > display-connector instance and wiring this to the DP node. In the same > way it's done here: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28 > > Regards, > Bjorn > >> >> -- >> 2.7.4 >>
On 10/07/2023 19:18, Kuogee Hsieh wrote: > > On 7/7/2023 5:04 PM, Dmitry Baryshkov wrote: >> On 08/07/2023 02:52, Kuogee Hsieh wrote: >>> Incorporating pm runtime framework into DP driver so that power >>> and clock resource handling can be centralized allowing easier >>> control of these resources in preparation of registering aux bus >>> uring probe. >>> >>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ >>> drivers/gpu/drm/msm/dp/dp_display.c | 75 >>> +++++++++++++++++++++++++++++-------- >>> 2 files changed, 63 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c >>> b/drivers/gpu/drm/msm/dp/dp_aux.c >>> index 8e3b677..c592064 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >>> *dp_aux, >>> return -EINVAL; >>> } >>> + pm_runtime_get_sync(dp_aux->dev); >> >> Let me quote the function's documentation: >> Consider using pm_runtime_resume_and_get() instead of it, especially >> if its return value is checked by the caller, as this is likely to >> result in cleaner code. > > pm_runtime_resume_and_get() will call pm_runtime_resume() every time. > > Since aux_transfer is called very frequently, is it just simple to call > pm_runtiem_get_sync() which will call pm_runtime_reusme() if power > counter is 0 before increased it. > > otherwise it just increase power counter? As you are adding meaningful runtime PM calls, you have to add error checking to these calls. Just calling pm_runtime_get_sync() is not enough. And once you add error handling, you will see what is the difference between two mentioned functions and why one is suggested to be used instead of the other one. > > >> >> So two notes concerning the whole patch: >> - error checking is missing >> - please use pm_runtime_resume_and_get() instead. >> >>> mutex_lock(&aux->mutex); >>> if (!aux->initted) { >>> ret = -EIO; >>> @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >>> *dp_aux, >>> exit: >>> mutex_unlock(&aux->mutex); >>> + pm_runtime_mark_last_busy(dp_aux->dev); >>> + pm_runtime_put_autosuspend(dp_aux->dev); >>> return ret; >>> } >>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>> b/drivers/gpu/drm/msm/dp/dp_display.c >>> index 76f1395..2c5706a 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, >>> struct device *master, >>> goto end; >>> } >>> + pm_runtime_enable(dev); >> >> devm_pm_runtime_enable() removes need for a cleanup. >> >>> + pm_runtime_set_autosuspend_delay(dev, 1000); >>> + pm_runtime_use_autosuspend(dev); >> >> Why do you want to use autosuspend here? >> >>> + >>> return 0; >>> end: >>> return rc; >>> @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, >>> struct device *master, >>> struct dp_display_private *dp = dev_get_dp_display_private(dev); >>> struct msm_drm_private *priv = dev_get_drvdata(master); >>> - /* disable all HPD interrupts */ >>> - if (dp->core_initialized) >>> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, >>> false); >>> + pm_runtime_dont_use_autosuspend(dev); >>> + pm_runtime_disable(dev); >>> kthread_stop(dp->ev_tsk); >>> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct >>> dp_display_private *dp) >>> dp->dp_display.connector_type, dp->core_initialized, >>> dp->phy_initialized); >>> - dp_power_init(dp->power); >>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>> - dp_aux_init(dp->aux); >>> - dp->core_initialized = true; >>> + if (!dp->core_initialized) { >>> + dp_power_init(dp->power); >>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>> + dp_aux_init(dp->aux); >>> + dp->core_initialized = true; >>> + } >> >> Is this relevant to PM runtime? I don't think so. >> >>> } >>> static void dp_display_host_deinit(struct dp_display_private *dp) >>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct >>> dp_display_private *dp) >>> dp->dp_display.connector_type, dp->core_initialized, >>> dp->phy_initialized); >>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>> - dp_aux_deinit(dp->aux); >>> - dp_power_deinit(dp->power); >>> - dp->core_initialized = false; >>> + if (dp->core_initialized) { >>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>> + dp_aux_deinit(dp->aux); >>> + dp_power_deinit(dp->power); >>> + dp->core_initialized = false; >>> + } >>> } >>> static int dp_display_usbpd_configure_cb(struct device *dev) >>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct >>> platform_device *pdev) >>> dp_display_deinit_sub_modules(dp); >>> platform_set_drvdata(pdev, NULL); >>> + pm_runtime_put_sync_suspend(&pdev->dev); >>> + >>> + return 0; >>> +} >>> + >>> +static int dp_pm_runtime_suspend(struct device *dev) >>> +{ >>> + struct platform_device *pdev = to_platform_device(dev); >>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >>> + struct dp_display_private *dp; >>> + >>> + dp = container_of(dp_display, struct dp_display_private, >>> dp_display); >>> + >>> + dp_display_host_phy_exit(dp); >>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >> >> What? NO! >> >>> + dp_display_host_deinit(dp); >>> + >>> + return 0; >>> +} >>> + >>> +static int dp_pm_runtime_resume(struct device *dev) >>> +{ >>> + struct platform_device *pdev = to_platform_device(dev); >>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >>> + struct dp_display_private *dp; >>> + >>> + dp = container_of(dp_display, struct dp_display_private, >>> dp_display); >>> + >>> + dp_display_host_init(dp); >>> + if (dp_display->is_edp) { >>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >>> + dp_display_host_phy_init(dp); >>> + } >>> return 0; >>> } >>> @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) >>> } >>> static const struct dev_pm_ops dp_pm_ops = { >>> + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, >>> NULL) >>> .suspend = dp_pm_suspend, >>> .resume = dp_pm_resume, >> >> With the runtime PM in place, can we change suspend/resume to use >> pm_runtime_force_suspend() and pm_runtime_force_resume() ? > > Let em try if i can move checking device connection status out of > dp_pm_resume(). it handles external dp panel plugin/unplug during > suspend cases. As we discussed during the telco, it does it in a very strange way, which is not compatible with the external HPD support. Thus it should be rewritten anyway. Yes, this change can be handled in a separate patch, What bugs me here is that I don't see runtime PM calls in the device's suspend/resume path. Shouldn't you put the runtime PM status in suspend path? > >> >> >>> }; >>> @@ -1493,10 +1534,6 @@ static int dp_display_get_next_bridge(struct >>> msm_dp *dp) >>> aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >>> if (aux_bus && dp->is_edp) { >>> - dp_display_host_init(dp_priv); >>> - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); >>> - dp_display_host_phy_init(dp_priv); >> >> Are you going to populate the AUX bus (which can cause AUX bus access) >> without waking up the device? > >> devm_of_dp_aux_populate_ep_devices() ==> will call >> pm_runtiemget_sync() internally which will call pm_runtime_resume() to >> wake dp driver >>> - >>> /* >>> * The code below assumes that the panel will finish probing >>> * by the time devm_of_dp_aux_populate_ep_devices() returns. >>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge >>> *drm_bridge, >>> dp_hpd_plug_handle(dp_display, 0); >> >> Nearly the same question. Resume device before accessing registers. >> >>> mutex_lock(&dp_display->event_mutex); >>> + pm_runtime_get_sync(&dp_display->pdev->dev); >>> state = dp_display->hpd_state; >>> if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { >>> @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct >>> drm_bridge *drm_bridge, >>> } >>> drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type); >>> + >>> + pm_runtime_put_sync(&dp_display->pdev->dev); >>> mutex_unlock(&dp_display->event_mutex); >>> } >>> @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct drm_bridge >>> *bridge) >>> struct dp_display_private *dp = container_of(dp_display, struct >>> dp_display_private, dp_display); >>> mutex_lock(&dp->event_mutex); >>> + pm_runtime_get_sync(&dp->pdev->dev); >>> + >>> dp_catalog_ctrl_hpd_enable(dp->catalog); >>> /* enable HDP interrupts */ >>> @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge >>> *bridge) >>> dp_catalog_ctrl_hpd_disable(dp->catalog); >>> dp_display->internal_hpd = false; >>> + >>> + pm_runtime_mark_last_busy(&dp->pdev->dev); >>> + pm_runtime_put_autosuspend(&dp->pdev->dev); >>> mutex_unlock(&dp->event_mutex); >>> } >>
On 7/7/2023 5:04 PM, Dmitry Baryshkov wrote: > On 08/07/2023 02:52, Kuogee Hsieh wrote: >> Incorporating pm runtime framework into DP driver so that power >> and clock resource handling can be centralized allowing easier >> control of these resources in preparation of registering aux bus >> uring probe. >> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ >> drivers/gpu/drm/msm/dp/dp_display.c | 75 >> +++++++++++++++++++++++++++++-------- >> 2 files changed, 63 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c >> b/drivers/gpu/drm/msm/dp/dp_aux.c >> index 8e3b677..c592064 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >> *dp_aux, >> return -EINVAL; >> } >> + pm_runtime_get_sync(dp_aux->dev); > > Let me quote the function's documentation: > Consider using pm_runtime_resume_and_get() instead of it, especially > if its return value is checked by the caller, as this is likely to > result in cleaner code. > > So two notes concerning the whole patch: > - error checking is missing > - please use pm_runtime_resume_and_get() instead. > >> mutex_lock(&aux->mutex); >> if (!aux->initted) { >> ret = -EIO; >> @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >> *dp_aux, >> exit: >> mutex_unlock(&aux->mutex); >> + pm_runtime_mark_last_busy(dp_aux->dev); >> + pm_runtime_put_autosuspend(dp_aux->dev); >> return ret; >> } >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >> b/drivers/gpu/drm/msm/dp/dp_display.c >> index 76f1395..2c5706a 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, >> struct device *master, >> goto end; >> } >> + pm_runtime_enable(dev); > > devm_pm_runtime_enable() removes need for a cleanup. > >> + pm_runtime_set_autosuspend_delay(dev, 1000); >> + pm_runtime_use_autosuspend(dev); > > Why do you want to use autosuspend here? > >> + >> return 0; >> end: >> return rc; >> @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, >> struct device *master, >> struct dp_display_private *dp = dev_get_dp_display_private(dev); >> struct msm_drm_private *priv = dev_get_drvdata(master); >> - /* disable all HPD interrupts */ >> - if (dp->core_initialized) >> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, >> false); >> + pm_runtime_dont_use_autosuspend(dev); >> + pm_runtime_disable(dev); >> kthread_stop(dp->ev_tsk); >> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct >> dp_display_private *dp) >> dp->dp_display.connector_type, dp->core_initialized, >> dp->phy_initialized); >> - dp_power_init(dp->power); >> - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >> - dp_aux_init(dp->aux); >> - dp->core_initialized = true; >> + if (!dp->core_initialized) { >> + dp_power_init(dp->power); >> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >> + dp_aux_init(dp->aux); >> + dp->core_initialized = true; >> + } > > Is this relevant to PM runtime? I don't think so. > >> } >> static void dp_display_host_deinit(struct dp_display_private *dp) >> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct >> dp_display_private *dp) >> dp->dp_display.connector_type, dp->core_initialized, >> dp->phy_initialized); >> - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >> - dp_aux_deinit(dp->aux); >> - dp_power_deinit(dp->power); >> - dp->core_initialized = false; >> + if (dp->core_initialized) { >> + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >> + dp_aux_deinit(dp->aux); >> + dp_power_deinit(dp->power); >> + dp->core_initialized = false; >> + } >> } >> static int dp_display_usbpd_configure_cb(struct device *dev) >> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct >> platform_device *pdev) >> dp_display_deinit_sub_modules(dp); >> platform_set_drvdata(pdev, NULL); >> + pm_runtime_put_sync_suspend(&pdev->dev); >> + >> + return 0; >> +} >> + >> +static int dp_pm_runtime_suspend(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >> + struct dp_display_private *dp; >> + >> + dp = container_of(dp_display, struct dp_display_private, >> dp_display); >> + >> + dp_display_host_phy_exit(dp); >> + dp_catalog_ctrl_hpd_enable(dp->catalog); > > What? NO! > >> + dp_display_host_deinit(dp); >> + >> + return 0; >> +} >> + >> +static int dp_pm_runtime_resume(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >> + struct dp_display_private *dp; >> + >> + dp = container_of(dp_display, struct dp_display_private, >> dp_display); >> + >> + dp_display_host_init(dp); >> + if (dp_display->is_edp) { >> + dp_catalog_ctrl_hpd_enable(dp->catalog); >> + dp_display_host_phy_init(dp); >> + } >> return 0; >> } >> @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) >> } >> static const struct dev_pm_ops dp_pm_ops = { >> + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, >> NULL) >> .suspend = dp_pm_suspend, >> .resume = dp_pm_resume, > > With the runtime PM in place, can we change suspend/resume to use > pm_runtime_force_suspend() and pm_runtime_force_resume() ? > > >> }; >> @@ -1493,10 +1534,6 @@ static int dp_display_get_next_bridge(struct >> msm_dp *dp) >> aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >> if (aux_bus && dp->is_edp) { >> - dp_display_host_init(dp_priv); >> - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); >> - dp_display_host_phy_init(dp_priv); > > Are you going to populate the AUX bus (which can cause AUX bus access) > without waking up the device? no, pm_runtime_get_sync() will be called inside generic_edp_panel_probe() which will trigger dp_pm_runtime_resume() be called to wake up device (initialize dp host) before retrieve panel_id from edp panel through aux bus. > >> - >> /* >> * The code below assumes that the panel will finish probing >> * by the time devm_of_dp_aux_populate_ep_devices() returns. >> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge >> *drm_bridge, >> dp_hpd_plug_handle(dp_display, 0); > > Nearly the same question. Resume device before accessing registers. > >> mutex_lock(&dp_display->event_mutex); >> + pm_runtime_get_sync(&dp_display->pdev->dev); >> state = dp_display->hpd_state; >> if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { >> @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct >> drm_bridge *drm_bridge, >> } >> drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type); >> + >> + pm_runtime_put_sync(&dp_display->pdev->dev); >> mutex_unlock(&dp_display->event_mutex); >> } >> @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct drm_bridge >> *bridge) >> struct dp_display_private *dp = container_of(dp_display, struct >> dp_display_private, dp_display); >> mutex_lock(&dp->event_mutex); >> + pm_runtime_get_sync(&dp->pdev->dev); >> + >> dp_catalog_ctrl_hpd_enable(dp->catalog); >> /* enable HDP interrupts */ >> @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge >> *bridge) >> dp_catalog_ctrl_hpd_disable(dp->catalog); >> dp_display->internal_hpd = false; >> + >> + pm_runtime_mark_last_busy(&dp->pdev->dev); >> + pm_runtime_put_autosuspend(&dp->pdev->dev); >> mutex_unlock(&dp->event_mutex); >> } >
On 7/10/2023 9:22 AM, Kuogee Hsieh wrote: > > On 7/8/2023 7:52 PM, Bjorn Andersson wrote: >> On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote: >>> Incorporating pm runtime framework into DP driver so that power >>> and clock resource handling can be centralized allowing easier >>> control of these resources in preparation of registering aux bus >>> uring probe. >>> >>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ >>> drivers/gpu/drm/msm/dp/dp_display.c | 75 >>> +++++++++++++++++++++++++++++-------- >>> 2 files changed, 63 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c >>> b/drivers/gpu/drm/msm/dp/dp_aux.c >>> index 8e3b677..c592064 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >>> *dp_aux, >>> return -EINVAL; >>> } >>> + pm_runtime_get_sync(dp_aux->dev); >>> mutex_lock(&aux->mutex); >>> if (!aux->initted) { >>> ret = -EIO; >>> @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >>> *dp_aux, >>> exit: >>> mutex_unlock(&aux->mutex); >>> + pm_runtime_mark_last_busy(dp_aux->dev); >>> + pm_runtime_put_autosuspend(dp_aux->dev); >>> return ret; >>> } >>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>> b/drivers/gpu/drm/msm/dp/dp_display.c >>> index 76f1395..2c5706a 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, >>> struct device *master, >>> goto end; >>> } >>> + pm_runtime_enable(dev); >>> + pm_runtime_set_autosuspend_delay(dev, 1000); >>> + pm_runtime_use_autosuspend(dev); >>> + >>> return 0; >>> end: >>> return rc; >>> @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device >>> *dev, struct device *master, >>> struct dp_display_private *dp = dev_get_dp_display_private(dev); >>> struct msm_drm_private *priv = dev_get_drvdata(master); >>> - /* disable all HPD interrupts */ >>> - if (dp->core_initialized) >>> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, >>> false); >>> + pm_runtime_dont_use_autosuspend(dev); >>> + pm_runtime_disable(dev); >>> kthread_stop(dp->ev_tsk); >>> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct >>> dp_display_private *dp) >>> dp->dp_display.connector_type, dp->core_initialized, >>> dp->phy_initialized); >>> - dp_power_init(dp->power); >>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>> - dp_aux_init(dp->aux); >>> - dp->core_initialized = true; >>> + if (!dp->core_initialized) { >>> + dp_power_init(dp->power); >>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>> + dp_aux_init(dp->aux); >>> + dp->core_initialized = true; >> There are two cases that queries core_initialized, both of those are >> done to avoid accessing the DP block without it first being powered up. >> With the introduction of runtime PM, it seems reasonable to just power >> up the block in those two code paths (and remove the variable). >> >>> + } >>> } >>> static void dp_display_host_deinit(struct dp_display_private *dp) >>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct >>> dp_display_private *dp) >>> dp->dp_display.connector_type, dp->core_initialized, >>> dp->phy_initialized); >>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>> - dp_aux_deinit(dp->aux); >>> - dp_power_deinit(dp->power); >>> - dp->core_initialized = false; >>> + if (dp->core_initialized) { >>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>> + dp_aux_deinit(dp->aux); >>> + dp_power_deinit(dp->power); >>> + dp->core_initialized = false; >>> + } >>> } >>> static int dp_display_usbpd_configure_cb(struct device *dev) >>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct >>> platform_device *pdev) >>> dp_display_deinit_sub_modules(dp); >>> platform_set_drvdata(pdev, NULL); >>> + pm_runtime_put_sync_suspend(&pdev->dev); >>> + >>> + return 0; >>> +} >>> + >>> +static int dp_pm_runtime_suspend(struct device *dev) >>> +{ >>> + struct platform_device *pdev = to_platform_device(dev); >>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >> platform_get_drvdata() is a wrapper for dev_get_drvdata(&pdev->dev), so >> there's no need to resolve the platform_device first... >> >>> + struct dp_display_private *dp; >>> + >>> + dp = container_of(dp_display, struct dp_display_private, >>> dp_display); >>> + >>> + dp_display_host_phy_exit(dp); >>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >>> + dp_display_host_deinit(dp); >>> + >>> + return 0; >>> +} >>> + >>> +static int dp_pm_runtime_resume(struct device *dev) >>> +{ >>> + struct platform_device *pdev = to_platform_device(dev); >>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >>> + struct dp_display_private *dp; >>> + >>> + dp = container_of(dp_display, struct dp_display_private, >>> dp_display); >>> + >>> + dp_display_host_init(dp); >>> + if (dp_display->is_edp) { >>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >>> + dp_display_host_phy_init(dp); >>> + } >>> return 0; >>> } >>> @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) >>> } >>> static const struct dev_pm_ops dp_pm_ops = { >>> + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, >>> NULL) >>> .suspend = dp_pm_suspend, >>> .resume = dp_pm_resume, >>> }; >>> @@ -1493,10 +1534,6 @@ static int dp_display_get_next_bridge(struct >>> msm_dp *dp) >>> aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >>> if (aux_bus && dp->is_edp) { >>> - dp_display_host_init(dp_priv); >>> - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); >>> - dp_display_host_phy_init(dp_priv); >> I'm probably just missing it, but how do we get here with the host >> powered up and the phy initialized? > > if (!dp->core_initialized) is at dp_display_host_init() > >> >>> - >>> /* >>> * The code below assumes that the panel will finish probing >>> * by the time devm_of_dp_aux_populate_ep_devices() returns. >>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge >>> *drm_bridge, >>> dp_hpd_plug_handle(dp_display, 0); >>> mutex_lock(&dp_display->event_mutex); >>> + pm_runtime_get_sync(&dp_display->pdev->dev); >>> state = dp_display->hpd_state; >>> if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { >>> @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct >>> drm_bridge *drm_bridge, >>> } >>> drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type); >>> + >>> + pm_runtime_put_sync(&dp_display->pdev->dev); >>> mutex_unlock(&dp_display->event_mutex); >>> } >>> @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct drm_bridge >>> *bridge) >>> struct dp_display_private *dp = container_of(dp_display, >>> struct dp_display_private, dp_display); >>> mutex_lock(&dp->event_mutex); >>> + pm_runtime_get_sync(&dp->pdev->dev); >>> + >>> dp_catalog_ctrl_hpd_enable(dp->catalog); >>> /* enable HDP interrupts */ >>> @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge >>> *bridge) >>> dp_catalog_ctrl_hpd_disable(dp->catalog); >>> dp_display->internal_hpd = false; >>> + >>> + pm_runtime_mark_last_busy(&dp->pdev->dev); >>> + pm_runtime_put_autosuspend(&dp->pdev->dev); >>> mutex_unlock(&dp->event_mutex); >>> } >> The runtime_get/put in dp_bridge_hpd_enable() and disable matches my >> expectations. But in the case that we have an external HPD source, where >> will the power be turned on? >> >> Note that you can test this on your device by routing the HPD GPIO to a >> display-connector instance and wiring this to the DP node. In the same >> way it's done here: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28 >> at sc7280, gpio-47 has function 2 as dp-hot-plug pin. but it does not has function for general purpose pin. Just curious, to work with external HPD source, 1) which DRM_BRIDGE_OP_xxx should be used for bridge->ops? 2) are both .hpd_enable and .hpd_disable have to be populated? >> >> Regards, >> Bjorn >> >>> -- >>> 2.7.4 >>>
On 26/07/2023 01:25, Kuogee Hsieh wrote: > > On 7/10/2023 9:22 AM, Kuogee Hsieh wrote: >> >> On 7/8/2023 7:52 PM, Bjorn Andersson wrote: >>> On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote: >>>> Incorporating pm runtime framework into DP driver so that power >>>> and clock resource handling can be centralized allowing easier >>>> control of these resources in preparation of registering aux bus >>>> uring probe. >>>> >>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ >>>> drivers/gpu/drm/msm/dp/dp_display.c | 75 >>>> +++++++++++++++++++++++++++++-------- >>>> 2 files changed, 63 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c >>>> b/drivers/gpu/drm/msm/dp/dp_aux.c >>>> index 8e3b677..c592064 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >>>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >>>> *dp_aux, >>>> return -EINVAL; >>>> } >>>> + pm_runtime_get_sync(dp_aux->dev); >>>> mutex_lock(&aux->mutex); >>>> if (!aux->initted) { >>>> ret = -EIO; >>>> @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux >>>> *dp_aux, >>>> exit: >>>> mutex_unlock(&aux->mutex); >>>> + pm_runtime_mark_last_busy(dp_aux->dev); >>>> + pm_runtime_put_autosuspend(dp_aux->dev); >>>> return ret; >>>> } >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>> index 76f1395..2c5706a 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, >>>> struct device *master, >>>> goto end; >>>> } >>>> + pm_runtime_enable(dev); >>>> + pm_runtime_set_autosuspend_delay(dev, 1000); >>>> + pm_runtime_use_autosuspend(dev); >>>> + >>>> return 0; >>>> end: >>>> return rc; >>>> @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device >>>> *dev, struct device *master, >>>> struct dp_display_private *dp = dev_get_dp_display_private(dev); >>>> struct msm_drm_private *priv = dev_get_drvdata(master); >>>> - /* disable all HPD interrupts */ >>>> - if (dp->core_initialized) >>>> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, >>>> false); >>>> + pm_runtime_dont_use_autosuspend(dev); >>>> + pm_runtime_disable(dev); >>>> kthread_stop(dp->ev_tsk); >>>> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct >>>> dp_display_private *dp) >>>> dp->dp_display.connector_type, dp->core_initialized, >>>> dp->phy_initialized); >>>> - dp_power_init(dp->power); >>>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>>> - dp_aux_init(dp->aux); >>>> - dp->core_initialized = true; >>>> + if (!dp->core_initialized) { >>>> + dp_power_init(dp->power); >>>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>>> + dp_aux_init(dp->aux); >>>> + dp->core_initialized = true; >>> There are two cases that queries core_initialized, both of those are >>> done to avoid accessing the DP block without it first being powered up. >>> With the introduction of runtime PM, it seems reasonable to just power >>> up the block in those two code paths (and remove the variable). >>> >>>> + } >>>> } >>>> static void dp_display_host_deinit(struct dp_display_private *dp) >>>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct >>>> dp_display_private *dp) >>>> dp->dp_display.connector_type, dp->core_initialized, >>>> dp->phy_initialized); >>>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>>> - dp_aux_deinit(dp->aux); >>>> - dp_power_deinit(dp->power); >>>> - dp->core_initialized = false; >>>> + if (dp->core_initialized) { >>>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>>> + dp_aux_deinit(dp->aux); >>>> + dp_power_deinit(dp->power); >>>> + dp->core_initialized = false; >>>> + } >>>> } >>>> static int dp_display_usbpd_configure_cb(struct device *dev) >>>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct >>>> platform_device *pdev) >>>> dp_display_deinit_sub_modules(dp); >>>> platform_set_drvdata(pdev, NULL); >>>> + pm_runtime_put_sync_suspend(&pdev->dev); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int dp_pm_runtime_suspend(struct device *dev) >>>> +{ >>>> + struct platform_device *pdev = to_platform_device(dev); >>>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >>> platform_get_drvdata() is a wrapper for dev_get_drvdata(&pdev->dev), so >>> there's no need to resolve the platform_device first... >>> >>>> + struct dp_display_private *dp; >>>> + >>>> + dp = container_of(dp_display, struct dp_display_private, >>>> dp_display); >>>> + >>>> + dp_display_host_phy_exit(dp); >>>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >>>> + dp_display_host_deinit(dp); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int dp_pm_runtime_resume(struct device *dev) >>>> +{ >>>> + struct platform_device *pdev = to_platform_device(dev); >>>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >>>> + struct dp_display_private *dp; >>>> + >>>> + dp = container_of(dp_display, struct dp_display_private, >>>> dp_display); >>>> + >>>> + dp_display_host_init(dp); >>>> + if (dp_display->is_edp) { >>>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >>>> + dp_display_host_phy_init(dp); >>>> + } >>>> return 0; >>>> } >>>> @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) >>>> } >>>> static const struct dev_pm_ops dp_pm_ops = { >>>> + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, >>>> NULL) >>>> .suspend = dp_pm_suspend, >>>> .resume = dp_pm_resume, >>>> }; >>>> @@ -1493,10 +1534,6 @@ static int dp_display_get_next_bridge(struct >>>> msm_dp *dp) >>>> aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >>>> if (aux_bus && dp->is_edp) { >>>> - dp_display_host_init(dp_priv); >>>> - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); >>>> - dp_display_host_phy_init(dp_priv); >>> I'm probably just missing it, but how do we get here with the host >>> powered up and the phy initialized? >> >> if (!dp->core_initialized) is at dp_display_host_init() >> >>> >>>> - >>>> /* >>>> * The code below assumes that the panel will finish probing >>>> * by the time devm_of_dp_aux_populate_ep_devices() returns. >>>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge >>>> *drm_bridge, >>>> dp_hpd_plug_handle(dp_display, 0); >>>> mutex_lock(&dp_display->event_mutex); >>>> + pm_runtime_get_sync(&dp_display->pdev->dev); >>>> state = dp_display->hpd_state; >>>> if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { >>>> @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct >>>> drm_bridge *drm_bridge, >>>> } >>>> drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type); >>>> + >>>> + pm_runtime_put_sync(&dp_display->pdev->dev); >>>> mutex_unlock(&dp_display->event_mutex); >>>> } >>>> @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct drm_bridge >>>> *bridge) >>>> struct dp_display_private *dp = container_of(dp_display, >>>> struct dp_display_private, dp_display); >>>> mutex_lock(&dp->event_mutex); >>>> + pm_runtime_get_sync(&dp->pdev->dev); >>>> + >>>> dp_catalog_ctrl_hpd_enable(dp->catalog); >>>> /* enable HDP interrupts */ >>>> @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge >>>> *bridge) >>>> dp_catalog_ctrl_hpd_disable(dp->catalog); >>>> dp_display->internal_hpd = false; >>>> + >>>> + pm_runtime_mark_last_busy(&dp->pdev->dev); >>>> + pm_runtime_put_autosuspend(&dp->pdev->dev); >>>> mutex_unlock(&dp->event_mutex); >>>> } >>> The runtime_get/put in dp_bridge_hpd_enable() and disable matches my >>> expectations. But in the case that we have an external HPD source, where >>> will the power be turned on? >>> >>> Note that you can test this on your device by routing the HPD GPIO to a >>> display-connector instance and wiring this to the DP node. In the same >>> way it's done here: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28 > > at sc7280, gpio-47 has function 2 as dp-hot-plug pin. but it does not > has function for general purpose pin. It has a 'gpio' function, so that the pin can be used as a generic GPIO with a dp-connector device. > > Just curious, to work with external HPD source, > > 1) which DRM_BRIDGE_OP_xxx should be used for bridge->ops? There is no difference. The drm_bridge_connector will select the bridges according to the needs. E.g. the dp-connector can provide DRM_BRIDGE_OP_DETECT / OP_HPD (if the hpd-gpios property is configured). If it does, it will be selected for detection/HPD handling. If not, the main dp bridge will handle these operations. > > 2) are both .hpd_enable and .hpd_disable have to be populated? No, they are both optional.
On 7/25/2023 3:33 PM, Dmitry Baryshkov wrote: > On 26/07/2023 01:25, Kuogee Hsieh wrote: >> >> On 7/10/2023 9:22 AM, Kuogee Hsieh wrote: >>> >>> On 7/8/2023 7:52 PM, Bjorn Andersson wrote: >>>> On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote: >>>>> Incorporating pm runtime framework into DP driver so that power >>>>> and clock resource handling can be centralized allowing easier >>>>> control of these resources in preparation of registering aux bus >>>>> uring probe. >>>>> >>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>>>> --- >>>>> drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ >>>>> drivers/gpu/drm/msm/dp/dp_display.c | 75 >>>>> +++++++++++++++++++++++++++++-------- >>>>> 2 files changed, 63 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c >>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c >>>>> index 8e3b677..c592064 100644 >>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >>>>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct >>>>> drm_dp_aux *dp_aux, >>>>> return -EINVAL; >>>>> } >>>>> + pm_runtime_get_sync(dp_aux->dev); >>>>> mutex_lock(&aux->mutex); >>>>> if (!aux->initted) { >>>>> ret = -EIO; >>>>> @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct >>>>> drm_dp_aux *dp_aux, >>>>> exit: >>>>> mutex_unlock(&aux->mutex); >>>>> + pm_runtime_mark_last_busy(dp_aux->dev); >>>>> + pm_runtime_put_autosuspend(dp_aux->dev); >>>>> return ret; >>>>> } >>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>>> index 76f1395..2c5706a 100644 >>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device >>>>> *dev, struct device *master, >>>>> goto end; >>>>> } >>>>> + pm_runtime_enable(dev); >>>>> + pm_runtime_set_autosuspend_delay(dev, 1000); >>>>> + pm_runtime_use_autosuspend(dev); >>>>> + >>>>> return 0; >>>>> end: >>>>> return rc; >>>>> @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device >>>>> *dev, struct device *master, >>>>> struct dp_display_private *dp = >>>>> dev_get_dp_display_private(dev); >>>>> struct msm_drm_private *priv = dev_get_drvdata(master); >>>>> - /* disable all HPD interrupts */ >>>>> - if (dp->core_initialized) >>>>> - dp_catalog_hpd_config_intr(dp->catalog, >>>>> DP_DP_HPD_INT_MASK, false); >>>>> + pm_runtime_dont_use_autosuspend(dev); >>>>> + pm_runtime_disable(dev); >>>>> kthread_stop(dp->ev_tsk); >>>>> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct >>>>> dp_display_private *dp) >>>>> dp->dp_display.connector_type, dp->core_initialized, >>>>> dp->phy_initialized); >>>>> - dp_power_init(dp->power); >>>>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>>>> - dp_aux_init(dp->aux); >>>>> - dp->core_initialized = true; >>>>> + if (!dp->core_initialized) { >>>>> + dp_power_init(dp->power); >>>>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>>>> + dp_aux_init(dp->aux); >>>>> + dp->core_initialized = true; >>>> There are two cases that queries core_initialized, both of those are >>>> done to avoid accessing the DP block without it first being powered >>>> up. >>>> With the introduction of runtime PM, it seems reasonable to just power >>>> up the block in those two code paths (and remove the variable). >>>> >>>>> + } >>>>> } >>>>> static void dp_display_host_deinit(struct dp_display_private *dp) >>>>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct >>>>> dp_display_private *dp) >>>>> dp->dp_display.connector_type, dp->core_initialized, >>>>> dp->phy_initialized); >>>>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>>>> - dp_aux_deinit(dp->aux); >>>>> - dp_power_deinit(dp->power); >>>>> - dp->core_initialized = false; >>>>> + if (dp->core_initialized) { >>>>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>>>> + dp_aux_deinit(dp->aux); >>>>> + dp_power_deinit(dp->power); >>>>> + dp->core_initialized = false; >>>>> + } >>>>> } >>>>> static int dp_display_usbpd_configure_cb(struct device *dev) >>>>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct >>>>> platform_device *pdev) >>>>> dp_display_deinit_sub_modules(dp); >>>>> platform_set_drvdata(pdev, NULL); >>>>> + pm_runtime_put_sync_suspend(&pdev->dev); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int dp_pm_runtime_suspend(struct device *dev) >>>>> +{ >>>>> + struct platform_device *pdev = to_platform_device(dev); >>>>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >>>> platform_get_drvdata() is a wrapper for >>>> dev_get_drvdata(&pdev->dev), so >>>> there's no need to resolve the platform_device first... >>>> >>>>> + struct dp_display_private *dp; >>>>> + >>>>> + dp = container_of(dp_display, struct dp_display_private, >>>>> dp_display); >>>>> + >>>>> + dp_display_host_phy_exit(dp); >>>>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >>>>> + dp_display_host_deinit(dp); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int dp_pm_runtime_resume(struct device *dev) >>>>> +{ >>>>> + struct platform_device *pdev = to_platform_device(dev); >>>>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >>>>> + struct dp_display_private *dp; >>>>> + >>>>> + dp = container_of(dp_display, struct dp_display_private, >>>>> dp_display); >>>>> + >>>>> + dp_display_host_init(dp); >>>>> + if (dp_display->is_edp) { >>>>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >>>>> + dp_display_host_phy_init(dp); >>>>> + } >>>>> return 0; >>>>> } >>>>> @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) >>>>> } >>>>> static const struct dev_pm_ops dp_pm_ops = { >>>>> + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, >>>>> dp_pm_runtime_resume, NULL) >>>>> .suspend = dp_pm_suspend, >>>>> .resume = dp_pm_resume, >>>>> }; >>>>> @@ -1493,10 +1534,6 @@ static int >>>>> dp_display_get_next_bridge(struct msm_dp *dp) >>>>> aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >>>>> if (aux_bus && dp->is_edp) { >>>>> - dp_display_host_init(dp_priv); >>>>> - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); >>>>> - dp_display_host_phy_init(dp_priv); >>>> I'm probably just missing it, but how do we get here with the host >>>> powered up and the phy initialized? >>> >>> if (!dp->core_initialized) is at dp_display_host_init() >>> >>>> >>>>> - >>>>> /* >>>>> * The code below assumes that the panel will finish >>>>> probing >>>>> * by the time devm_of_dp_aux_populate_ep_devices() >>>>> returns. >>>>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct >>>>> drm_bridge *drm_bridge, >>>>> dp_hpd_plug_handle(dp_display, 0); >>>>> mutex_lock(&dp_display->event_mutex); >>>>> + pm_runtime_get_sync(&dp_display->pdev->dev); >>>>> state = dp_display->hpd_state; >>>>> if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { >>>>> @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct >>>>> drm_bridge *drm_bridge, >>>>> } >>>>> drm_dbg_dp(dp->drm_dev, "type=%d Done\n", >>>>> dp->connector_type); >>>>> + >>>>> + pm_runtime_put_sync(&dp_display->pdev->dev); >>>>> mutex_unlock(&dp_display->event_mutex); >>>>> } >>>>> @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct >>>>> drm_bridge *bridge) >>>>> struct dp_display_private *dp = container_of(dp_display, >>>>> struct dp_display_private, dp_display); >>>>> mutex_lock(&dp->event_mutex); >>>>> + pm_runtime_get_sync(&dp->pdev->dev); >>>>> + >>>>> dp_catalog_ctrl_hpd_enable(dp->catalog); >>>>> /* enable HDP interrupts */ >>>>> @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge >>>>> *bridge) >>>>> dp_catalog_ctrl_hpd_disable(dp->catalog); >>>>> dp_display->internal_hpd = false; >>>>> + >>>>> + pm_runtime_mark_last_busy(&dp->pdev->dev); >>>>> + pm_runtime_put_autosuspend(&dp->pdev->dev); >>>>> mutex_unlock(&dp->event_mutex); >>>>> } >>>> The runtime_get/put in dp_bridge_hpd_enable() and disable matches my >>>> expectations. But in the case that we have an external HPD source, >>>> where >>>> will the power be turned on? >>>> >>>> Note that you can test this on your device by routing the HPD GPIO >>>> to a >>>> display-connector instance and wiring this to the DP node. In the same >>>> way it's done here: >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28 >>>> >> >> at sc7280, gpio-47 has function 2 as dp-hot-plug pin. but it does not >> has function for general purpose pin. > > It has a 'gpio' function, so that the pin can be used as a generic > GPIO with a dp-connector device. function 0 is for PBL_DEBUG2, which function # should I use? > >> >> Just curious, to work with external HPD source, >> >> 1) which DRM_BRIDGE_OP_xxx should be used for bridge->ops? > > There is no difference. The drm_bridge_connector will select the > bridges according to the needs. E.g. the dp-connector can provide > DRM_BRIDGE_OP_DETECT / OP_HPD (if the hpd-gpios property is > configured). If it does, it will be selected for detection/HPD > handling. If not, the main dp bridge will handle these operations. can you please point me out where "hpd-gpios" info get parsed and during polling where the gpio status get read from? Thanks, > >> >> 2) are both .hpd_enable and .hpd_disable have to be populated? > > No, they are both optional. >
On 26/07/2023 02:26, Kuogee Hsieh wrote: > > On 7/25/2023 3:33 PM, Dmitry Baryshkov wrote: >> On 26/07/2023 01:25, Kuogee Hsieh wrote: >>> >>> On 7/10/2023 9:22 AM, Kuogee Hsieh wrote: >>>> >>>> On 7/8/2023 7:52 PM, Bjorn Andersson wrote: >>>>> On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote: >>>>>> Incorporating pm runtime framework into DP driver so that power >>>>>> and clock resource handling can be centralized allowing easier >>>>>> control of these resources in preparation of registering aux bus >>>>>> uring probe. >>>>>> >>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>>>>> --- >>>>>> drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ >>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 75 >>>>>> +++++++++++++++++++++++++++++-------- >>>>>> 2 files changed, 63 insertions(+), 15 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c >>>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c >>>>>> index 8e3b677..c592064 100644 >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >>>>>> @@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct >>>>>> drm_dp_aux *dp_aux, >>>>>> return -EINVAL; >>>>>> } >>>>>> + pm_runtime_get_sync(dp_aux->dev); >>>>>> mutex_lock(&aux->mutex); >>>>>> if (!aux->initted) { >>>>>> ret = -EIO; >>>>>> @@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct >>>>>> drm_dp_aux *dp_aux, >>>>>> exit: >>>>>> mutex_unlock(&aux->mutex); >>>>>> + pm_runtime_mark_last_busy(dp_aux->dev); >>>>>> + pm_runtime_put_autosuspend(dp_aux->dev); >>>>>> return ret; >>>>>> } >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> index 76f1395..2c5706a 100644 >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> @@ -309,6 +309,10 @@ static int dp_display_bind(struct device >>>>>> *dev, struct device *master, >>>>>> goto end; >>>>>> } >>>>>> + pm_runtime_enable(dev); >>>>>> + pm_runtime_set_autosuspend_delay(dev, 1000); >>>>>> + pm_runtime_use_autosuspend(dev); >>>>>> + >>>>>> return 0; >>>>>> end: >>>>>> return rc; >>>>>> @@ -320,9 +324,8 @@ static void dp_display_unbind(struct device >>>>>> *dev, struct device *master, >>>>>> struct dp_display_private *dp = >>>>>> dev_get_dp_display_private(dev); >>>>>> struct msm_drm_private *priv = dev_get_drvdata(master); >>>>>> - /* disable all HPD interrupts */ >>>>>> - if (dp->core_initialized) >>>>>> - dp_catalog_hpd_config_intr(dp->catalog, >>>>>> DP_DP_HPD_INT_MASK, false); >>>>>> + pm_runtime_dont_use_autosuspend(dev); >>>>>> + pm_runtime_disable(dev); >>>>>> kthread_stop(dp->ev_tsk); >>>>>> @@ -466,10 +469,12 @@ static void dp_display_host_init(struct >>>>>> dp_display_private *dp) >>>>>> dp->dp_display.connector_type, dp->core_initialized, >>>>>> dp->phy_initialized); >>>>>> - dp_power_init(dp->power); >>>>>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>>>>> - dp_aux_init(dp->aux); >>>>>> - dp->core_initialized = true; >>>>>> + if (!dp->core_initialized) { >>>>>> + dp_power_init(dp->power); >>>>>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); >>>>>> + dp_aux_init(dp->aux); >>>>>> + dp->core_initialized = true; >>>>> There are two cases that queries core_initialized, both of those are >>>>> done to avoid accessing the DP block without it first being powered >>>>> up. >>>>> With the introduction of runtime PM, it seems reasonable to just power >>>>> up the block in those two code paths (and remove the variable). >>>>> >>>>>> + } >>>>>> } >>>>>> static void dp_display_host_deinit(struct dp_display_private *dp) >>>>>> @@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct >>>>>> dp_display_private *dp) >>>>>> dp->dp_display.connector_type, dp->core_initialized, >>>>>> dp->phy_initialized); >>>>>> - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>>>>> - dp_aux_deinit(dp->aux); >>>>>> - dp_power_deinit(dp->power); >>>>>> - dp->core_initialized = false; >>>>>> + if (dp->core_initialized) { >>>>>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); >>>>>> + dp_aux_deinit(dp->aux); >>>>>> + dp_power_deinit(dp->power); >>>>>> + dp->core_initialized = false; >>>>>> + } >>>>>> } >>>>>> static int dp_display_usbpd_configure_cb(struct device *dev) >>>>>> @@ -1304,6 +1311,39 @@ static int dp_display_remove(struct >>>>>> platform_device *pdev) >>>>>> dp_display_deinit_sub_modules(dp); >>>>>> platform_set_drvdata(pdev, NULL); >>>>>> + pm_runtime_put_sync_suspend(&pdev->dev); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int dp_pm_runtime_suspend(struct device *dev) >>>>>> +{ >>>>>> + struct platform_device *pdev = to_platform_device(dev); >>>>>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >>>>> platform_get_drvdata() is a wrapper for >>>>> dev_get_drvdata(&pdev->dev), so >>>>> there's no need to resolve the platform_device first... >>>>> >>>>>> + struct dp_display_private *dp; >>>>>> + >>>>>> + dp = container_of(dp_display, struct dp_display_private, >>>>>> dp_display); >>>>>> + >>>>>> + dp_display_host_phy_exit(dp); >>>>>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >>>>>> + dp_display_host_deinit(dp); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int dp_pm_runtime_resume(struct device *dev) >>>>>> +{ >>>>>> + struct platform_device *pdev = to_platform_device(dev); >>>>>> + struct msm_dp *dp_display = platform_get_drvdata(pdev); >>>>>> + struct dp_display_private *dp; >>>>>> + >>>>>> + dp = container_of(dp_display, struct dp_display_private, >>>>>> dp_display); >>>>>> + >>>>>> + dp_display_host_init(dp); >>>>>> + if (dp_display->is_edp) { >>>>>> + dp_catalog_ctrl_hpd_enable(dp->catalog); >>>>>> + dp_display_host_phy_init(dp); >>>>>> + } >>>>>> return 0; >>>>>> } >>>>>> @@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev) >>>>>> } >>>>>> static const struct dev_pm_ops dp_pm_ops = { >>>>>> + SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, >>>>>> dp_pm_runtime_resume, NULL) >>>>>> .suspend = dp_pm_suspend, >>>>>> .resume = dp_pm_resume, >>>>>> }; >>>>>> @@ -1493,10 +1534,6 @@ static int >>>>>> dp_display_get_next_bridge(struct msm_dp *dp) >>>>>> aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >>>>>> if (aux_bus && dp->is_edp) { >>>>>> - dp_display_host_init(dp_priv); >>>>>> - dp_catalog_ctrl_hpd_enable(dp_priv->catalog); >>>>>> - dp_display_host_phy_init(dp_priv); >>>>> I'm probably just missing it, but how do we get here with the host >>>>> powered up and the phy initialized? >>>> >>>> if (!dp->core_initialized) is at dp_display_host_init() >>>> >>>>> >>>>>> - >>>>>> /* >>>>>> * The code below assumes that the panel will finish >>>>>> probing >>>>>> * by the time devm_of_dp_aux_populate_ep_devices() >>>>>> returns. >>>>>> @@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct >>>>>> drm_bridge *drm_bridge, >>>>>> dp_hpd_plug_handle(dp_display, 0); >>>>>> mutex_lock(&dp_display->event_mutex); >>>>>> + pm_runtime_get_sync(&dp_display->pdev->dev); >>>>>> state = dp_display->hpd_state; >>>>>> if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) { >>>>>> @@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct >>>>>> drm_bridge *drm_bridge, >>>>>> } >>>>>> drm_dbg_dp(dp->drm_dev, "type=%d Done\n", >>>>>> dp->connector_type); >>>>>> + >>>>>> + pm_runtime_put_sync(&dp_display->pdev->dev); >>>>>> mutex_unlock(&dp_display->event_mutex); >>>>>> } >>>>>> @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct >>>>>> drm_bridge *bridge) >>>>>> struct dp_display_private *dp = container_of(dp_display, >>>>>> struct dp_display_private, dp_display); >>>>>> mutex_lock(&dp->event_mutex); >>>>>> + pm_runtime_get_sync(&dp->pdev->dev); >>>>>> + >>>>>> dp_catalog_ctrl_hpd_enable(dp->catalog); >>>>>> /* enable HDP interrupts */ >>>>>> @@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge >>>>>> *bridge) >>>>>> dp_catalog_ctrl_hpd_disable(dp->catalog); >>>>>> dp_display->internal_hpd = false; >>>>>> + >>>>>> + pm_runtime_mark_last_busy(&dp->pdev->dev); >>>>>> + pm_runtime_put_autosuspend(&dp->pdev->dev); >>>>>> mutex_unlock(&dp->event_mutex); >>>>>> } >>>>> The runtime_get/put in dp_bridge_hpd_enable() and disable matches my >>>>> expectations. But in the case that we have an external HPD source, >>>>> where >>>>> will the power be turned on? >>>>> >>>>> Note that you can test this on your device by routing the HPD GPIO >>>>> to a >>>>> display-connector instance and wiring this to the DP node. In the same >>>>> way it's done here: >>>>> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28 >>> >>> at sc7280, gpio-47 has function 2 as dp-hot-plug pin. but it does not >>> has function for general purpose pin. >> >> It has a 'gpio' function, so that the pin can be used as a generic >> GPIO with a dp-connector device. > function 0 is for PBL_DEBUG2, which function # should I use? does `function = "gpio"` work? >> >>> >>> Just curious, to work with external HPD source, >>> >>> 1) which DRM_BRIDGE_OP_xxx should be used for bridge->ops? >> >> There is no difference. The drm_bridge_connector will select the >> bridges according to the needs. E.g. the dp-connector can provide >> DRM_BRIDGE_OP_DETECT / OP_HPD (if the hpd-gpios property is >> configured). If it does, it will be selected for detection/HPD >> handling. If not, the main dp bridge will handle these operations. > > can you please point me out where "hpd-gpios" info get parsed and > during polling where the gpio status get read from? Can you please take a look at the display-controller.c driver? > > Thanks, > >> >>> >>> 2) are both .hpd_enable and .hpd_disable have to be populated? >> >> No, they are both optional. >>
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); }
Incorporating pm runtime framework into DP driver so that power and clock resource handling can be centralized allowing easier control of these resources in preparation of registering aux bus uring probe. Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_aux.c | 3 ++ drivers/gpu/drm/msm/dp/dp_display.c | 75 +++++++++++++++++++++++++++++-------- 2 files changed, 63 insertions(+), 15 deletions(-)