Re: [PATCH 02/14] drm/exynos: Remove exynos_plane_dpms() call with no effect
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 > >>> > >>> 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 > >>> --- > >>> 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] drm/exynos: Remove exynos_plane_dpms() call with no effect
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 >>> >>> 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 >>> --- >>> 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. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] drm/exynos: Remove exynos_plane_dpms() call with no effect
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 > > > > 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 > > --- > > 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] drm/exynos: Remove exynos_plane_dpms() call with no effect
Hi, On 02/04/2015 04:14 AM, Gustavo Padovan wrote: > From: Gustavo Padovan > > 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 > --- > 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. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
