[Intel-gfx] [PATCH 1/3] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)

[Ville Syrjälä]

On Wed, Mar 22, 2017 at 04:06:32PM +0100, Mario Kleiner wrote:
> On 03/15/2017 10:00 PM, Ville Syrjälä wrote:
> > On Wed, Mar 15, 2017 at 08:40:25PM +0000, Chris Wilson wrote:
> >> On vblank instant-off systems, we can get into a situation where the cost
> >> of enabling and disabling the vblank IRQ around a drmWaitVblank query
> >> dominates. And with the advent of even deeper hardware sleep state,
> >> touching registers becomes ever more expensive.  However, we know that if
> >> the user wants the current vblank counter, they are also very likely to
> >> immediately queue a vblank wait and so we can keep the interrupt around
> >> and only turn it off if we have no further vblank requests queued within
> >> the interrupt interval.
> >>
> >> After vblank event delivery, this patch adds a shadow of one vblank where
> >> the interrupt is kept alive for the user to query and queue another vblank
> >> event. Similarly, if the user is using blocking drmWaitVblanks, the
> >> interrupt will be disabled on the IRQ following the wait completion.
> >> However, if the user is simply querying the current vblank counter and
> >> timestamp, the interrupt will be disabled after every IRQ and the user
> >> will enabled it again on the first query following the IRQ.
> >>
> >> v2: Mario Kleiner -
> >> After testing this, one more thing that would make sense is to move
> >> the disable block at the end of drm_handle_vblank() instead of at the
> >> top.
> >>
> >> Turns out that if high precision timestaming is disabled or doesn't
> >> work for some reason (as can be simulated by echo 0 >
> >> /sys/module/drm/parameters/timestamp_precision_usec), then with your
> >> delayed disable code at its current place, the vblank counter won't
> >> increment anymore at all for instant queries, ie. with your other
> >> "instant query" patches. Clients which repeatedly query the counter
> >> and wait for it to progress will simply hang, spinning in an endless
> >> query loop. There's that comment in vblank_disable_and_save:
> >>
> >> "* Skip this step if there isn't any high precision timestamp
> >>  * available. In that case we can't account for this and just
> >>  * hope for the best.
> >>  */
> >>
> >> With the disable happening after leading edge of vblank (== hw counter
> >> increment already happened) but before the vblank counter/timestamp
> >> handling in drm_handle_vblank, that step is needed to keep the counter
> >> progressing, so skipping it is bad.
> >>
> >> Now without high precision timestamping support, a kms driver must not
> >> set dev->vblank_disable_immediate = true, as this would cause problems
> >> for clients, so this shouldn't matter, but it would be good to still
> >> make this robust against a future kms driver which might have
> >> unreliable high precision timestamping, e.g., high precision
> >> timestamping that intermittently doesn't work.
> >>
> >> v3: Patch before coffee needs extra coffee.
> >>
> >> Testcase: igt/kms_vblank
> >> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> Cc: Daniel Vetter <daniel at ffwll.ch>
> >> Cc: Michel Dänzer <michel at daenzer.net>
> >> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> Cc: Dave Airlie <airlied at redhat.com>,
> >> Cc: Mario Kleiner <mario.kleiner.de at gmail.com>
> >
> > Yep. This seems like a good idea to me. I just neglected to review it
> > last time around (and maybe even before that?) for some reason. Locks
> > seem to be taken in the right order, so it at least looks safe to me.
> >
> > Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> 
> Hi,
> 
> as a followup to this one, maybe we should move the 
> drm_handle_vblank_events(dev, pipe); down, immediately after Chris new 
> delayed disable code?
> 
> The idea was to avoid lots of redundant enable->disable->enable... calls 
> by having some 1 frame delay before disable. This works for pure vblank 
> count/ts queries.
> 
> But both DRI2 and DRI3/Present use vblank events to trigger a 
> pageflip-ioctl at the right target vblank. With the current ordering we 
> may dispatch the vblank swap trigger event to the X-Server and drop the 
> vblank refcount to zero due to the vblank_put inside 
> drm_handle_vblank_events for the dispatched event, then detect in this 
> patch that refcount == 0 and disable vblanks, but a few microseconds 
> later the server will queue a pageflip ioctl which bumps the refcount 
> and reenables vblank irqs, so we have a redundant disable->enable.
> 
> Also many kms drivers now use drm_crtc_arm_vblank_event() for pageflip 
> completion handling at vblank, the pageflip completion events are also 
> dispatched via drm_handle_vblank_events(). After a pageflip completes, 
> it makes sense to have this "swap shadow" of 1 full frame, as animations 
> would likely queue a new vblank query/event immediately for the next 
> animation frame.

That does seem like a decent idea. It won't actually change anything for
i915 page flips since we still hang on to our vblank reference after
drm_handle_vblank() returns. But if you, for example, just call
glXWaitVideoSyncSGI(1,0,...) in a loop the current code will still
result on enable<->disable ping-pong, whereas with your proposed
reordering we'd keep the vblank interrupt enabled all the time.

Chris, any thoughts?

-- 
Ville Syrjälä
Intel OTC