[Intel-gfx] [PATCH 1/3] drm/vblank: Document and fix vblank count barrier semantics

[Daniel Vetter]

On Fri, Jul 19, 2019 at 7:06 PM Ville Syrjälä
<ville.syrjala at 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 at gmail.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter at 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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch