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

[Chris Wilson]

On Wed, Apr 15, 2015 at 11:25:00AM +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2015 at 09:17:03AM +0100, Chris Wilson wrote:
> > On Wed, Apr 15, 2015 at 09:17:02AM +0200, Daniel Vetter wrote:
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index c8a34476570a..23bfbc61a494 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -74,6 +74,33 @@ module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600);
> > >  module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600);
> > >  module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
> > >  
> > > +static void store_vblank(struct drm_device *dev, int crtc,
> > > +			 unsigned vblank_count_inc,
> > > +			 struct timeval *t_vblank)
> > > +{
> > > +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> > > +	u32 tslot;
> > > +
> > > +	assert_spin_locked(&dev->vblank_time_lock);
> > > +
> > > +	if (t_vblank) {
> > > +		tslot = vblank->count + vblank_count_inc;
> > > +		vblanktimestamp(dev, crtc, tslot) = *t_vblank;
> > > +	}
> > 
> > It is not obvious this updates the right tslot in all circumstances.
> > Care to explain?
> 
> Writers are synchronized with vblank_time_lock, so there shouldn't be any
> races. Mario also has a patch to clear the ts slot if we don't have
> anything to set it too (that one will conflict ofc).
> 
> Or what exactly do you mean?

I was staring at vblank->count and reading backwards from the smp_wmb().

Just something like:
if (t_vblank) {
	/* All writers hold the spinlock, but readers are serialized by
	 * the latching of vblank->count below.
	 */
	 u32 tslot = vblank->count + vblank_count_inc;
	 ...

would help me understand the relationship better.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre