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@xxxxxxxxxxxxxxx wrote: > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > > 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@xxxxxxxx> > > > > > > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > --- > > > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
