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

["Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@public.gmane.org>]

On 19/01/17 07:18 AM, Grodzovsky, Andrey wrote:
>> From: Michel Dänzer [mailto:michel@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@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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau