Re: [PATCH] drm/vblank: Document and fix vblank count barrier semantics

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Sep 03, 2019 at 11:17:12AM -0400, Rodrigo Siqueira wrote:
> Hi, thanks for the explanation.
> 
> I noticed that I forgot to add my r-b.
> 
> Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

Uh I just pushed, so can't add your r-b now :-/

Sry.
-Daniel

> 
> On 09/03, Daniel Vetter wrote:
> > On Tue, Sep 03, 2019 at 08:47:03AM -0400, Rodrigo Siqueira wrote:
> > > Hi Daniel,
> > > 
> > > All the series look really good for me. I just have some few questions
> > > here.
> > > 
> > > On 07/23, 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
> > > 
> > > I have to admit that I'm a little bit confused about the "missed vblank
> > > hilarity we've been seeing". Could you elaborate a little bit more about
> > > this problem in the commit message?
> > 
> > We've had various reports on various drivers that hw vblanks seem to get
> > lost and the driver stuck on vblank waits. I think most of those where
> > just driver bugs, but could be also that there's some issues in the vblank
> > core.
> > 
> > > Additionally, how about break this commit in two? One dedicated to the barriers
> > > and the atomic64, and the other related to the documentation?
> > > 
> > > > 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.
> > > > 
> > > > v3:
> > > > 
> > > > Ville noticed that on 32bit we might be breaking up the load/stores,
> > > > now that the vblank counter has been switched over to be 64 bit. Fix
> > > > that up by switching to atomic64_t. This this happens so rarely in
> > > > practice I figured no need to cc: stable ...
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Keith Packard <keithp@keithp.com>
> > > > References: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]")
> > > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_vblank.c | 45 ++++++++++++++++++++++++++++++++----
> > > >  include/drm/drm_vblank.h     | 15 ++++++++++--
> > > >  2 files changed, 54 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > index 603ab105125d..03e37bceac9c 100644
> > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > @@ -107,7 +107,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > > >  
> > > >  	write_seqlock(&vblank->seqlock);
> > > >  	vblank->time = t_vblank;
> > > > -	vblank->count += vblank_count_inc;
> > > > +	atomic64_add(vblank_count_inc, &vblank->count);
> > > >  	write_sequnlock(&vblank->seqlock);
> > > >  }
> > > >  
> > > > @@ -273,7 +273,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > >  
> > > >  	DRM_DEBUG_VBL("updating vblank count on crtc %u:"
> > > >  		      " current=%llu, diff=%u, hw=%u hw_last=%u\n",
> > > > -		      pipe, vblank->count, diff, cur_vblank, vblank->last);
> > > > +		      pipe, atomic64_read(&vblank->count), diff,
> > > > +		      cur_vblank, vblank->last);
> > > >  
> > > >  	if (diff == 0) {
> > > >  		WARN_ON_ONCE(cur_vblank != vblank->last);
> > > > @@ -295,11 +296,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 = atomic64_read(&vblank->count);
> > > > +
> > > > +	/*
> > > > +	 * 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();
> > > 
> > > I think I did not get all the idea behind the smp_rmb() in this function. FWIU,
> > > smp_xxx are used for preventing race conditions in a multiprocessor system,
> > > right? In this sense, I can presume that this change can bring benefits for
> > > VKMS or any other virtual driver; on the other hand, this will not bring any
> > > advantage on real drivers like i915 and amdgpu since these devices are not
> > > related with smp stuff, right?
> > 
> > smp or not smp is about the cpu your driver is running on, not anything to
> > do with the device hardware itself. And nowadays there's simply no
> > single-threaded processors anymore, everything has at least 2 cores (even
> > the tiniest soc). So yeah, this matters for everyone.
> > 
> > smp_* functions only get compiled out to nothing if you have CONFIG_UP
> > (which means only 1 cpu core with only 1 SMT thread is supported).
> > 
> > And yeah correctly placing smp barriers is Real Hard Stuff (tm).
> > -Daniel
> > 
> >  
> > > Thanks
> > > 
> > > > +
> > > > +	return count;
> > > >  }
> > > >  
> > > >  /**
> > > > @@ -764,6 +777,14 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> > > >   * vblank interrupt (since it only reports the software vblank counter), see
> > > >   * drm_crtc_accurate_vblank_count() for such use-cases.
> > > >   *
> > > > + * Note that for a given vblank counter value drm_crtc_handle_vblank()
> > > > + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
> > > > + * provide a barrier: Any writes done before calling
> > > > + * drm_crtc_handle_vblank() will be visible to callers of the later
> > > > + * functions, iff the vblank count is the same or a later one.
> > > > + *
> > > > + * See also &drm_vblank_crtc.count.
> > > > + *
> > > >   * Returns:
> > > >   * The software vblank counter.
> > > >   */
> > > > @@ -801,7 +822,7 @@ static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> > > >  
> > > >  	do {
> > > >  		seq = read_seqbegin(&vblank->seqlock);
> > > > -		vblank_count = vblank->count;
> > > > +		vblank_count = atomic64_read(&vblank->count);
> > > >  		*vblanktime = vblank->time;
> > > >  	} while (read_seqretry(&vblank->seqlock, seq));
> > > >  
> > > > @@ -818,6 +839,14 @@ static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> > > >   * vblank events since the system was booted, including lost events due to
> > > >   * modesetting activity. Returns corresponding system timestamp of the time
> > > >   * of the vblank interval that corresponds to the current vblank counter value.
> > > > + *
> > > > + * Note that for a given vblank counter value drm_crtc_handle_vblank()
> > > > + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
> > > > + * provide a barrier: Any writes done before calling
> > > > + * drm_crtc_handle_vblank() will be visible to callers of the later
> > > > + * functions, iff the vblank count is the same or a later one.
> > > > + *
> > > > + * See also &drm_vblank_crtc.count.
> > > >   */
> > > >  u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> > > >  				   ktime_t *vblanktime)
> > > > @@ -1791,6 +1820,14 @@ EXPORT_SYMBOL(drm_handle_vblank);
> > > >   *
> > > >   * This is the native KMS version of drm_handle_vblank().
> > > >   *
> > > > + * Note that for a given vblank counter value drm_crtc_handle_vblank()
> > > > + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
> > > > + * provide a barrier: Any writes done before calling
> > > > + * drm_crtc_handle_vblank() will be visible to callers of the later
> > > > + * functions, iff the vblank count is the same or a later one.
> > > > + *
> > > > + * See also &drm_vblank_crtc.count.
> > > > + *
> > > >   * Returns:
> > > >   * True if the event was successfully handled, false on failure.
> > > >   */
> > > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > > > index 9fe4ba8bc622..c16c44052b3d 100644
> > > > --- a/include/drm/drm_vblank.h
> > > > +++ b/include/drm/drm_vblank.h
> > > > @@ -109,9 +109,20 @@ struct drm_vblank_crtc {
> > > >  	seqlock_t seqlock;
> > > >  
> > > >  	/**
> > > > -	 * @count: Current software vblank counter.
> > > > +	 * @count:
> > > > +	 *
> > > > +	 * Current software vblank counter.
> > > > +	 *
> > > > +	 * Note that for a given vblank counter value drm_crtc_handle_vblank()
> > > > +	 * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
> > > > +	 * provide a barrier: Any writes done before calling
> > > > +	 * drm_crtc_handle_vblank() will be visible to callers of the later
> > > > +	 * functions, iff the vblank count is the same or a later one.
> > > > +	 *
> > > > +	 * IMPORTANT: This guarantee requires barriers, therefor never access
> > > > +	 * this field directly. Use drm_crtc_vblank_count() instead.
> > > >  	 */
> > > > -	u64 count;
> > > > +	atomic64_t count;
> > > >  	/**
> > > >  	 * @time: Vblank timestamp corresponding to @count.
> > > >  	 */
> > > > -- 
> > > > 2.22.0
> > > > 
> > > 
> > > -- 
> > > Rodrigo Siqueira
> > > Software Engineer, Advanced Micro Devices (AMD)
> > > https://siqueira.tech
> > 
> > 
> > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Rodrigo Siqueira
> Software Engineer, Advanced Micro Devices (AMD)
> https://siqueira.tech



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx