On Mon, Sep 14, 2015 at 10:43:52PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> The vblank timestamp ringbuffer only has two entries, so if the
> vblank->count is incremented by an even number readers may end up seeing
> the new vblank timestamp alongside the old vblank counter value.
>
> Fix the problem by storing the vblank counter in a ringbuffer as well,
> and always increment the ringbuffer "slot" by one when storing a new
> timestamp+counter pair.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Imo if we bother with this we might as well just switch over to using
full-blown seqlocks. They internally use a two-stage update which means
race-free even with just one copy of the data we protect. Also more
standardized to boot.
Series looks good otherwise, I'll wait for Maarten to r-b it and then pull
it in.
-Daniel
> ---
> drivers/gpu/drm/drm_irq.c | 40 ++++++++++++++++++++++++----------------
> include/drm/drmP.h | 12 ++++++------
> 2 files changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 88fbee4..8de236a 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -43,8 +43,10 @@
> #include <linux/export.h>
>
> /* Access macro for slots in vblank timestamp ringbuffer. */
> -#define vblanktimestamp(dev, pipe, count) \
> - ((dev)->vblank[pipe].time[(count) % DRM_VBLANKTIME_RBSIZE])
> +#define vblanktimestamp(dev, pipe, slot) \
> + ((dev)->vblank[pipe].time[(slot) % DRM_VBLANK_RBSIZE])
> +#define vblankcount(dev, pipe, slot) \
> + ((dev)->vblank[pipe].count[(slot) % DRM_VBLANK_RBSIZE])
>
> /* Retry timestamp calculation up to 3 times to satisfy
> * drm_timestamp_precision before giving up.
> @@ -76,20 +78,23 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
>
> static void store_vblank(struct drm_device *dev, unsigned int pipe,
> u32 vblank_count_inc,
> - struct timeval *t_vblank, u32 last)
> + const struct timeval *t_vblank, u32 last)
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> - u32 tslot;
> + u32 slot;
>
> assert_spin_locked(&dev->vblank_time_lock);
>
> + slot = vblank->slot + 1;
> +
> vblank->last = last;
>
> /* All writers hold the spinlock, but readers are serialized by
> - * the latching of vblank->count below.
> + * the latching of vblank->slot below.
> */
> - tslot = vblank->count + vblank_count_inc;
> - vblanktimestamp(dev, pipe, tslot) = *t_vblank;
> + vblankcount(dev, pipe, slot) =
> + vblankcount(dev, pipe, vblank->slot) + vblank_count_inc;
> + vblanktimestamp(dev, pipe, slot) = *t_vblank;
>
> /*
> * vblank timestamp updates are protected on the write side with
> @@ -100,7 +105,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> * The read-side barriers for this are in drm_vblank_count_and_time.
> */
> smp_wmb();
> - vblank->count += vblank_count_inc;
> + vblank->slot = slot;
> smp_wmb();
> }
>
> @@ -202,7 +207,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> const struct timeval *t_old;
> u64 diff_ns;
>
> - t_old = &vblanktimestamp(dev, pipe, vblank->count);
> + t_old = &vblanktimestamp(dev, pipe, vblank->slot);
> diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
>
> /*
> @@ -223,7 +228,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>
> DRM_DEBUG("updating vblank count on crtc %u:"
> " current=%u, diff=%u, hw=%u hw_last=%u\n",
> - pipe, vblank->count, diff, cur_vblank, vblank->last);
> + pipe, vblankcount(dev, pipe, vblank->slot),
> + diff, cur_vblank, vblank->last);
>
> if (diff == 0) {
> WARN_ON_ONCE(cur_vblank != vblank->last);
> @@ -883,7 +889,7 @@ u32 drm_vblank_count(struct drm_device *dev, int pipe)
> if (WARN_ON(pipe >= dev->num_crtcs))
> return 0;
>
> - return vblank->count;
> + return vblankcount(dev, pipe, vblank->slot);
> }
> EXPORT_SYMBOL(drm_vblank_count);
>
> @@ -923,7 +929,8 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> int count = DRM_TIMESTAMP_MAXRETRIES;
> - u32 cur_vblank;
> + u32 vblankcount;
> + u32 slot;
>
> if (WARN_ON(pipe >= dev->num_crtcs))
> return 0;
> @@ -934,13 +941,14 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> * This works like a seqlock. The write-side barriers are in store_vblank.
> */
> do {
> - cur_vblank = vblank->count;
> + slot = vblank->slot;
> smp_rmb();
> - *vblanktime = vblanktimestamp(dev, pipe, cur_vblank);
> + *vblanktime = vblanktimestamp(dev, pipe, slot);
> + vblankcount = vblankcount(dev, pipe, slot);
> smp_rmb();
> - } while (cur_vblank != vblank->count && --count > 0);
> + } while (slot != vblank->slot && --count > 0);
>
> - return cur_vblank;
> + return vblankcount;
> }
> EXPORT_SYMBOL(drm_vblank_count_and_time);
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 6717a7d..9de9fde 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -374,10 +374,10 @@ struct drm_master {
> void *driver_priv;
> };
>
> -/* Size of ringbuffer for vblank timestamps. Just double-buffer
> +/* Size of ringbuffer for vblank counts/timestamps. Just double-buffer
> * in initial implementation.
> */
> -#define DRM_VBLANKTIME_RBSIZE 2
> +#define DRM_VBLANK_RBSIZE 2
>
> /* Flags and return codes for get_vblank_timestamp() driver function. */
> #define DRM_CALLED_FROM_VBLIRQ 1
> @@ -692,10 +692,10 @@ struct drm_vblank_crtc {
> wait_queue_head_t queue; /**< VBLANK wait queue */
> struct timer_list disable_timer; /* delayed disable timer */
>
> - /* vblank counter, protected by dev->vblank_time_lock for writes */
> - u32 count;
> - /* vblank timestamps, protected by dev->vblank_time_lock for writes */
> - struct timeval time[DRM_VBLANKTIME_RBSIZE];
> + u32 slot;
> + /* vblank timestamps and counter, protected by dev->vblank_time_lock for writes */
> + struct timeval time[DRM_VBLANK_RBSIZE];
> + u32 count[DRM_VBLANK_RBSIZE];
>
> atomic_t refcount; /* number of users of vblank interruptsper crtc */
> u32 last; /* protected by dev->vbl_lock, used */
> --
> 2.4.6
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
