[PATCH 3/4] drm/amd/display: Switch to using atomic_helper for flip.

[Grodzovsky, Andrey]

> -----Original Message-----
> From: Michel Dänzer [mailto:michel at daenzer.net]
> Sent: Tuesday, January 17, 2017 8:50 PM
> To: Laurent Pinchart
> Cc: dri-devel at lists.freedesktop.org; Grodzovsky, Andrey;
> daniel.vetter at intel.com; amd-gfx at lists.freedesktop.org;
> nouveau at lists.freedesktop.org
> Subject: Re: [PATCH 3/4] drm/amd/display: Switch to using atomic_helper for
> flip.
> 
> On 17/01/17 07:16 AM, Laurent Pinchart wrote:
> > On Monday 16 Jan 2017 10:44:57 Andrey Grodzovsky wrote:
> >> Change-Id: Iad3e0b9b3546e4e4dc79be9233daf4fe4dba83e0
> >> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
> >> ---
> >>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92 ++---------
> -------
> >>  1 file changed, 6 insertions(+), 86 deletions(-)
> >>
> >> diff --git
> a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> index
> >> a443b70..d4664bf 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> >> @@ -1060,83 +1060,6 @@ static int dm_crtc_funcs_atomic_set_property(
> >>  	return 0;
> >>  }
> >>
> >> -
> >> -static int amdgpu_atomic_helper_page_flip(struct drm_crtc *crtc,
> >> -				struct drm_framebuffer *fb,
> >> -				struct drm_pending_vblank_event *event,
> >> -				uint32_t flags)
> >> -{
> >> -	struct drm_plane *plane = crtc->primary;
> >> -	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> >> -	struct drm_atomic_state *state;
> >> -	struct drm_plane_state *plane_state;
> >> -	struct drm_crtc_state *crtc_state;
> >> -	int ret = 0;
> >> -
> >> -	state = drm_atomic_state_alloc(plane->dev);
> >> -	if (!state)
> >> -		return -ENOMEM;
> >> -
> >> -	ret = drm_crtc_vblank_get(crtc);
> >
> > The DRM core's atomic page flip helper doesn't get/put vblank. Have
> > you double-checked that removing them isn't a problem ?
> 
> This patch makes the amdgpu DM code use the page_flip_target hook.
> drm_mode_page_flip_ioctl calls drm_crtc_vblank_get before the
> page_flip_target hook.
> 
> You're right though that the fact that drm_atomic_helper_page_flip doesn't
> call drm_crtc_vblank_get is a bit alarming. From the
> DRM_IOCTL_MODE_PAGE_FLIP POV, drm_crtc_vblank_get must be called
> when userspace calls the ioctl (either by drm_mode_page_flip_ioctl or the
> page_flip hook implementation), and drm_crtc_vblank_put must be called
> when the flip completes and the event is sent to userspace. How is this
> supposed to be handled with atomic?

First let me ask you - why we have to enable vlbank as part of pflip, 
I understand why it has to be enbbled in WAIT_FOR_VBLANK IOCTL 
but not sure about PFLIP IOCTL. 

IMHO in atomic as before in legacy page_flip, it's up to the  driver 
implementation in atomic_commit hook   to manage vblank ISR on,off.
> 
> Andrey, did you check via code audit and/or testing that the vblank
> reference count is still balanced after this change?
> 
With the page_flip_target yes, if I switch back to page_flip hook then 
vblank ISR never gets enabled on FLIP IPCTL and then I see warning in DAL's
pflip done IRQ handler from vblank_put which is obvious, which BTW
didn't impact the flips, I still was able to run glxgears in vsync mode.

> 
> >> @@ -3143,8 +3064,7 @@ int amdgpu_dm_atomic_check(struct
> drm_device *dev,
> >>  			 * 1. This commit is not a page flip.
> >>  			 * 2. This commit is a page flip, and targets are
> > created.
> >>  			 */
> >> -			if (!page_flip_needed(plane_state, old_plane_state,
> >> -					      true) ||
> >> +			if (!page_flip_needed(plane_state, old_plane_state,
> > true) ||
> >
> > This seems to be an unrelated change.
> 
> Yeah, such whitespace-only changes should be dropped.

Will remove this.
> 
> 
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer