Re: [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jun 30, 2017 at 08:18:19PM +0200, Daniel Vetter wrote:
> On Fri, Jun 30, 2017 at 5:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Add drm_crtc_vblank_get_accurate() which is the same as
> >> drm_crtc_vblank_get() except that it will forcefully update the
> >> the current vblank counter/timestamp value even if the interrupt
> >> is already enabled.
> >>
> >> And we'll switch drm_wait_one_vblank() over to using it to make sure it
> >> will really wait at least one vblank even if it races with the irq
> >> handler.
> >>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Hm, I just thought of doing an
> > s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
> > drm_crtc_arm_vblank_event.
> >
> > What does this buy us above&beyond the other patch? And why isn't
> > vblank_get accurate always?
> 
> This also seems to have the risk of not fixing the arm_vblank_event
> race if someone puts the vblank_get_accurate outside of the critical
> section. Then we're back to the same old race. And since that means
> you need to have a vblank_get_accurate right before the
> arm_vblank_event in all cases, we might as well just put it into the
> helper. You wrote in the other thread putting it into arm_vblank_event
> might be wasteful, but I don't see any scenario where that could be
> the case ...
> 
> Or do I miss something?

The interrupt most likely will be on already when pipe_update_end() gets
called since pipe_update_start() will enable it already, and Chris's
disable_immediate optimization will keep it on until the next interrupt.
Prior to that optimization the drm_vblank_get() in pipe_update_end()
would have had to turn on the interrupt and perform the vblank counter
update, and thus the second update from drm_crtc_accurate_vblank_count()
would have been wasteful.

I'm not sure I like relying on the fact that pipe_update_start() already
turned the interrupt on and it's going to be kept on magically. One
solution for that would be to hang on to the reference from
pipe_update_start() until we've armed the event. Then we know the
vblank_get() won't have to enable the interrupt.

However, if we start to sample the counter for some other purpose
(watermarks, fb unpinning etc.) during pipe_update_end() then we'll
again be faced with another potentially pointless update if we
decide to use drm_crtc_accurate_vblank_count() again.

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