Re: [PATCH 1/3] drm/vblank: Document and fix vblank count barrier semantics

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 1/3] drm/vblank: Document and fix vblank count barrier semantics
Date: Fri, 19 Jul 2019 21:56:42 +0300	[thread overview]
Message-ID: <20190719185642.GU5942@intel.com> (raw)
In-Reply-To: <CAKMK7uFmF7sMoJj=1C+70nyd5rMoan20hAGb2gtY4o0D8Y4cOQ@mail.gmail.com>

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.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx