On Thu, Feb 05, 2015 at 10:05:43AM +0900, Joonyoung Shim wrote:
Hi Daniel,
On 02/04/2015 11:16 PM, Daniel Vetter wrote:
On Wed, Feb 04, 2015 at 04:42:57PM +0900, Joonyoung Shim wrote:
Hi,
On 02/04/2015 04:14 AM, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
exynos_plane_dpms(DRM_MODE_DPMS_ON) calls the win_enable()'s callback from the underlying layer. However neither one of these layers implement win_enable() - FIMD, Mixer and VIDI. Thus the call to exynos_plane_dpms() is pointless.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index b2a4b84..ad675fb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -65,8 +65,6 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
if (exynos_crtc->ops->commit) exynos_crtc->ops->commit(exynos_crtc);
- exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
As i said, this needs to keep pair enabled flag of struct exynos_drm_plane.
The reason exynos needs that exynos_plane->enable is because it has its own per-plane dpms state. There's two problems with that:
- It's highyl non-standard, the drm kms way is to just disable the plane and not have some additional knob on top.
- The atomic helpers will not be able to handle this. They assume that there's only one dpms knob for the entire display pipeline, and per-plane enable/disable is handled by setting plane->state->crtc, which must be set iff plane->state->fb is set right now.
I recommend we rip this all out if we can adjust existing userspace to stop using the "mode" property on planes and crtcs.
And with that non-standard exynos plane mode thing gone we can indeed rip out exynos_plane_dpms and exynos_plane->enabled and just directly call manager->ops->win_enable/disble. And then rip out the win_enable since it's unused.
But this cleanup on current codes doesn't care now current operation normally. First let's cleanup non-standard exynos plane dpms state as you said.
Yeah my reply wasn't too clear, so let me clarify: I agree with you, Padovan's patch as-is can't be merged. First we need to get rid of the non-standard plane dpms stuff, then we can remove the ->win_enable hook and then can we remove this call here. -Daniel