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