[Intel-gfx] [PATCH 1/3] drm/vblank: Document and fix vblank count barrier semantics

[Ville Syrjälä]

On Fri, Jul 19, 2019 at 05:23:12PM +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.
> 
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 38 +++++++++++++++++++++++++++++++++++-
>  include/drm/drm_vblank.h     | 13 +++++++++++-
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 603ab105125d..eb2a8304536c 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -295,11 +295,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 = vblank->count;

Hmm. This is now a 64bit quantity, which means on 32bit the load/store
won't be atomic. That doesn't seem particularly great.

> +
> +	/*
> +	 * 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;
>  }
>  
>  /**

-- 
Ville Syrjälä
Intel