[Intel-gfx] [PATCH] drm/vblank: Fixup and document timestamp update/read barriers

[Chris Wilson]

On Wed, Apr 15, 2015 at 07:34:43PM +0200, Daniel Vetter wrote:
> This was a bit too much cargo-culted, so lets make it solid:
> - vblank->count doesn't need to be an atomic, writes are always done
>   under the protection of dev->vblank_time_lock. Switch to an unsigned
>   long instead and update comments. Note that atomic_read is just a
>   normal read of a volatile variable, so no need to audit all the
>   read-side access specifically.
> 
> - The barriers for the vblank counter seqlock weren't complete: The
>   read-side was missing the first barrier between the counter read and
>   the timestamp read, it only had a barrier between the ts and the
>   counter read. We need both.
> 
> - Barriers weren't properly documented. Since barriers only work if
>   you have them on boths sides of the transaction it's prudent to
>   reference where the other side is. To avoid duplicating the
>   write-side comment 3 times extract a little store_vblank() helper.
>   In that helper also assert that we do indeed hold
>   dev->vblank_time_lock, since in some cases the lock is acquired a
>   few functions up in the callchain.
> 
> Spotted while reviewing a patch from Chris Wilson to add a fastpath to
> the vblank_wait ioctl.
> 
> v2: Add comment to better explain how store_vblank works, suggested by
> Chris.
> 
> v3: Peter noticed that as-is the 2nd smp_wmb is redundant with the
> implicit barrier in the spin_unlock. But that can only be proven by
> auditing all callers and my point in extracting this little helper was
> to localize all the locking into just one place. Hence I think that
> additional optimization is too risky.
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mario Kleiner <mario.kleiner.de at gmail.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Michel Dänzer <michel at daenzer.net>
> Cc: Peter Hurley <peter at hurleysoftware.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

Fwiw, there was no discernible difference in the time to query the
vblank counter (on an ivb i7-3720QM).

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre