On 04/16/2015 03:29 AM, Peter Hurley wrote: > On 04/15/2015 05:26 PM, Mario Kleiner wrote: >> A couple of questions to educate me and one review comment. >> >> On 04/15/2015 07:34 PM, 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 at chris-wilson.co.uk> >>> Cc: Mario Kleiner <mario.kleiner.de at gmail.com> >>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com> >>> Cc: Michel Dänzer <michel at daenzer.net> >>> Cc: Peter Hurley <peter at hurleysoftware.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com> >>> --- >>> drivers/gpu/drm/drm_irq.c | 95 >>> +++++++++++++++++++++++++---------------------- >>> include/drm/drmP.h | 8 +++- >>> 2 files changed, 57 insertions(+), 46 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c >>> index c8a34476570a..8694b77d0002 100644 >>> --- a/drivers/gpu/drm/drm_irq.c >>> +++ b/drivers/gpu/drm/drm_irq.c >>> @@ -74,6 +74,36 @@ 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) { >>> + /* All writers hold the spinlock, but readers are serialized by >>> + * the latching of vblank->count below. >>> + */ >>> + 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(); >>> +} >>> + >>> /** >>> * drm_update_vblank_count - update the master vblank counter >>> * @dev: DRM device >>> @@ -93,7 +123,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 +159,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 +242,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 +258,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 +867,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; >> >> I wrongly assumed atomic_read would guarantee more than it actually does, so >> please help me to learn something. Why don't we need some smp_rmb() here >> before returning vblank->count? What guarantees that drm_vblank_count() does >> return the latest value assigned to vblank->count in store_vblank()? In >> store_vblank() there is a smp_wmb(), but why don't we need a matching >> smp_rmb() here to benefit from it? > > Well, you could but the vblank counter resolution is very low (60HZ), > so practically speaking, what would be the point? >
Thanks for the explanations Peter. Depends on the latency induced. A few microseconds don't matter, a millisecond or more would matter for some applications. > The trailing write barrier in store_vblank() is only necessary to > force the ordering of the timestamp wrt to vblank count *in case there > was no write barrier executed between to calls to store_vblank()*, > not to ensure the cpu forces the write to main memory immediately. > That part i understand, the possibly slightly earlier write of some store buffers to main memory is just a possible nice side effect. Bits and pieces of my memory about how cache coherency etc. work slowly come back to my own memory... > Because the time scales for these events don't require that level of > resolution; consider how much code has to get executed between a > hardware vblank irq triggering and the vblank counter being updated. > > Realistically, the only relevant requirement is that the timestamp > match the counter. > Yes that is the really important part. A msec delay would possibly matter for some timing sensitive apps like mine - some more exotic displays run at 200 Hz, and some apps need to synchronize to the vblank not strictly for graphics. But i assume potential delays here are more on the order of a few microseconds if some pending loads from the cache would get reordered for overall efficiency? >> >>> } >>> EXPORT_SYMBOL(drm_vblank_count); >>> >>> @@ -897,16 +912,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); >>> >> >> Similar question as above. We have a new smp_rmb() after the cur_vblank >> assignment and then after *vblanktime assignment. My original wrong >> assumption was that the first smp_rmb() wouldn't be needed because >> atomic_read() would imply that, and that the compiler/cpu couldn't reorder >> anything here because the *vblanktime assignment depends on cur_vblank from >> the preceeding atomic_read. >> >> But why can we now do the comparison while(cur_vblank != vblank->count) >> without needing something like >> >> new_vblank = vblank->count; >> smp_rmb(); >> } while (cur_vblank != new_vblank); >> >> to make sure the value from the 2nd vblank->count read isn't stale wrt. to >> potential updates from store_vblank()? > > Similar response here. > > What matters is that the timestamp read is consistent with the > vblank count; not that you "just missed" new values. > >> Another question is why the drm_vblank_count_and_time() code ever worked >> without triggering any of my own tests and consistency checks in my >> software, or any of your igt tests? I run my tests very often, but only on >> Intel architecture cpus. I assume the same is true for the igt tests? Is >> there anything specific about Intel cpu's that makes this still work or very >> unlikely to break? Or are the tests insufficient to catch this? Or just luck? > > Intel x86 cpus are strongly-ordered, so smp_rmb() and smp_wmb() are only > compiler barriers on x86 arch. > Ok, that makes sense as part of the explanation why stuff still worked, at least on the tested x86 arch. > The compiler could have hoisted the vblanktimestamp read in front of > the vblank count read, but chose not to. > This one still confuses me. I know why the smp_rmb after the vblanktimestamp read is there - i placed one there in the current code myself. But why could the compiler - in absence of the new smp_rmb compiler barrier in this patch - reorder those two loads without violating the semantic of the code? Why could it have turned this cur_vblank = vblank->count; // smp_rmb(); vblanktime = vblanktimestamp(dev, crtc, cur_vblank); into this instead *vblanktime = vblanktimestamp(dev, crtc, cur_vblank); // smp_rmb(); cur_vblank = vblank->count; when the first load would need the value of cur_vblank - and thereby the result of the 2nd load - as input to calculate its address for the *vblanktime = ... load? I think i'm still not getting something about why the compiler would be allowed to reorder in this way in absence of the additional smp_rmb? Or is that barrier required for other archs which are less strongly ordered? thanks, -mario >> I looked through kernels back to 3.16 and most uses of the function would be >> safe from races due to the locking around it, holding of vblank refcounts, >> or the place and order of execution, e.g., from within drm_handle_vblank(). >> But in some tight test loop just calling the drmWaitVblank ioctl to query >> current values i'd expect it to at least occassionally return corrupted >> timestamps, e.g., time jumping forward or backward, etc.? > > As a test, reverse the load order of vblanktimestamp wrt vblank count > and see if your tests trip; if not, you know the tests are insufficient. > > Regards, > Peter Hurley > >> >>> return cur_vblank; >>> } >>> @@ -1715,7 +1731,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 +1747,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; >> >> Why is count an unsigned long (= 64 bit on 64-bit kernels) instead of u32 >> when all users of count are u32? Is this intentional? >> >> >>> + /* 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 */ >>> >> >> Thanks, >> -mario >