| Message ID | 20180130202913.28724-6-thierry.escande@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande: > From: zain wang <wzz@rock-chips.com> > > The bridge does not need to be powered in analogix_dp_bind(), so > remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp() > as well as their power-off counterparts. > > Cc: Stéphane Marchesin <marcheu@chromium.org> > Signed-off-by: zain wang <wzz@rock-chips.com> > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > [the patch originally just removed the power_on portion, seanpaul removed > the power off code as well as improved the commit message] > Signed-off-by: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com> > --- > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index > cb5e18d6ba04..1477ea9ba85d 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct > drm_device *drm_dev, > > pm_runtime_enable(dev); > > - pm_runtime_get_sync(dev); > - phy_power_on(dp->phy); > - > - analogix_dp_init_dp(dp); > - > ret = devm_request_threaded_irq(&pdev->dev, dp->irq, > analogix_dp_hardirq, > analogix_dp_irq_thread, Not 100% sure here, as the driver has the request-irq + disable-irq hack here. So a pending interrupt could possibly fire between request and disable. Right now the block should be on, but can it still handle such an irq when the power is removed? So before removing the power here, we might want something similar to what Marc posted for the vop [0] for the analogix-dp? Heiko [0] https://patchwork.kernel.org/patch/10210513/
On 28/02/18 14:37, Heiko Stübner wrote: > Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande: >> From: zain wang <wzz@rock-chips.com> >> >> The bridge does not need to be powered in analogix_dp_bind(), so >> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp() >> as well as their power-off counterparts. >> >> Cc: Stéphane Marchesin <marcheu@chromium.org> >> Signed-off-by: zain wang <wzz@rock-chips.com> >> Signed-off-by: Caesar Wang <wxt@rock-chips.com> >> [the patch originally just removed the power_on portion, seanpaul removed >> the power off code as well as improved the commit message] >> Signed-off-by: Sean Paul <seanpaul@chromium.org> >> Signed-off-by: Thierry Escande <thierry.escande@collabora.com> >> --- >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ---------- >> 1 file changed, 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index >> cb5e18d6ba04..1477ea9ba85d 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct >> drm_device *drm_dev, >> >> pm_runtime_enable(dev); >> >> - pm_runtime_get_sync(dev); >> - phy_power_on(dp->phy); >> - >> - analogix_dp_init_dp(dp); >> - >> ret = devm_request_threaded_irq(&pdev->dev, dp->irq, >> analogix_dp_hardirq, >> analogix_dp_irq_thread, > > Not 100% sure here, as the driver has the request-irq + disable-irq hack > here. So a pending interrupt could possibly fire between request and > disable. > > Right now the block should be on, but can it still handle such an irq > when the power is removed? Probably not (see below). > So before removing the power here, we might want something > similar to what Marc posted for the vop [0] for the analogix-dp? You can do that trick only if the interrupt is not shared. In the VOP case, it is shared with the IOMMU, which makes it more... interesting. And when it comes to power and the analogix-dp driver, I've been carrying this[1] for a while. Fully exploitable from userspace. I know it is about to be replaced by this series, but at least 4.15 and 4.16 are affected. M. [1] https://www.spinics.net/lists/arm-kernel/msg623892.html
Am Mittwoch, 28. Februar 2018, 15:54:30 CET schrieb Marc Zyngier: > On 28/02/18 14:37, Heiko Stübner wrote: > > Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande: > >> From: zain wang <wzz@rock-chips.com> > >> > >> The bridge does not need to be powered in analogix_dp_bind(), so > >> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp() > >> as well as their power-off counterparts. > >> > >> Cc: Stéphane Marchesin <marcheu@chromium.org> > >> Signed-off-by: zain wang <wzz@rock-chips.com> > >> Signed-off-by: Caesar Wang <wxt@rock-chips.com> > >> [the patch originally just removed the power_on portion, seanpaul removed > >> the power off code as well as improved the commit message] > >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > >> Signed-off-by: Thierry Escande <thierry.escande@collabora.com> > >> --- > >> > >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ---------- > >> 1 file changed, 10 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index > >> cb5e18d6ba04..1477ea9ba85d 100644 > >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > >> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct > >> drm_device *drm_dev, > >> > >> pm_runtime_enable(dev); > >> > >> - pm_runtime_get_sync(dev); > >> - phy_power_on(dp->phy); > >> - > >> - analogix_dp_init_dp(dp); > >> - > >> > >> ret = devm_request_threaded_irq(&pdev->dev, dp->irq, > >> > >> analogix_dp_hardirq, > >> analogix_dp_irq_thread, > > > > Not 100% sure here, as the driver has the request-irq + disable-irq hack > > here. So a pending interrupt could possibly fire between request and > > disable. > > > > Right now the block should be on, but can it still handle such an irq > > when the power is removed? > > Probably not (see below). > > > So before removing the power here, we might want something > > similar to what Marc posted for the vop [0] for the analogix-dp? > > You can do that trick only if the interrupt is not shared. In the VOP > case, it is shared with the IOMMU, which makes it more... interesting. Yep, which is why I mentioned it, as the dp-irq should not be shared I'd think :-) Heiko
Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande: > From: zain wang <wzz@rock-chips.com> > > The bridge does not need to be powered in analogix_dp_bind(), so > remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp() > as well as their power-off counterparts. > > Cc: Stéphane Marchesin <marcheu@chromium.org> > Signed-off-by: zain wang <wzz@rock-chips.com> > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > [the patch originally just removed the power_on portion, seanpaul removed > the power off code as well as improved the commit message] > Signed-off-by: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com> > --- > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index > cb5e18d6ba04..1477ea9ba85d 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct > drm_device *drm_dev, > > pm_runtime_enable(dev); > > - pm_runtime_get_sync(dev); > - phy_power_on(dp->phy); > - > - analogix_dp_init_dp(dp); > - > ret = devm_request_threaded_irq(&pdev->dev, dp->irq, > analogix_dp_hardirq, > analogix_dp_irq_thread, > @@ -1414,15 +1409,10 @@ analogix_dp_bind(struct device *dev, struct > drm_device *drm_dev, goto err_disable_pm_runtime; > } > > - phy_power_off(dp->phy); > - pm_runtime_put(dev); > - > return dp; > > err_disable_pm_runtime: > > - phy_power_off(dp->phy); > - pm_runtime_put(dev); > pm_runtime_disable(dev); > > return ERR_PTR(ret); In general, this patch seems to also create the opposite than "drm/bridge: analogix_dp: Keep PHY powered between driver bind/unbind" [0] posted on monday? [0] https://patchwork.kernel.org/patch/10242493/
Hi Heiko, Thanks for adding me to this thread. On 2018-02-28 16:20, Heiko Stübner wrote: > Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande: >> From: zain wang <wzz@rock-chips.com> >> >> The bridge does not need to be powered in analogix_dp_bind(), so >> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp() >> as well as their power-off counterparts. >> >> Cc: Stéphane Marchesin <marcheu@chromium.org> >> Signed-off-by: zain wang <wzz@rock-chips.com> >> Signed-off-by: Caesar Wang <wxt@rock-chips.com> >> [the patch originally just removed the power_on portion, seanpaul removed >> the power off code as well as improved the commit message] >> Signed-off-by: Sean Paul <seanpaul@chromium.org> >> Signed-off-by: Thierry Escande <thierry.escande@collabora.com> >> --- >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ---------- >> 1 file changed, 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index >> cb5e18d6ba04..1477ea9ba85d 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct >> drm_device *drm_dev, >> >> pm_runtime_enable(dev); >> >> - pm_runtime_get_sync(dev); >> - phy_power_on(dp->phy); >> - >> - analogix_dp_init_dp(dp); >> - >> ret = devm_request_threaded_irq(&pdev->dev, dp->irq, >> analogix_dp_hardirq, >> analogix_dp_irq_thread, >> @@ -1414,15 +1409,10 @@ analogix_dp_bind(struct device *dev, struct >> drm_device *drm_dev, goto err_disable_pm_runtime; >> } >> >> - phy_power_off(dp->phy); >> - pm_runtime_put(dev); >> - >> return dp; >> >> err_disable_pm_runtime: >> >> - phy_power_off(dp->phy); >> - pm_runtime_put(dev); >> pm_runtime_disable(dev); >> >> return ERR_PTR(ret); > In general, this patch seems to also create the opposite than > "drm/bridge: analogix_dp: Keep PHY powered between driver bind/unbind" [0] > > posted on monday? > > [0] https://patchwork.kernel.org/patch/10242493/ Well, my patch was a quick workaround to avoid board freeze. This patch looks like a proper fix. Besides removing runtime pm and phy power calls from dp_bind, it also removes dp register access done in analogix_dp_init_dp, as there is really no need to touch registers in bind operation. The patchset however suffers from other issues on Exynos hardware. I will post them in that thread. Best regards
Hi, On 2018-01-30 21:28, Thierry Escande wrote: > From: zain wang <wzz@rock-chips.com> > > The bridge does not need to be powered in analogix_dp_bind(), so > remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp() > as well as their power-off counterparts. > > Cc: Stéphane Marchesin <marcheu@chromium.org> > Signed-off-by: zain wang <wzz@rock-chips.com> > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > [the patch originally just removed the power_on portion, seanpaul removed > the power off code as well as improved the commit message] > Signed-off-by: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com> > --- > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index cb5e18d6ba04..1477ea9ba85d 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, > > pm_runtime_enable(dev); > > - pm_runtime_get_sync(dev); > - phy_power_on(dp->phy); > - > - analogix_dp_init_dp(dp); > - > ret = devm_request_threaded_irq(&pdev->dev, dp->irq, > analogix_dp_hardirq, > analogix_dp_irq_thread, > @@ -1414,15 +1409,10 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, > goto err_disable_pm_runtime; > } > > - phy_power_off(dp->phy); > - pm_runtime_put(dev); > - > return dp; > > err_disable_pm_runtime: > > - phy_power_off(dp->phy); > - pm_runtime_put(dev); > pm_runtime_disable(dev); > > return ERR_PTR(ret); Once this change is applied, there is also no need to keep dp->clock prepared & enabled between bind/unbind. analogix_dp_set_bridge() and analogix_dp_bridge_disable() properly manage dp->clock on their own. Best regards
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index cb5e18d6ba04..1477ea9ba85d 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, pm_runtime_enable(dev); - pm_runtime_get_sync(dev); - phy_power_on(dp->phy); - - analogix_dp_init_dp(dp); - ret = devm_request_threaded_irq(&pdev->dev, dp->irq, analogix_dp_hardirq, analogix_dp_irq_thread, @@ -1414,15 +1409,10 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev, goto err_disable_pm_runtime; } - phy_power_off(dp->phy); - pm_runtime_put(dev); - return dp; err_disable_pm_runtime: - phy_power_off(dp->phy); - pm_runtime_put(dev); pm_runtime_disable(dev); return ERR_PTR(ret);