| Message ID | 20190723131337.22031-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vblank: Document and fix vblank count barrier semantics | expand |
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); > *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
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? 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? 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 >
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
Hi, thanks for the explanation. I noticed that I forgot to add my r-b. Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> 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
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
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); *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. */
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> --- drivers/gpu/drm/drm_vblank.c | 45 ++++++++++++++++++++++++++++++++---- include/drm/drm_vblank.h | 15 ++++++++++-- 2 files changed, 54 insertions(+), 6 deletions(-)