From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank. Date: Thu, 28 Apr 2011 13:34:14 -0700 Message-ID: <20110428133414.270834b5@jbarnes-desktop> References: <1303884659-739-1-git-send-email-christopher.halse.rogers@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy6-pub.bluehost.com (oproxy6-pub.bluehost.com [67.222.54.6]) by gabe.freedesktop.org (Postfix) with SMTP id 7281F9E73D for ; Thu, 28 Apr 2011 13:34:18 -0700 (PDT) In-Reply-To: <1303884659-739-1-git-send-email-christopher.halse.rogers@canonical.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: christopher.halse.rogers@canonical.com Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Wed, 27 Apr 2011 16:10:57 +1000 christopher.halse.rogers@canonical.com wrote: > From: Christopher James Halse Rogers > > This is the least-bad behaviour. It means that we signal the > vblank event before it actually happens, but since we're disabling > vblanks there's no guarantee that it will *ever* happen otherwise. > > This prevents GL applications which use WaitMSC from hanging > indefinitely. > > Signed-off-by: Christopher James Halse Rogers > --- > drivers/gpu/drm/drm_irq.c | 23 +++++++++++++++++++++++ > 1 files changed, 23 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 741457b..a1f12cb 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -932,11 +932,34 @@ EXPORT_SYMBOL(drm_vblank_put); > > void drm_vblank_off(struct drm_device *dev, int crtc) > { > + struct drm_pending_vblank_event *e, *t; > + struct timeval now; > unsigned long irqflags; > + unsigned int seq; > > spin_lock_irqsave(&dev->vbl_lock, irqflags); > vblank_disable_and_save(dev, crtc); > DRM_WAKEUP(&dev->vbl_queue[crtc]); > + > + /* Send any queued vblank events, lest the natives grow disquiet */ > + seq = drm_vblank_count_and_time(dev, crtc, &now); > + list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) { > + if (e->pipe != crtc) > + continue; > + DRM_DEBUG("Sending premature vblank event on disable: \ > + wanted %d, current %d\n", > + e->event.sequence, seq); > + > + e->event.sequence = seq; > + e->event.tv_sec = now.tv_sec; > + e->event.tv_usec = now.tv_usec; > + drm_vblank_put(dev, e->pipe); > + list_move_tail(&e->base.link, &e->base.file_priv->event_list); > + wake_up_interruptible(&e->base.file_priv->event_wait); > + trace_drm_vblank_event_delivered(e->base.pid, e->pipe, > + e->event.sequence); > + } > + > spin_unlock_irqrestore(&dev->vbl_lock, irqflags); > } > EXPORT_SYMBOL(drm_vblank_off); Yeah, this matches what we do for the blocking waits, and it's probably a good idea. Userspace can decide what to do with apps running against a disabled CRTC. Would be nice to share the code with drm_handle_vblank_events though somehow. Looks like e->event.sequence = seq; e->event.tv_sec = now.tv_sec; e->event.tv_usec = now.tv_usec; drm_vblank_put(dev, e->pipe); list_move_tail(&e->base.link, &e->base.file_priv->event_list); wake_up_interruptible(&e->base.file_priv->event_wait); trace_drm_vblank_event_delivered(e->base.pid, e->pipe, e->event.sequence); could be pulled out into a separate function for both to use. -- Jesse Barnes, Intel Open Source Technology Center