Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> If userspace wants a page flip to take effect during vblank sequence n,
> it has to wait for vblank seqno n-1 before calling the
> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
> 
> This change makes sure that we do not program the flip to the hardware
> before the end of vblank seqno n-1 in this case, to prevent the flip
> from taking effect too early.
> 
> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
> during vblank, but userspace didn't wait for the current vblank seqno
> before, this change would still allow the flip to be programmed during
> the current vblank seqno.

This just sounds like you're sending vblank events out a bit too early.
And watching vblank waits that userspace does works, but it's fragile,
add-hoc and I don't really jump in joy about adding that to the vblank
core. Is there no way you can adjust sending out the vblank events
similarly, to make sure userspace can never sneak in a pageflip too early?

That way it would be all nicely contained to amdgpu, and not leaking into
the core.
-Daniel

> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h         |  3 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 34 +++++++++++++++++++++++++----
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 992f00b..7b53967 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -712,10 +712,11 @@ void amdgpu_doorbell_get_kfd_info(struct amdgpu_device *adev,
>   */
>  
>  struct amdgpu_flip_work {
> -	struct work_struct		flip_work;
> +	struct delayed_work		flip_work;
>  	struct work_struct		unpin_work;
>  	struct amdgpu_device		*adev;
>  	int				crtc_id;
> +	u32				avoid_vblank;
>  	uint64_t			base;
>  	struct drm_pending_vblank_event *event;
>  	struct amdgpu_bo		*old_rbo;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index de95ea7..a9f7851 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -41,7 +41,7 @@ static void amdgpu_flip_callback(struct fence *f, struct fence_cb *cb)
>  		container_of(cb, struct amdgpu_flip_work, cb);
>  
>  	fence_put(f);
> -	schedule_work(&work->flip_work);
> +	schedule_work(&work->flip_work.work);
>  }
>  
>  static bool amdgpu_flip_handle_fence(struct amdgpu_flip_work *work,
> @@ -63,8 +63,10 @@ static bool amdgpu_flip_handle_fence(struct amdgpu_flip_work *work,
>  
>  static void amdgpu_flip_work_func(struct work_struct *__work)
>  {
> +	struct delayed_work *delayed_work =
> +		container_of(__work, struct delayed_work, work);
>  	struct amdgpu_flip_work *work =
> -		container_of(__work, struct amdgpu_flip_work, flip_work);
> +		container_of(delayed_work, struct amdgpu_flip_work, flip_work);
>  	struct amdgpu_device *adev = work->adev;
>  	struct amdgpu_crtc *amdgpuCrtc = adev->mode_info.crtcs[work->crtc_id];
>  
> @@ -81,6 +83,19 @@ static void amdgpu_flip_work_func(struct work_struct *__work)
>  		if (amdgpu_flip_handle_fence(work, &work->shared[i]))
>  			return;
>  
> +	/* Wait until we're out of the last waited-for vertical blank
> +	 * period
> +	 */
> +	if (amdgpuCrtc->enabled &&
> +	    drm_crtc_vblank_count(crtc) == work->avoid_vblank &&
> +	    (amdgpu_get_crtc_scanoutpos(adev->ddev, work->crtc_id, 0,
> +					&vpos, &hpos, NULL, NULL,
> +					&crtc->hwmode)
> +	     & DRM_SCANOUTPOS_IN_VBLANK)) {
> +		schedule_delayed_work(&work->flip_work, usecs_to_jiffies(1000));
> +		return;
> +	}
> +
>  	/* We borrow the event spin lock for protecting flip_status */
>  	spin_lock_irqsave(&crtc->dev->event_lock, flags);
>  
> @@ -175,6 +190,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
>  			  uint32_t page_flip_flags)
>  {
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_file *file_priv = event->base.file_priv;
>  	struct amdgpu_device *adev = dev->dev_private;
>  	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>  	struct amdgpu_framebuffer *old_amdgpu_fb;
> @@ -191,7 +207,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> -	INIT_WORK(&work->flip_work, amdgpu_flip_work_func);
> +	INIT_DELAYED_WORK(&work->flip_work, amdgpu_flip_work_func);
>  	INIT_WORK(&work->unpin_work, amdgpu_unpin_work_func);
>  
>  	work->event = event;
> @@ -244,6 +260,16 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
>  		goto pflip_cleanup;
>  	}
>  
> +	/* If this file descriptor has waited for the current vblank period,
> +	 * do not program the flip during this vblank period
> +	 */
> +	if (amdgpu_crtc->crtc_id < file_priv->num_crtcs)
> +		work->avoid_vblank =
> +			file_priv->last_vblank_wait[amdgpu_crtc->crtc_id];
> +
> +	if (!work->avoid_vblank)
> +		work->avoid_vblank = drm_crtc_vblank_count(crtc) - 1;
> +
>  	/* we borrow the event spin lock for protecting flip_wrok */
>  	spin_lock_irqsave(&crtc->dev->event_lock, flags);
>  	if (amdgpu_crtc->pflip_status != AMDGPU_FLIP_NONE) {
> @@ -262,7 +288,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
>  	/* update crtc fb */
>  	crtc->primary->fb = fb;
>  	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> -	amdgpu_flip_work_func(&work->flip_work);
> +	amdgpu_flip_work_func(&work->flip_work.work);
>  	return 0;
>  
>  vblank_cleanup:
> -- 
> 2.8.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel