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@xxxxxxxxxxxxxxxxxx> > >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> Cc: Daniel Vetter <daniel@xxxxxxxx> > >> Cc: Michel Dänzer <michel@xxxxxxxxxxx> > >> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > >> Cc: Dave Airlie <airlied@xxxxxxxxxx>, > >> Cc: Mario Kleiner <mario.kleiner.de@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > > > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
