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

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

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@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.

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Jun 30, 2017 at 03:27:29PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@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).

Anyway, as long as it's not cc: stable (that would need more convincing on
my side):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@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.

> 
> Anyway, as long as it's not cc: stable (that would need more convincing on
> my side):
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@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. 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@ffwll.ch>
> > > 
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@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@ffwll.ch>
> > > > 
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel