[PATCH 2/2] drm/amd/display: Prevent premature pageflip when comitting in vblank.

[Michel Dänzer]

On 22/04/17 01:23 AM, Mario Kleiner wrote:
> Make sure we do not program a hw pageflip inside vblank 'n' iff the
> atomic flip is comitted while inside the same vblank 'n'. We must
> defer such a flip by one refresh cycle to vblank 'n+1'.
> 
> Without this, pageflips programmed via X11 GLX_OML_sync_control extensions
> glXSwapBuffersMscOML(..., target_msc, ...); call and/or via DRI3/Present
> PresentPixmap(..., target_msc, ...); request will complete one vblank
> too early whenever target_msc > current_msc + 1, ie. more than 1 vblank
> in the future. In such a case, the call of the pageflip ioctl() would be
> triggered by a queued drmWaitVblank() vblank event, which itself gets
> dispatched inside the vblank one frame before the target_msc vblank.
> 
> Testing with this patch does no longer show any problems with
> OML_sync_control swap scheduling or flip completion timestamps.
> Tested on R9 380 Tonga.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> Cc: Harry Wentland <Harry.Wentland at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Michel Dänzer <michel.daenzer at amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 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 086a842..19be2d9 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
> @@ -2460,6 +2460,9 @@ static void amdgpu_dm_do_flip(
>  	struct amdgpu_device *adev = crtc->dev->dev_private;
>  	bool async_flip = (acrtc->flip_flags & DRM_MODE_PAGE_FLIP_ASYNC) != 0;
>  
> +	/* Prepare wait for target vblank early - before the fence-waits */
> +	target_vblank = target - drm_crtc_vblank_count(crtc) +
> +			amdgpu_get_vblank_counter_kms(crtc->dev, acrtc->crtc_id);
>  
>  	/*TODO This might fail and hence better not used, wait
>  	 * explicitly on fences instead
> @@ -2478,13 +2481,9 @@ static void amdgpu_dm_do_flip(
>  
>  	amdgpu_bo_unreserve(abo);
>  
> -	/* Wait for target vblank */
>  	/* Wait until we're out of the vertical blank period before the one
>  	 * targeted by the flip
>  	 */
> -	target_vblank = target - drm_crtc_vblank_count(crtc) +
> -			amdgpu_get_vblank_counter_kms(crtc->dev, acrtc->crtc_id);
> -
>  	while ((acrtc->enabled &&
>  		(amdgpu_get_crtc_scanoutpos(adev->ddev, acrtc->crtc_id, 0,
>  					&vpos, &hpos, NULL, NULL,

Makes sense.


> @@ -2763,7 +2762,7 @@ void amdgpu_dm_atomic_commit_tail(
>  			amdgpu_dm_do_flip(
>  					crtc,
>  					fb,
> -					drm_crtc_vblank_count(crtc));
> +					drm_crtc_vblank_count(crtc) + 1);
>  
>  			wait_for_vblank =
>  					acrtc->flip_flags & DRM_MODE_PAGE_FLIP_ASYNC ?
> 

I suspect this code runs relatively late, so if userspace calls
DRM_IOCTL_MODE_PAGE_FLIP shortly before the start of vertical blank, it
could result in the flip being unnecessarily delayed by one frame. I
added drm_crtc_funcs::page_flip_target to address this.

Anyway, this is okay for now.

Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>


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