Re: [PATCH] drm/vblank: Document and fix vblank count barrier semantics

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jul 23, 2019 at 03:13:37PM +0200, Daniel Vetter wrote:
> Noticed while reviewing code. I'm not sure whether this might or might
> not explain some of the missed vblank hilarity we've been seeing. I
> think those all go through the vblank completion event, which has
> unconditional barriers - it always takes the spinlock. Therefore no
> cc stable.
> 
> v2:
> - Barrriers are hard, put them in in the right order (Chris).
> - Improve the comments a bit.
> 
> v3:
> 
> Ville noticed that on 32bit we might be breaking up the load/stores,
> now that the vblank counter has been switched over to be 64 bit. Fix
> that up by switching to atomic64_t. This this happens so rarely in
> practice I figured no need to cc: stable ...
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Keith Packard <keithp@keithp.com>
> References: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]")
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_vblank.c | 45 ++++++++++++++++++++++++++++++++----
>  include/drm/drm_vblank.h     | 15 ++++++++++--
>  2 files changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 603ab105125d..03e37bceac9c 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -107,7 +107,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
>  
>  	write_seqlock(&vblank->seqlock);
>  	vblank->time = t_vblank;
> -	vblank->count += vblank_count_inc;
> +	atomic64_add(vblank_count_inc, &vblank->count);
>  	write_sequnlock(&vblank->seqlock);
>  }
>  
> @@ -273,7 +273,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  
>  	DRM_DEBUG_VBL("updating vblank count on crtc %u:"
>  		      " current=%llu, diff=%u, hw=%u hw_last=%u\n",
> -		      pipe, vblank->count, diff, cur_vblank, vblank->last);
> +		      pipe, atomic64_read(&vblank->count), diff,
> +		      cur_vblank, vblank->last);
>  
>  	if (diff == 0) {
>  		WARN_ON_ONCE(cur_vblank != vblank->last);
> @@ -295,11 +296,23 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +	u64 count;
>  
>  	if (WARN_ON(pipe >= dev->num_crtcs))
>  		return 0;
>  
> -	return vblank->count;
> +	count = atomic64_read(&vblank->count);
> +
> +	/*
> +	 * This read barrier corresponds to the implicit write barrier of the
> +	 * write seqlock in store_vblank(). Note that this is the only place
> +	 * where we need an explicit barrier, since all other access goes
> +	 * through drm_vblank_count_and_time(), which already has the required
> +	 * read barrier curtesy of the read seqlock.
> +	 */
> +	smp_rmb();
> +
> +	return count;
>  }
>  
>  /**
> @@ -764,6 +777,14 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
>   * vblank interrupt (since it only reports the software vblank counter), see
>   * drm_crtc_accurate_vblank_count() for such use-cases.
>   *
> + * Note that for a given vblank counter value drm_crtc_handle_vblank()
> + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
> + * provide a barrier: Any writes done before calling
> + * drm_crtc_handle_vblank() will be visible to callers of the later
> + * functions, iff the vblank count is the same or a later one.
> + *
> + * See also &drm_vblank_crtc.count.
> + *
>   * Returns:
>   * The software vblank counter.
>   */
> @@ -801,7 +822,7 @@ static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
>  
>  	do {
>  		seq = read_seqbegin(&vblank->seqlock);
> -		vblank_count = vblank->count;
> +		vblank_count = atomic64_read(&vblank->count);
>  		*vblanktime = vblank->time;
>  	} while (read_seqretry(&vblank->seqlock, seq));
>  
> @@ -818,6 +839,14 @@ static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
>   * vblank events since the system was booted, including lost events due to
>   * modesetting activity. Returns corresponding system timestamp of the time
>   * of the vblank interval that corresponds to the current vblank counter value.
> + *
> + * Note that for a given vblank counter value drm_crtc_handle_vblank()
> + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
> + * provide a barrier: Any writes done before calling
> + * drm_crtc_handle_vblank() will be visible to callers of the later
> + * functions, iff the vblank count is the same or a later one.
> + *
> + * See also &drm_vblank_crtc.count.
>   */
>  u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
>  				   ktime_t *vblanktime)
> @@ -1791,6 +1820,14 @@ EXPORT_SYMBOL(drm_handle_vblank);
>   *
>   * This is the native KMS version of drm_handle_vblank().
>   *
> + * Note that for a given vblank counter value drm_crtc_handle_vblank()
> + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
> + * provide a barrier: Any writes done before calling
> + * drm_crtc_handle_vblank() will be visible to callers of the later
> + * functions, iff the vblank count is the same or a later one.
> + *
> + * See also &drm_vblank_crtc.count.
> + *
>   * Returns:
>   * True if the event was successfully handled, false on failure.
>   */
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 9fe4ba8bc622..c16c44052b3d 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -109,9 +109,20 @@ struct drm_vblank_crtc {
>  	seqlock_t seqlock;
>  
>  	/**
> -	 * @count: Current software vblank counter.
> +	 * @count:
> +	 *
> +	 * Current software vblank counter.
> +	 *
> +	 * Note that for a given vblank counter value drm_crtc_handle_vblank()
> +	 * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
> +	 * provide a barrier: Any writes done before calling
> +	 * drm_crtc_handle_vblank() will be visible to callers of the later
> +	 * functions, iff the vblank count is the same or a later one.
> +	 *
> +	 * IMPORTANT: This guarantee requires barriers, therefor never access
> +	 * this field directly. Use drm_crtc_vblank_count() instead.
>  	 */
> -	u64 count;
> +	atomic64_t count;
>  	/**
>  	 * @time: Vblank timestamp corresponding to @count.
>  	 */
> -- 
> 2.22.0

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel