Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Peter Hurley" <peter@hurleysoftware.com>,
	"Michel Dänzer" <michel@daenzer.net>,
	"DRI Development" <dri-devel@lists.freedesktop.org>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Intel Graphics Development" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
Date: Wed, 15 Apr 2015 18:49:16 +0100	[thread overview]
Message-ID: <20150415174916.GB3981@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1429119283-11215-1-git-send-email-daniel.vetter@ffwll.ch>

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@chris-wilson.co.uk>
> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Peter Hurley <peter@hurleysoftware.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel