Hi Daniel,
On 04/15/2015 03:17 AM, 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.
>
> 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>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_irq.c | 92 ++++++++++++++++++++++++-----------------------
> include/drm/drmP.h | 8 +++--
> 2 files changed, 54 insertions(+), 46 deletions(-)
>
> 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;
> + }
> +
> + /*
> + * vblank timestamp updates are protected on the write side with
> + * vblank_time_lock, but on the read side done locklessly using a
> + * sequence-lock on the vblank counter. Ensure correct ordering using
> + * memory barrriers. We need the barrier both before and also after the
> + * counter update to synchronize with the next timestamp write.
> + * The read-side barriers for this are in drm_vblank_count_and_time.
> + */
> + smp_wmb();
> + vblank->count += vblank_count_inc;
> + smp_wmb();
The comment and the code are each self-contradictory.
If vblank->count writes are always protected by vblank_time_lock (something I
did not verify but that the comment above asserts), then the trailing write
barrier is not required (and the assertion that it is in the comment is incorrect).
A spin unlock operation is always a write barrier.
Regards,
Peter Hurley
> +}
> +
> /**
> * drm_update_vblank_count - update the master vblank counter
> * @dev: DRM device
> @@ -93,7 +120,7 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
> static void drm_update_vblank_count(struct drm_device *dev, int crtc)
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> - u32 cur_vblank, diff, tslot;
> + u32 cur_vblank, diff;
> bool rc;
> struct timeval t_vblank;
>
> @@ -129,18 +156,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
> if (diff == 0)
> return;
>
> - /* Reinitialize corresponding vblank timestamp if high-precision query
> - * available. Skip this step if query unsupported or failed. Will
> - * reinitialize delayed at next vblank interrupt in that case.
> + /*
> + * Only reinitialize corresponding vblank timestamp if high-precision query
> + * available and didn't fail. Will reinitialize delayed at next vblank
> + * interrupt in that case.
> */
> - if (rc) {
> - tslot = atomic_read(&vblank->count) + diff;
> - vblanktimestamp(dev, crtc, tslot) = t_vblank;
> - }
> -
> - smp_mb__before_atomic();
> - atomic_add(diff, &vblank->count);
> - smp_mb__after_atomic();
> + store_vblank(dev, crtc, diff, rc ? &t_vblank : NULL);
> }
>
> /*
> @@ -218,7 +239,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
> /* Compute time difference to stored timestamp of last vblank
> * as updated by last invocation of drm_handle_vblank() in vblank irq.
> */
> - vblcount = atomic_read(&vblank->count);
> + vblcount = vblank->count;
> diff_ns = timeval_to_ns(&tvblank) -
> timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount));
>
> @@ -234,17 +255,8 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
> * available. In that case we can't account for this and just
> * hope for the best.
> */
> - if (vblrc && (abs64(diff_ns) > 1000000)) {
> - /* Store new timestamp in ringbuffer. */
> - vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;
> -
> - /* Increment cooked vblank count. This also atomically commits
> - * the timestamp computed above.
> - */
> - smp_mb__before_atomic();
> - atomic_inc(&vblank->count);
> - smp_mb__after_atomic();
> - }
> + if (vblrc && (abs64(diff_ns) > 1000000))
> + store_vblank(dev, crtc, 1, &tvblank);
>
> spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
> }
> @@ -852,7 +864,7 @@ u32 drm_vblank_count(struct drm_device *dev, int crtc)
>
> if (WARN_ON(crtc >= dev->num_crtcs))
> return 0;
> - return atomic_read(&vblank->count);
> + return vblank->count;
> }
> EXPORT_SYMBOL(drm_vblank_count);
>
> @@ -897,16 +909,17 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
> if (WARN_ON(crtc >= dev->num_crtcs))
> return 0;
>
> - /* Read timestamp from slot of _vblank_time ringbuffer
> - * that corresponds to current vblank count. Retry if
> - * count has incremented during readout. This works like
> - * a seqlock.
> + /*
> + * Vblank timestamps are read lockless. To ensure consistency the vblank
> + * counter is rechecked and ordering is ensured using memory barriers.
> + * This works like a seqlock. The write-side barriers are in store_vblank.
> */
> do {
> - cur_vblank = atomic_read(&vblank->count);
> + cur_vblank = vblank->count;
> + smp_rmb();
> *vblanktime = vblanktimestamp(dev, crtc, cur_vblank);
> smp_rmb();
> - } while (cur_vblank != atomic_read(&vblank->count));
> + } while (cur_vblank != vblank->count);
>
> return cur_vblank;
> }
> @@ -1715,7 +1728,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
> */
>
> /* Get current timestamp and count. */
> - vblcount = atomic_read(&vblank->count);
> + vblcount = vblank->count;
> drm_get_last_vbltimestamp(dev, crtc, &tvblank, DRM_CALLED_FROM_VBLIRQ);
>
> /* Compute time difference to timestamp of last vblank */
> @@ -1731,20 +1744,11 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
> * e.g., due to spurious vblank interrupts. We need to
> * ignore those for accounting.
> */
> - if (abs64(diff_ns) > DRM_REDUNDANT_VBLIRQ_THRESH_NS) {
> - /* Store new timestamp in ringbuffer. */
> - vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;
> -
> - /* Increment cooked vblank count. This also atomically commits
> - * the timestamp computed above.
> - */
> - smp_mb__before_atomic();
> - atomic_inc(&vblank->count);
> - smp_mb__after_atomic();
> - } else {
> + if (abs64(diff_ns) > DRM_REDUNDANT_VBLIRQ_THRESH_NS)
> + store_vblank(dev, crtc, 1, &tvblank);
> + else
> DRM_DEBUG("crtc %d: Redundant vblirq ignored. diff_ns = %d\n",
> crtc, (int) diff_ns);
> - }
>
> spin_unlock(&dev->vblank_time_lock);
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 62c40777c009..4c31a2cc5a33 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -686,9 +686,13 @@ struct drm_pending_vblank_event {
> struct drm_vblank_crtc {
> struct drm_device *dev; /* pointer to the drm_device */
> wait_queue_head_t queue; /**< VBLANK wait queue */
> - struct timeval time[DRM_VBLANKTIME_RBSIZE]; /**< timestamp of current count */
> struct timer_list disable_timer; /* delayed disable timer */
> - atomic_t count; /**< number of VBLANK interrupts */
> +
> + /* vblank counter, protected by dev->vblank_time_lock for writes */
> + unsigned long count;
> + /* vblank timestamps, protected by dev->vblank_time_lock for writes */
> + struct timeval time[DRM_VBLANKTIME_RBSIZE];
> +
> atomic_t refcount; /* number of users of vblank interruptsper crtc */
> u32 last; /* protected by dev->vbl_lock, used */
> /* for wraparound handling */
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
