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@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>
> ---
> 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@xxxxxxxxxxxxxxx>
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
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
