| Message ID | 20170315204027.20160-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Michel Dänzer <michel@daenzer.net> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Dave Airlie <airlied@redhat.com>, > Cc: Mario Kleiner <mario.kleiner.de@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@linux.intel.com> > --- > drivers/gpu/drm/drm_irq.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 9bdca69f754c..e64b05ea95ea 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -1198,9 +1198,9 @@ static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) > if (atomic_dec_and_test(&vblank->refcount)) { > if (drm_vblank_offdelay == 0) > return; > - else if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0) > + else if (drm_vblank_offdelay < 0) > vblank_disable_fn((unsigned long)vblank); > - else > + else if (!dev->vblank_disable_immediate) > mod_timer(&vblank->disable_timer, > jiffies + ((drm_vblank_offdelay * HZ)/1000)); > } > @@ -1734,6 +1734,16 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) > wake_up(&vblank->queue); > drm_handle_vblank_events(dev, pipe); > > + /* With instant-off, we defer disabling the interrupt until after > + * we finish processing the following vblank. The disable has to > + * be last (after drm_handle_vblank_events) so that the timestamp > + * is always accurate. > + */ > + if (dev->vblank_disable_immediate && > + drm_vblank_offdelay > 0 && > + !atomic_read(&vblank->refcount)) > + vblank_disable_fn((unsigned long)vblank); > + > spin_unlock_irqrestore(&dev->event_lock, irqflags); > > return true; > -- > 2.11.0
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@chris-wilson.co.uk> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Cc: Michel Dänzer <michel@daenzer.net> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Cc: Dave Airlie <airlied@redhat.com>, >> Cc: Mario Kleiner <mario.kleiner.de@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@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. -mario >> --- >> drivers/gpu/drm/drm_irq.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c >> index 9bdca69f754c..e64b05ea95ea 100644 >> --- a/drivers/gpu/drm/drm_irq.c >> +++ b/drivers/gpu/drm/drm_irq.c >> @@ -1198,9 +1198,9 @@ static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) >> if (atomic_dec_and_test(&vblank->refcount)) { >> if (drm_vblank_offdelay == 0) >> return; >> - else if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0) >> + else if (drm_vblank_offdelay < 0) >> vblank_disable_fn((unsigned long)vblank); >> - else >> + else if (!dev->vblank_disable_immediate) >> mod_timer(&vblank->disable_timer, >> jiffies + ((drm_vblank_offdelay * HZ)/1000)); >> } >> @@ -1734,6 +1734,16 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) >> wake_up(&vblank->queue); >> drm_handle_vblank_events(dev, pipe); >> >> + /* With instant-off, we defer disabling the interrupt until after >> + * we finish processing the following vblank. The disable has to >> + * be last (after drm_handle_vblank_events) so that the timestamp >> + * is always accurate. >> + */ >> + if (dev->vblank_disable_immediate && >> + drm_vblank_offdelay > 0 && >> + !atomic_read(&vblank->refcount)) >> + vblank_disable_fn((unsigned long)vblank); >> + >> spin_unlock_irqrestore(&dev->event_lock, irqflags); >> >> return true; >> -- >> 2.11.0 >
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@chris-wilson.co.uk> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Daniel Vetter <daniel@ffwll.ch> > >> Cc: Michel Dänzer <michel@daenzer.net> > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Cc: Dave Airlie <airlied@redhat.com>, > >> Cc: Mario Kleiner <mario.kleiner.de@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@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?
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 9bdca69f754c..e64b05ea95ea 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1198,9 +1198,9 @@ static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) if (atomic_dec_and_test(&vblank->refcount)) { if (drm_vblank_offdelay == 0) return; - else if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0) + else if (drm_vblank_offdelay < 0) vblank_disable_fn((unsigned long)vblank); - else + else if (!dev->vblank_disable_immediate) mod_timer(&vblank->disable_timer, jiffies + ((drm_vblank_offdelay * HZ)/1000)); } @@ -1734,6 +1734,16 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) wake_up(&vblank->queue); drm_handle_vblank_events(dev, pipe); + /* With instant-off, we defer disabling the interrupt until after + * we finish processing the following vblank. The disable has to + * be last (after drm_handle_vblank_events) so that the timestamp + * is always accurate. + */ + if (dev->vblank_disable_immediate && + drm_vblank_offdelay > 0 && + !atomic_read(&vblank->refcount)) + vblank_disable_fn((unsigned long)vblank); + spin_unlock_irqrestore(&dev->event_lock, irqflags); return true;
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@chris-wilson.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Michel Dänzer <michel@daenzer.net> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Dave Airlie <airlied@redhat.com>, Cc: Mario Kleiner <mario.kleiner.de@gmail.com> --- drivers/gpu/drm/drm_irq.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)