On Mon, Jul 03, 2017 at 05:26:24PM +0300, Ville Syrjälä wrote: > 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@xxxxxxxx> wrote: > > > On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote: > > >> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > >> > > >> 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@xxxxxxxx> > > >> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > 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. Yeah, relying on some implict vblank_get happening in the right way, far away from the arm_vblank_event, is the part I don't like. Makes review harder, and I don't see the gain we'll get in return. > 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. I think at that point we should write our own arm_vblank_event and cache the accurate vblank ts within i915 code. My point is that for a generic helper, we should aim for safe over fastest option. Having a special vblank_get that you have to call before you call arm_vblank_event, and within the critical section, just adds to the list of caveats of that function. Simply making arm_vblank_event more robust seems like the much better plan (and we can always fix things in i915 if the overhead is too much). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
