[Intel-gfx] [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()

[Daniel Vetter]

On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Add drm_crtc_vblank_get_accurate() which is the same as
> drm_crtc_vblank_get() except that it will forcefully update the
> the current vblank counter/timestamp value even if the interrupt
> is already enabled.
> 
> And we'll switch drm_wait_one_vblank() over to using it to make sure it
> will really wait at least one vblank even if it races with the irq
> handler.
> 
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Hm, I just thought of doing an
s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
drm_crtc_arm_vblank_event.

What does this buy us above&beyond the other patch? And why isn't
vblank_get accurate always?

/me confused

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_vblank.c | 37 ++++++++++++++++++++++++++++++++-----
>  include/drm/drm_vblank.h     |  1 +
>  2 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 823c853de052..c57383b8753b 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -955,7 +955,8 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
>  	return ret;
>  }
>  
> -static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
> +static int drm_vblank_get(struct drm_device *dev, unsigned int pipe,
> +			  bool force_update)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	unsigned long irqflags;
> @@ -975,6 +976,10 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
>  		if (!vblank->enabled) {
>  			atomic_dec(&vblank->refcount);
>  			ret = -EINVAL;
> +		} else if (force_update) {
> +			spin_lock(&dev->vblank_time_lock);
> +			drm_update_vblank_count(dev, pipe, false);
> +			spin_unlock(&dev->vblank_time_lock);
>  		}
>  	}
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> @@ -994,10 +999,32 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
>   */
>  int drm_crtc_vblank_get(struct drm_crtc *crtc)
>  {
> -	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc));
> +	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), false);
>  }
>  EXPORT_SYMBOL(drm_crtc_vblank_get);
>  
> +/**
> + * drm_crtc_vblank_get_accurate - get a reference count on vblank events and
> + *                                make sure the counter is uptodate
> + * @crtc: which CRTC to own
> + *
> + * Acquire a reference count on vblank events to avoid having them disabled
> + * while in use.
> + *
> + * Also make sure the current vblank counter is value is fully up to date
> + * even if we're already past the start of vblank but the irq hasn't fired
> + * yet, which may be the case with some hardware that raises the interrupt
> + * only some time after the start of vblank.
> + *
> + * Returns:
> + * Zero on success or a negative error code on failure.
> + */
> +int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc)
> +{
> +	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), true);
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_get_accurate);
> +
>  static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> @@ -1053,7 +1080,7 @@ void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
>  	if (WARN_ON(pipe >= dev->num_crtcs))
>  		return;
>  
> -	ret = drm_vblank_get(dev, pipe);
> +	ret = drm_vblank_get(dev, pipe, true);
>  	if (WARN(ret, "vblank not available on crtc %i, ret=%i\n", pipe, ret))
>  		return;
>  
> @@ -1248,7 +1275,7 @@ static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
>  	 */
>  	if (!vblank->inmodeset) {
>  		vblank->inmodeset = 0x1;
> -		if (drm_vblank_get(dev, pipe) == 0)
> +		if (drm_vblank_get(dev, pipe, false) == 0)
>  			vblank->inmodeset |= 0x2;
>  	}
>  }
> @@ -1448,7 +1475,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  		return 0;
>  	}
>  
> -	ret = drm_vblank_get(dev, pipe);
> +	ret = drm_vblank_get(dev, pipe, false);
>  	if (ret) {
>  		DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
>  		return ret;
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 7fba9efe4951..5629e7841318 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -162,6 +162,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>  bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
>  int drm_crtc_vblank_get(struct drm_crtc *crtc);
> +int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc);
>  void drm_crtc_vblank_put(struct drm_crtc *crtc);
>  void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
>  void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
> -- 
> 2.13.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch