| Message ID | 20170323075106.7494-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Better subject might be drm: Use vblank irq shadow for entire flip sequence
On Thu, Mar 23, 2017 at 07:51:06AM +0000, Chris Wilson wrote: > We want to provide the vblank irq shadow for pageflip events as well as > vblank queries. Such events are completed within the vblank interrupt > handler, and so the current check for disabling the irq will disable it > from with the same interrupt as the last pageflip event. If we move the > decision on whether to disable the irq (based on there no being no > remaining vblank events, i.e. vblank->refcount == 0) to before we signal > the events, we will only disable the irq on the interrupt after the last > event was signaled. In the normal course of events, this will keep the > vblank irq enabled for the entire flip sequence whereas before it would > flip-flop around every interrupt. > > 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 | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 5b77057e91ca..1d6bcee3708f 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -1741,6 +1741,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) > { > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > unsigned long irqflags; > + bool disable_irq; > > if (WARN_ON_ONCE(!dev->num_crtcs)) > return false; > @@ -1768,16 +1769,19 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) > spin_unlock(&dev->vblank_time_lock); > > 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. > + * we finish processing the following vblank after all events have > + * been signaled. The disable has to be last (after > + * drm_handle_vblank_events) so that the timestamp is always accurate. We wouldn't actually do the disable as long there's a reference still held, so the timestamp should be fine in that case. And if there aren't any references the timestamp shouldn't matter... I think. But it's probably more clear to keep to the order you propose here anyway. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Oh, and now that I think about this stuff again, I start to wonder why I made the disable actually update the seq/ts. If the interrupt is currently enabled the seq/ts should be reasonably uptodate already when we do disable the interrupt. Perhaps I was only thinking about drm_vblank_off() when I made that change, or I decided that I didn't want two different disable codepaths. Anyways, just an idea that we might be able to make the vblank irq disable a little cheaper. > */ > - if (dev->vblank_disable_immediate && > - drm_vblank_offdelay > 0 && > - !atomic_read(&vblank->refcount)) > + disable_irq = (dev->vblank_disable_immediate && > + drm_vblank_offdelay > 0 && > + !atomic_read(&vblank->refcount)); > + > + drm_handle_vblank_events(dev, pipe); > + > + if (disable_irq) > vblank_disable_fn((unsigned long)vblank); > > spin_unlock_irqrestore(&dev->event_lock, irqflags); > -- > 2.11.0
On 03/23/2017 02:26 PM, Ville Syrjälä wrote: > On Thu, Mar 23, 2017 at 07:51:06AM +0000, Chris Wilson wrote: >> We want to provide the vblank irq shadow for pageflip events as well as >> vblank queries. Such events are completed within the vblank interrupt >> handler, and so the current check for disabling the irq will disable it >> from with the same interrupt as the last pageflip event. If we move the >> decision on whether to disable the irq (based on there no being no >> remaining vblank events, i.e. vblank->refcount == 0) to before we signal >> the events, we will only disable the irq on the interrupt after the last >> event was signaled. In the normal course of events, this will keep the >> vblank irq enabled for the entire flip sequence whereas before it would >> flip-flop around every interrupt. >> >> 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 | 18 +++++++++++------- >> 1 file changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c >> index 5b77057e91ca..1d6bcee3708f 100644 >> --- a/drivers/gpu/drm/drm_irq.c >> +++ b/drivers/gpu/drm/drm_irq.c >> @@ -1741,6 +1741,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) >> { >> struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; >> unsigned long irqflags; >> + bool disable_irq; >> >> if (WARN_ON_ONCE(!dev->num_crtcs)) >> return false; >> @@ -1768,16 +1769,19 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) >> spin_unlock(&dev->vblank_time_lock); >> >> 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. >> + * we finish processing the following vblank after all events have >> + * been signaled. The disable has to be last (after >> + * drm_handle_vblank_events) so that the timestamp is always accurate. > > We wouldn't actually do the disable as long there's a reference still > held, so the timestamp should be fine in that case. And if there aren't > any references the timestamp shouldn't matter... I think. But it's > probably more clear to keep to the order you propose here anyway. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Looks good to me. As a further optimization, i think we could move the vblank_disable_fn() call outside/below the spin_unlock_irqrestore for event_lock, as vblank_disable_fn() doesn't need any locks held at call time, so slightly reduce event_lock hold time. Don't know if it is worth it. In any case Reviewed-by: Mario Kleiner <mario.kleiner.de@gmail.com> thanks, -mario > Oh, and now that I think about this stuff again, I start to wonder why > I made the disable actually update the seq/ts. If the interrupt is > currently enabled the seq/ts should be reasonably uptodate already > when we do disable the interrupt. Perhaps I was only thinking about > drm_vblank_off() when I made that change, or I decided that I didn't > want two different disable codepaths. Anyways, just an idea that > we might be able to make the vblank irq disable a little cheaper. > >> */ >> - if (dev->vblank_disable_immediate && >> - drm_vblank_offdelay > 0 && >> - !atomic_read(&vblank->refcount)) >> + disable_irq = (dev->vblank_disable_immediate && >> + drm_vblank_offdelay > 0 && >> + !atomic_read(&vblank->refcount)); >> + >> + drm_handle_vblank_events(dev, pipe); >> + >> + if (disable_irq) >> vblank_disable_fn((unsigned long)vblank); >> >> spin_unlock_irqrestore(&dev->event_lock, irqflags); >> -- >> 2.11.0 >
On Fri, Mar 24, 2017 at 04:16:28AM +0100, Mario Kleiner wrote: > Looks good to me. As a further optimization, i think we could move > the vblank_disable_fn() call outside/below the > spin_unlock_irqrestore for event_lock, as vblank_disable_fn() > doesn't need any locks held at call time, so slightly reduce > event_lock hold time. Don't know if it is worth it. Hmm, I was cautious since I don't trust myself around the vbl spinlocks. But yes, the vblank->refcount is controlled by vbl_lock and that is checked inside the disable callback, so we can move it out from the vblank_event_lock. -Chris
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 5b77057e91ca..1d6bcee3708f 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1741,6 +1741,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; unsigned long irqflags; + bool disable_irq; if (WARN_ON_ONCE(!dev->num_crtcs)) return false; @@ -1768,16 +1769,19 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) spin_unlock(&dev->vblank_time_lock); 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. + * we finish processing the following vblank after all events have + * been signaled. 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)) + disable_irq = (dev->vblank_disable_immediate && + drm_vblank_offdelay > 0 && + !atomic_read(&vblank->refcount)); + + drm_handle_vblank_events(dev, pipe); + + if (disable_irq) vblank_disable_fn((unsigned long)vblank); spin_unlock_irqrestore(&dev->event_lock, irqflags);
We want to provide the vblank irq shadow for pageflip events as well as vblank queries. Such events are completed within the vblank interrupt handler, and so the current check for disabling the irq will disable it from with the same interrupt as the last pageflip event. If we move the decision on whether to disable the irq (based on there no being no remaining vblank events, i.e. vblank->refcount == 0) to before we signal the events, we will only disable the irq on the interrupt after the last event was signaled. In the normal course of events, this will keep the vblank irq enabled for the entire flip sequence whereas before it would flip-flop around every interrupt. 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 | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)