[PATCH] drm/vblank: Fix race between drm_crtc_arm_vblank_event() and the irq

[Ville Syrjälä]

On Fri, Jun 30, 2017 at 03:36:50PM +0200, Daniel Vetter wrote:
> On Fri, Jun 30, 2017 at 04:26:49PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 30, 2017 at 03:12:51PM +0200, Daniel Vetter wrote:
> > > On Fri, Jun 30, 2017 at 03:27:29PM +0300, ville.syrjala at linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > 
> > > > Make sure the armed even doesn't fire until the next vblank by adding
> > > > one to the current vblank counter value. This will prevent the event
> > > > being fired off prematurely if drm_handle_vblank() is called
> > > > redundantly, or if the irq handler gets delayed somehow.
> > > > 
> > > > This is actually a real race on Intel gen2/3 hardware due to the vblank
> > > > interrupt firing approx. one or more scanlines after the start of vblank
> > > > (which is when the double buffered registers get latched). So if we were
> > > > to perform an atomic update just after the start of vblank, but before
> > > > the irq has fired, the irq handler would send out the the event immediately
> > > > instead of waiting for the next vblank like it should.
> > > > 
> > > > This also makes logs less confusing because now it normally looks like
> > > > the vblank interrupt was somehow missed and the event gets sent one
> > > > frame too late.
> > > 
> > > So this para here I like, since the others are already written off in the
> > > kernel-doc as "don't use this function if this is a problem for you". I'd
> > > say your claim is even wrong, since we're not using
> > > drm_accurate_vblank_count, and so the race still exists (if you're
> > > sufficiently unlucky at least, and since there can be an arbitrary delay
> > > between when the hw raises an irq and when the kernel runs the irq
> > > handler it's not any better).
> > 
> > At least for i915 it should be accurate if the irq wasn't enabled
> > prior to starting the pipe update as we'll update the counter when
> > enabling the irq. To make sure it's accurate we should probably
> > have a drm_vblank_get_accurate() or something like that.
> 
> We could switch the drm_vblank_count in the diff below to
> drm_accurate_vblank_count.

That's somewhat sub-optimal as we can avoid the update when we know that
drm_vblank_get() already did it for us.

> I typed this helper for drivers which don't
> have accurate vblank timestamping support, so the difference was moot. But
> with i915 using it that's changed (and iirc vc4 uses it too or something
> like that).
> -Daniel
> 
> > 
> > > 
> > > Anyway, as long as it's not cc: stable (that would need more convincing on
> > > my side):
> > > 
> > > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > 
> > > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_vblank.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > index bfe86fa2d3b1..823c853de052 100644
> > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > @@ -869,7 +869,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> > > >  	assert_spin_locked(&dev->event_lock);
> > > >  
> > > >  	e->pipe = pipe;
> > > > -	e->event.sequence = drm_vblank_count(dev, pipe);
> > > > +	e->event.sequence = drm_vblank_count(dev, pipe) + 1;
> > > >  	e->event.crtc_id = crtc->base.id;
> > > >  	list_add_tail(&e->base.link, &dev->vblank_event_list);
> > > >  }
> > > > -- 
> > > > 2.13.0
> > > > 
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC