| Message ID | 20180221100443.13041-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Marek, Thank you for the patch. On Wednesday, 21 February 2018 12:04:43 EET Marek Szyprowski wrote: > Patch f0a8b49c03d2 ("drm/bridge: analogix dp: Fix runtime PM state on > driver bind") fixed unbalanced call to phy_power_on() in analogix_dp_bind() > function by calling phy_power_off() at the end of bind operation. > > However it turned out that having PHY powered is required for proper DRM > display pipeline initialization on Peach-Pit Chromebook2 board, as this > board freezes otherwise when PHY power off is called in bind. Fix this by > keeping PHY powered for the whole bind/unbind driver life cycle. The > freeze is probably related to the fact that the display pipeline (Exynos > FIMD CTRC -> Exynos/Analogix DP bridge -> PS8625 dp2lvds bridge -> B116XW03 > panel) is already configured and enabled by the bootloader and require > reset of all components for proper shutdown. "Probably" always makes me feel uncomfortable in such a context :-) Leaving the PHY powered at all times isn't the best idea as it will increase power consumption even when the DP output isn't in use. I can live with that if there's no other way, but it feels to me like we should first get to the bottom of the issue. > Having PHY powered is also needed for proper EDID handling, which initially > fixed by commit 510353a63796 ("drm/bridge: analogix dp: Fix runtime PM state > in get_modes() callback"). > > Fixes: f0a8b49c03d2 ("drm/bridge: analogix dp: Fix runtime PM state on > driver bind") > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index > b2756bc4225a..42595863cb03 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -1415,7 +1415,6 @@ int 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 0; > @@ -1448,6 +1447,7 @@ void analogix_dp_unbind(struct device *dev, struct > device *master, > > drm_dp_aux_unregister(&dp->aux); > pm_runtime_disable(dev); > + phy_power_off(dp->phy); > clk_disable_unprepare(dp->clock); > } > EXPORT_SYMBOL_GPL(analogix_dp_unbind);
Hi Laurent, On 2018-02-21 13:49, Laurent Pinchart wrote: > Thank you for the patch. > > On Wednesday, 21 February 2018 12:04:43 EET Marek Szyprowski wrote: >> Patch f0a8b49c03d2 ("drm/bridge: analogix dp: Fix runtime PM state on >> driver bind") fixed unbalanced call to phy_power_on() in analogix_dp_bind() >> function by calling phy_power_off() at the end of bind operation. >> >> However it turned out that having PHY powered is required for proper DRM >> display pipeline initialization on Peach-Pit Chromebook2 board, as this >> board freezes otherwise when PHY power off is called in bind. Fix this by >> keeping PHY powered for the whole bind/unbind driver life cycle. The >> freeze is probably related to the fact that the display pipeline (Exynos >> FIMD CTRC -> Exynos/Analogix DP bridge -> PS8625 dp2lvds bridge -> B116XW03 >> panel) is already configured and enabled by the bootloader and require >> reset of all components for proper shutdown. > "Probably" always makes me feel uncomfortable in such a context :-) Leaving > the PHY powered at all times isn't the best idea as it will increase power > consumption even when the DP output isn't in use. I can live with that if > there's no other way, but it feels to me like we should first get to the > bottom of the issue. Frankly I spent some time on this and I have no other idea. Board freezes when phy_power_off is called and the initial code worked properly on that board by leaking phy turned on all the time. >> Having PHY powered is also needed for proper EDID handling, which initially >> fixed by commit 510353a63796 ("drm/bridge: analogix dp: Fix runtime PM state >> in get_modes() callback"). >> >> Fixes: f0a8b49c03d2 ("drm/bridge: analogix dp: Fix runtime PM state on >> driver bind") >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index >> b2756bc4225a..42595863cb03 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> @@ -1415,7 +1415,6 @@ int 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 0; >> @@ -1448,6 +1447,7 @@ void analogix_dp_unbind(struct device *dev, struct >> device *master, >> >> drm_dp_aux_unregister(&dp->aux); >> pm_runtime_disable(dev); >> + phy_power_off(dp->phy); >> clk_disable_unprepare(dp->clock); >> } >> EXPORT_SYMBOL_GPL(analogix_dp_unbind); Best regards
Hi Marek, On Friday, 23 February 2018 09:35:32 EET Marek Szyprowski wrote: > On 2018-02-21 13:49, Laurent Pinchart wrote: > > On Wednesday, 21 February 2018 12:04:43 EET Marek Szyprowski wrote: > >> Patch f0a8b49c03d2 ("drm/bridge: analogix dp: Fix runtime PM state on > >> driver bind") fixed unbalanced call to phy_power_on() in > >> analogix_dp_bind() function by calling phy_power_off() at the end of bind > >> operation. > >> > >> However it turned out that having PHY powered is required for proper DRM > >> display pipeline initialization on Peach-Pit Chromebook2 board, as this > >> board freezes otherwise when PHY power off is called in bind. Fix this by > >> keeping PHY powered for the whole bind/unbind driver life cycle. The > >> freeze is probably related to the fact that the display pipeline (Exynos > >> FIMD CTRC -> Exynos/Analogix DP bridge -> PS8625 dp2lvds bridge -> > >> B116XW03 panel) is already configured and enabled by the bootloader and > >> require reset of all components for proper shutdown. > > > > "Probably" always makes me feel uncomfortable in such a context :-) > > Leaving the PHY powered at all times isn't the best idea as it will > > increase power consumption even when the DP output isn't in use. I can > > live with that if there's no other way, but it feels to me like we should > > first get to the bottom of the issue. > > Frankly I spent some time on this and I have no other idea. Board > freezes when phy_power_off is called and the initial code worked properly on > that board by leaking phy turned on all the time. I don't care too much personally as I don't maintain any platform that uses this chip, but if I did, I would mind the extra power consumption on my platform to work around an issue on yours :-) If this gets merged I think you should at least add a big FIXME comment that explains why the PHY isn't powered off at bind time. > >> Having PHY powered is also needed for proper EDID handling, which > >> initially fixed by commit 510353a63796 ("drm/bridge: analogix dp: Fix > >> runtime PM state in get_modes() callback"). > >> > >> Fixes: f0a8b49c03d2 ("drm/bridge: analogix dp: Fix runtime PM state on > >> driver bind") > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> --- > >> > >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index > >> b2756bc4225a..42595863cb03 100644 > >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > >> @@ -1415,7 +1415,6 @@ int 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 0; > >> @@ -1448,6 +1447,7 @@ void analogix_dp_unbind(struct device *dev, struct > >> device *master, > >> > >> drm_dp_aux_unregister(&dp->aux); > >> pm_runtime_disable(dev); > >> + phy_power_off(dp->phy); > >> clk_disable_unprepare(dp->clock); > >> } > >> EXPORT_SYMBOL_GPL(analogix_dp_unbind);
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index b2756bc4225a..42595863cb03 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1415,7 +1415,6 @@ int 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 0; @@ -1448,6 +1447,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master, drm_dp_aux_unregister(&dp->aux); pm_runtime_disable(dev); + phy_power_off(dp->phy); clk_disable_unprepare(dp->clock); } EXPORT_SYMBOL_GPL(analogix_dp_unbind);
Patch f0a8b49c03d2 ("drm/bridge: analogix dp: Fix runtime PM state on driver bind") fixed unbalanced call to phy_power_on() in analogix_dp_bind() function by calling phy_power_off() at the end of bind operation. However it turned out that having PHY powered is required for proper DRM display pipeline initialization on Peach-Pit Chromebook2 board, as this board freezes otherwise when PHY power off is called in bind. Fix this by keeping PHY powered for the whole bind/unbind driver life cycle. The freeze is probably related to the fact that the display pipeline (Exynos FIMD CTRC -> Exynos/Analogix DP bridge -> PS8625 dp2lvds bridge -> B116XW03 panel) is already configured and enabled by the bootloader and require reset of all components for proper shutdown. Having PHY powered is also needed for proper EDID handling, which initially fixed by commit 510353a63796 ("drm/bridge: analogix dp: Fix runtime PM state in get_modes() callback"). Fixes: f0a8b49c03d2 ("drm/bridge: analogix dp: Fix runtime PM state on driver bind") Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)