From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mario Kleiner Subject: Re: [PATCH 10/11] drm/omap: use drm_send_vblank_event() helper Date: Wed, 10 Oct 2012 05:33:10 +0200 Message-ID: <5074EC76.6010106@tuebingen.mpg.de> References: <1349725849-22433-1-git-send-email-rob.clark@linaro.org> <1349725849-22433-11-git-send-email-rob.clark@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.mpg.de (mx1.mpg.de [134.76.10.32]) by gabe.freedesktop.org (Postfix) with ESMTP id 34A779E9FC for ; Tue, 9 Oct 2012 20:33:24 -0700 (PDT) In-Reply-To: <1349725849-22433-11-git-send-email-rob.clark@linaro.org> 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: Rob Clark Cc: patches@linaro.org, daniel.vetter@ffwll.ch, dri-devel@lists.freedesktop.org, laurent.pinchart@ideasonboard.com, gregkh@linuxfoundation.org, Rob Clark , bskeggs@redhat.com List-Id: dri-devel@lists.freedesktop.org On 08.10.12 21:50, Rob Clark wrote: > From: Rob Clark > > Signed-off-by: Rob Clark > --- > drivers/staging/omapdrm/omap_crtc.c | 31 ++++++------------------------- > 1 file changed, 6 insertions(+), 25 deletions(-) > > diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c > index 732f2ad..74e019a 100644 > --- a/drivers/staging/omapdrm/omap_crtc.c > +++ b/drivers/staging/omapdrm/omap_crtc.c > @@ -114,40 +114,21 @@ static void omap_crtc_load_lut(struct drm_crtc *crtc) > > static void vblank_cb(void *arg) > { > - static uint32_t sequence = 0; > struct drm_crtc *crtc = arg; > struct drm_device *dev = crtc->dev; > struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > - struct drm_pending_vblank_event *event = omap_crtc->event; > unsigned long flags; > - struct timeval now; > > WARN_ON(!event); > + spin_lock_irqsave(&dev->event_lock, flags); > + > + /* wakeup userspace */ > + if (omap_crtc->event) > + drm_send_vblank_event(dev, -1, omap_crtc->event); > > omap_crtc->event = NULL; > > - /* wakeup userspace */ > - if (event) { > - do_gettimeofday(&now); > - > - spin_lock_irqsave(&dev->event_lock, flags); > - /* TODO: we can't yet use the vblank time accounting, > - * because omapdss lower layer is the one that knows > - * the irq # and registers the handler, which more or > - * less defeats how drm_irq works.. for now just fake > - * the sequence number and use gettimeofday.. > - * > - event->event.sequence = drm_vblank_count_and_time( > - dev, omap_crtc->id, &now); > - */ I think this TOO comment should be retained as a reminder that there is work to do. > - event->event.sequence = sequence++; This is problematic. You lose the pseudo vblank counter implemented here, which is a violation of the spec, and from my own experience it causes extra pain and the need for awful workarounds in userspace clients. Nouveau-kms has the same problem for no good reason. But then, on second thought, the way it is implemented here in the original is even more wrong, returning zero might be better. -mario > - event->event.tv_sec = now.tv_sec; > - event->event.tv_usec = now.tv_usec; > - list_add_tail(&event->base.link, > - &event->base.file_priv->event_list); > - wake_up_interruptible(&event->base.file_priv->event_wait); > - spin_unlock_irqrestore(&dev->event_lock, flags); > - } > + spin_unlock_irqrestore(&dev->event_lock, flags); > } > > static void page_flip_cb(void *arg) >