On Tue, Jul 23, 2019 at 03:13:37PM +0200, 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 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
Reviewed-by: Ville Syrjälä ville.syrjala@linux.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();
- 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);@@ -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