| Message ID | 20190719152314.7706-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/vblank: Document and fix vblank count barrier semantics | expand |
On Fri, Jul 19, 2019 at 05:23:12PM +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. > > 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 | 38 +++++++++++++++++++++++++++++++++++- > include/drm/drm_vblank.h | 13 +++++++++++- > 2 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 603ab105125d..eb2a8304536c 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -295,11 +295,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 = vblank->count; Hmm. This is now a 64bit quantity, which means on 32bit the load/store won't be atomic. That doesn't seem particularly great. > + > + /* > + * 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; > } > > /**
On Fri, Jul 19, 2019 at 7:06 PM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, Jul 19, 2019 at 05:23:12PM +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. > > > > 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 | 38 +++++++++++++++++++++++++++++++++++- > > include/drm/drm_vblank.h | 13 +++++++++++- > > 2 files changed, 49 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > > index 603ab105125d..eb2a8304536c 100644 > > --- a/drivers/gpu/drm/drm_vblank.c > > +++ b/drivers/gpu/drm/drm_vblank.c > > @@ -295,11 +295,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 = vblank->count; > > Hmm. This is now a 64bit quantity, which means on 32bit the load/store > won't be atomic. That doesn't seem particularly great. Hm ... so read-side seqno here? At least for 32bit, but not sure that's worth it, probably simpler to just do it unconditionally. Otoh ... do we care? This matters like once every every year at 120Hz ... -Daniel > > > + > > + /* > > + * 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; > > } > > > > /** > > -- > Ville Syrjälä > Intel
On Fri, Jul 19, 2019 at 08:33:49PM +0200, Daniel Vetter wrote: > On Fri, Jul 19, 2019 at 7:06 PM Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > > > On Fri, Jul 19, 2019 at 05:23:12PM +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. > > > > > > 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 | 38 +++++++++++++++++++++++++++++++++++- > > > include/drm/drm_vblank.h | 13 +++++++++++- > > > 2 files changed, 49 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > > > index 603ab105125d..eb2a8304536c 100644 > > > --- a/drivers/gpu/drm/drm_vblank.c > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > @@ -295,11 +295,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 = vblank->count; > > > > Hmm. This is now a 64bit quantity, which means on 32bit the load/store > > won't be atomic. That doesn't seem particularly great. > > Hm ... so read-side seqno here? At least for 32bit, but not sure > that's worth it, probably simpler to just do it unconditionally. Or make it atomic64_t perhaps? > Otoh > ... do we care? This matters like once every every year at 120Hz ... Dunno. Might avoid a few odd bug reports maybe.
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 603ab105125d..eb2a8304536c 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -295,11 +295,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 = 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 +776,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. */ @@ -818,6 +838,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 +1819,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 e528bb2f659d..5ec623740158 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -110,7 +110,18 @@ 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; /**
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. 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 | 38 +++++++++++++++++++++++++++++++++++- include/drm/drm_vblank.h | 13 +++++++++++- 2 files changed, 49 insertions(+), 2 deletions(-)