On 19/01/17 07:18 AM, Grodzovsky, Andrey wrote: >> From: Michel Dänzer [mailto:michel at daenzer.net] >> 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. It's required for the vblank sequence counter to be accurate in the page flip completion event sent to userspace. > 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. When using the page_flip hook, it's all up to the implementation of that hook. When using the page_flip_target hook, drm_mode_page_flip_ioctl calls vblank_get, so the hook implementation must make sure that a matching vblank_put call is made when the flip completes. >> 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, For drivers using the atomic_helpers for flip, maybe there should be (or might be already) a corresponding helper which deals with things like this when the flip completes, so drivers don't have to call vblank_put and such. Anyway, it sounds like the vblank_get/puts are balanced with this patch for now. > which BTW didn't impact the flips, I still was able to run glxgears > in vsync mode. I don't think glxgears relies on the vblank sequence numbers being accurate. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
