Re: [PATCH] drm/bridge: analogix_dp: Keep PHY powered from between driver bind/unbind

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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);

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel