On Sat, 2018-03-03 at 22:30 +0000, Sasha Levin wrote: > From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > [ Upstream commit 608b20506941969ea30d8c08dc9ae02bb87dbf7d ] I think we need this one too: commit 75cff0837c14eaf632efabb8d7ab9eec6394d20d Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> Date: Fri Mar 24 17:30:58 2017 +0000 drm: Make the decision to keep vblank irq enabled earlier Ben. > 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> > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > Link: http://patchwork.freedesktop.org/patch/msgid/20170315204027.20160-1-chris@xxxxxxxxxxxxxxxxxx > Signed-off-by: Sasha Levin <alexander.levin@xxxxxxxxxxxxx> > --- > 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 8090989185b2..4ddbc49125cd 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -1271,9 +1271,9 @@ 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)); > } > @@ -1902,6 +1902,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.14.1 -- Ben Hutchings Software Developer, Codethink Ltd.