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@xxxxxxxxxxxxxxxxxx> > Cc: Mario Kleiner <mario.kleiner.de@xxxxxxxxx> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Michel Dänzer <michel@xxxxxxxxxxx> > Cc: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> Fwiw, there was no discernible difference in the time to query the vblank counter (on an ivb i7-3720QM). Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
