Re: [PATCH 02/14] drm/exynos: Remove exynos_plane_dpms() call with no effect

[Joonyoung Shim <jy0922.shim@samsung.com>]

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.

Thanks.