On Mon, Oct 08, 2012 at 02:50:39PM -0500, Rob Clark wrote:

From: Rob Clark rob@ti.com

A helper that drivers can use to send vblank event after a pageflip. If the driver doesn't support proper vblank irq based time/seqn then just pass -1 for the pipe # to get do_gettimestamp() behavior (since there are a lot of drivers that don't use drm_vblank_count_and_time())

Also an internal send_vblank_event() helper for the various other code paths within drm_irq that also need to send vblank events.

Signed-off-by: Rob Clark rob@ti.com

drivers/gpu/drm/drm_irq.c | 65 +++++++++++++++++++++++++++++---------------- include/drm/drmP.h | 2 ++ 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 076c4a8..7a00d94 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -802,6 +802,43 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, } EXPORT_SYMBOL(drm_vblank_count_and_time);

+static void send_vblank_event(struct drm_pending_vblank_event *e,

• unsigned long seq, struct timeval *now)
  

+{

• e->event.sequence = seq;
• e->event.tv_sec = now->tv_sec;
• e->event.tv_usec = now->tv_usec;
• list_add_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);
  

+}

+/**

• • drm_send_vblank_event - helper to send vblank event after pageflip
• • @dev: DRM device
• • @crtc: CRTC in question
• • @e: the event to send
• • Updates sequence # and timestamp on event, and sends it to userspace.
• */

+void drm_send_vblank_event(struct drm_device *dev, int crtc,

• struct drm_pending_vblank_event *e)
  

+{

• struct timeval now;
• unsigned int seq;
• if (crtc >= 0) {

• seq = drm_vblank_count_and_time(dev, crtc, &now);
  

• } else {

• seq = 0;
  

• do_gettimeofday(&now);
  

• }
• send_vblank_event(e, seq, &now);

+} +EXPORT_SYMBOL(drm_send_vblank_event);

/**

• drm_update_vblank_count - update the master vblank counter
• @dev: DRM device

@@ -955,15 +992,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc) 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;
  

• list_del(&e->base.link);
  

  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);
  

• send_vblank_event(e, seq, &now);
  

  }

  spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

@@ -1107,15 +1138,8 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,

e->event.sequence = vblwait->request.sequence; if ((seq - vblwait->request.sequence) <= (1 << 23)) {

• e->event.sequence = seq;
  

• e->event.tv_sec = now.tv_sec;
  

• e->event.tv_usec = now.tv_usec;
  

  drm_vblank_put(dev, pipe);

• list_add_tail(&e->base.link, &e->base.file_priv->event_list);
  

• wake_up_interruptible(&e->base.file_priv->event_wait);
  

• vblwait->reply.sequence = seq;
  

Déjà vu

http://lists.freedesktop.org/archives/dri-devel/2011-April/010693.html

:)

• trace_drm_vblank_event_delivered(current->pid, pipe,
  

• 				 vblwait->request.sequence);
  

• send_vblank_event(e, seq, &now);
  

  } else { /* drm_handle_vblank_events will call drm_vblank_put */ list_add_tail(&e->base.link, &dev->vblank_event_list);

@@ -1256,14 +1280,9 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc) DRM_DEBUG("vblank event on %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;
  

• list_del(&e->base.link);
  

  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);
  

• send_vblank_event(e, seq, &now);
  

  }

  spin_unlock_irqrestore(&dev->event_lock, flags);

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 05af0e7..ee8f927 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1437,6 +1437,8 @@ extern int drm_vblank_wait(struct drm_device *dev, unsigned int *vbl_seq); extern u32 drm_vblank_count(struct drm_device *dev, int crtc); extern u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, struct timeval *vblanktime); +extern void drm_send_vblank_event(struct drm_device *dev, int crtc,

• 		     struct drm_pending_vblank_event *e);
  

extern bool drm_handle_vblank(struct drm_device *dev, int crtc); extern int drm_vblank_get(struct drm_device *dev, int crtc); extern void drm_vblank_put(struct drm_device *dev, int crtc); --

On Thu, Oct 11, 2012 at 9:19 AM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Rob,

Thanks for the patch.

On Monday 08 October 2012 14:50:39 Rob Clark wrote:

...

From: Rob Clark rob@ti.com

A helper that drivers can use to send vblank event after a pageflip. If the driver doesn't support proper vblank irq based time/seqn then just pass -1 for the pipe # to get do_gettimestamp() behavior (since there are a lot of drivers that don't use drm_vblank_count_and_time())

Also an internal send_vblank_event() helper for the various other code paths within drm_irq that also need to send vblank events.

Signed-off-by: Rob Clark rob@ti.com

drivers/gpu/drm/drm_irq.c | 65 +++++++++++++++++++++++++++--------------- include/drm/drmP.h | 2 ++ 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 076c4a8..7a00d94 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -802,6 +802,43 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, } EXPORT_SYMBOL(drm_vblank_count_and_time);

+static void send_vblank_event(struct drm_pending_vblank_event *e,

•         unsigned long seq, struct timeval *now)
  

+{

• e->event.sequence = seq;
  

• e->event.tv_sec = now->tv_sec;
  

• e->event.tv_usec = now->tv_usec;
  

• list_add_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);
  

+}

+/**

• • drm_send_vblank_event - helper to send vblank event after pageflip
• • @dev: DRM device
• • @crtc: CRTC in question
• • @e: the event to send
• • Updates sequence # and timestamp on event, and sends it to userspace.

Due to the list_add_tail above, drm_send_vblank_event() requires locking. As you don't take any lock I expect the caller to be supposed to handle locking. That should be documented, along with the lock that the caller should take. Looking at the existing drivers that should be dev->event_lock, but not all drivers take it when calling the vblank functions below (for instance the i915 and gma500 drivers call drm_vblank_off() without holding the event_lock spinlock). We thus seem to have a locking issue that should be fixed, either as part of this patch set or as a preliminary patches series.

I have no strong opinion on who takes the spinlock (the caller or the send_vblank_event() function) at the moment, but taking it in send_vblank_event() would allow calling drm_vblank_count_and_time() and do_gettimeofday() below outside of the locked region.

I think probably I should add a comment that event_lock should be held. Most/all drivers are anyways having to hold the lock to update their own state around the call to send_vblank_event().

Seems like there is some room for some cleanup around this.. drm_vblank_off() grabs a *different* lock.. I guess we need to document that drm_vblank_off() caller also holds event_lock.

Might even be a good idea to throw in a WARN_ON(!spin_is_locked())?

...

• */

+void drm_send_vblank_event(struct drm_device *dev, int crtc,

•         struct drm_pending_vblank_event *e)
  

+{

• struct timeval now;
  

• unsigned int seq;
  

• if (crtc >= 0) {
  

•         seq = drm_vblank_count_and_time(dev, crtc, &now);
  

• } else {
  

•         seq = 0;
  

•         do_gettimeofday(&now);
  

Do you know why some drivers don't call drm_vblank_count_and_time() ? For instance nouveau sets the sequence to 0 and uses do_gettimeofday(), but it looks like it could just call drm_vblank_count_and_time().

I know why the current omap driver doesn't, because it is not really able to use the vblank stuff properly due to omapdss/omapdrm layering issues. (But that should be solved w/ the omapdss/omapdrm cleanups I am working on and kms re-write)

Other drivers, I'm not really sure. But there were enough drivers that were using do_gettimeofday() that I thought it best not to ignore them.

BR, -R

...

• }
  

• send_vblank_event(e, seq, &now);
  

+} +EXPORT_SYMBOL(drm_send_vblank_event);

/**

• drm_update_vblank_count - update the master vblank counter
• @dev: DRM device

@@ -955,15 +992,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc) 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;
  

•         list_del(&e->base.link);
          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);
  

•         send_vblank_event(e, seq, &now);
  }
  
  spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
  

@@ -1107,15 +1138,8 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,

  e->event.sequence = vblwait->request.sequence;
  if ((seq - vblwait->request.sequence) <= (1 << 23)) {

•         e->event.sequence = seq;
  

•         e->event.tv_sec = now.tv_sec;
  

•         e->event.tv_usec = now.tv_usec;
          drm_vblank_put(dev, pipe);
  

•         list_add_tail(&e->base.link, &e->base.file_priv->event_list);
  

•         wake_up_interruptible(&e->base.file_priv->event_wait);
  

•         vblwait->reply.sequence = seq;
  

•         trace_drm_vblank_event_delivered(current->pid, pipe,
  

•                                          vblwait->request.sequence);
  

•         send_vblank_event(e, seq, &now);
  } else {
          /* drm_handle_vblank_events will call drm_vblank_put */
          list_add_tail(&e->base.link, &dev->vblank_event_list);
  

@@ -1256,14 +1280,9 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc) DRM_DEBUG("vblank event on %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;
  

•         list_del(&e->base.link);
          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);
  

•         send_vblank_event(e, seq, &now);
  }
  
  spin_unlock_irqrestore(&dev->event_lock, flags);
  

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 05af0e7..ee8f927 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1437,6 +1437,8 @@ extern int drm_vblank_wait(struct drm_device *dev, unsigned int *vbl_seq); extern u32 drm_vblank_count(struct drm_device *dev, int crtc); extern u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, struct timeval *vblanktime); +extern void drm_send_vblank_event(struct drm_device *dev, int crtc,

•                              struct drm_pending_vblank_event *e);
  

extern bool drm_handle_vblank(struct drm_device *dev, int crtc); extern int drm_vblank_get(struct drm_device *dev, int crtc); extern void drm_vblank_put(struct drm_device *dev, int crtc);

-- Regards,

Laurent Pinchart

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Oct 08, 2012 at 02:50:40PM -0500, Rob Clark wrote:

From: Rob Clark rob@ti.com

Signed-off-by: Rob Clark rob@ti.com

Maybe note in the commit message that after this change pageflip events will also fire the vblank_event_delivered tracepoint.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/i915/intel_display.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f5bcf6f..4716c83 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6158,8 +6158,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_unpin_work *work; struct drm_i915_gem_object *obj;

• struct drm_pending_vblank_event *e;

• struct timeval tvbl; unsigned long flags;

  /* Ignore early vblank irqs */

@@ -6175,17 +6173,8 @@ static void do_intel_finish_page_flip(struct drm_device *dev,

intel_crtc->unpin_work = NULL;

• if (work->event) {

• e = work->event;
  

• e->event.sequence = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
  

• e->event.tv_sec = tvbl.tv_sec;
  

• e->event.tv_usec = tvbl.tv_usec;
  

• list_add_tail(&e->base.link,
  

• 	      &e->base.file_priv->event_list);
  

• wake_up_interruptible(&e->base.file_priv->event_wait);
  

• }

• if (work->event)

• drm_send_vblank_event(dev, intel_crtc->pipe, work->event);
  
  

  drm_vblank_put(dev, intel_crtc->pipe);

-- 1.7.9.5

Hi Dave,

Got it. we have to re-work this patch. Will send it soon.

Thanks, Inki Dae

-----Original Message----- From: Dave Airlie [mailto:airlied@gmail.com] Sent: Wednesday, May 22, 2013 8:20 AM To: Inki Dae Cc: dri-devel@lists.freedesktop.org Subject: Re: [PATCH 05/11] drm/exynos: use drm_send_vblank_event() helper

Hi Inki,

this doesn't apply cleanly anymore, and I think we want exynos to use drm_send_vblank_event where possible,

please apply to fixes tree and send to me.

Dave.

On Tue, Oct 9, 2012 at 5:50 AM, Rob Clark rob.clark@linaro.org wrote:

...

From: Rob Clark rob@ti.com

Signed-off-by: Rob Clark rob@ti.com

drivers/gpu/drm/exynos/exynos_drm_fimd.c | 10 ++-------- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 10 ++-------- drivers/gpu/drm/exynos/exynos_mixer.c | 9 ++------- 3 files changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c

b/drivers/gpu/drm/exynos/exynos_drm_fimd.c

...

index b19cd93..fe8fb78 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -587,7 +587,6 @@ static void fimd_finish_pageflip(struct drm_device

*drm_dev, int crtc)

...

{ struct exynos_drm_private *dev_priv = drm_dev->dev_private; struct drm_pending_vblank_event *e, *t;

•   struct timeval now;
    unsigned long flags;
    bool is_checked = false;
  
  

@@ -601,13 +600,8 @@ static void fimd_finish_pageflip(struct drm_device

*drm_dev, int crtc)

...

            is_checked = true;

•           do_gettimeofday(&now);
  

•           e->event.sequence = 0;
  

•           e->event.tv_sec = now.tv_sec;
  

•           e->event.tv_usec = now.tv_usec;
  

•           list_move_tail(&e->base.link, &e->base.file_priv-
  

event_list);

•           wake_up_interruptible(&e->base.file_priv->event_wait);
  

•           list_del(&e->base.link);
  

•           drm_send_vblank_event(drm_dev, -1, e);
    }
  
    if (is_checked) {
  

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c

b/drivers/gpu/drm/exynos/exynos_drm_vidi.c

...

index e364165..4549efb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c @@ -370,7 +370,6 @@ static void vidi_finish_pageflip(struct drm_device

*drm_dev, int crtc)

...

{ struct exynos_drm_private *dev_priv = drm_dev->dev_private; struct drm_pending_vblank_event *e, *t;

•   struct timeval now;
    unsigned long flags;
    bool is_checked = false;
  
  

@@ -384,13 +383,8 @@ static void vidi_finish_pageflip(struct drm_device

*drm_dev, int crtc)

...

            is_checked = true;

•           do_gettimeofday(&now);
  

•           e->event.sequence = 0;
  

•           e->event.tv_sec = now.tv_sec;
  

•           e->event.tv_usec = now.tv_usec;
  

•           list_move_tail(&e->base.link, &e->base.file_priv-
  

event_list);

•           wake_up_interruptible(&e->base.file_priv->event_wait);
  

•           list_del(&e->base.link);
  

•           drm_send_vblank_event(drm_dev, -1, e);
    }
  
    if (is_checked) {
  

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c

b/drivers/gpu/drm/exynos/exynos_mixer.c

...

index 25b97d5..325aefd 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -828,7 +828,6 @@ static void mixer_finish_pageflip(struct drm_device

*drm_dev, int crtc)

...

{ struct exynos_drm_private *dev_priv = drm_dev->dev_private; struct drm_pending_vblank_event *e, *t;

•   struct timeval now;
    unsigned long flags;
    bool is_checked = false;
  
  

@@ -841,13 +840,9 @@ static void mixer_finish_pageflip(struct drm_device

*drm_dev, int crtc)

...

                    continue;

            is_checked = true;

•           do_gettimeofday(&now);
  

•           e->event.sequence = 0;
  

•           e->event.tv_sec = now.tv_sec;
  

•           e->event.tv_usec = now.tv_usec;
  
  

•           list_move_tail(&e->base.link, &e->base.file_priv-
  

event_list);

•           wake_up_interruptible(&e->base.file_priv->event_wait);
  

•           list_del(&e->base.link);
  

•           drm_send_vblank_event(drm_dev, -1, e);
    }
  
    if (is_checked)
  

-- 1.7.9.5

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi,

On 05/22/2013 01:04 PM, Inki Dae wrote:

From: Rob Clark rob@ti.com

Rebased.

Signed-off-by: Rob Clark rob@ti.com Signed-off-by: Inki Dae inki.dae@samsung.com

---

drivers/gpu/drm/exynos/exynos_drm_crtc.c | 10 ++-------- 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index e8894bc..1e7825a 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -398,7 +398,6 @@ void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int crtc) { struct exynos_drm_private *dev_priv = dev->dev_private; struct drm_pending_vblank_event *e, *t;

• struct timeval now; unsigned long flags;

  DRM_DEBUG_KMS("%s\n", __FILE__);

@@ -411,14 +410,9 @@ void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int crtc) if (crtc != e->pipe) continue;

• do_gettimeofday(&now);
  

• e->event.sequence = 0;
  

• e->event.tv_sec = now.tv_sec;
  

• e->event.tv_usec = now.tv_usec;
  

• list_move_tail(&e->base.link, &e->base.file_priv->event_list);
  

• wake_up_interruptible(&e->base.file_priv->event_wait);
  

  drm_vblank_put(dev, crtc);

• list_del(&e->base.link);
  

• drm_send_vblank_event(dev, -1, e);
  

I think it's better to add above things before drm_vblank_put is called in comparison with prior codes.

Thanks.

From: Rob Clark rob@ti.com

Rebased.

Signed-off-by: Rob Clark rob@ti.com Signed-off-by: Inki Dae inki.dae@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 10 ++-------- 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index e8894bc..c6b2dbc 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -398,7 +398,6 @@ void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int crtc) { struct exynos_drm_private *dev_priv = dev->dev_private; struct drm_pending_vblank_event *e, *t; - struct timeval now; unsigned long flags;

DRM_DEBUG_KMS("%s\n", __FILE__); @@ -411,13 +410,8 @@ void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int crtc) if (crtc != e->pipe) continue;

- do_gettimeofday(&now); - e->event.sequence = 0; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; - - list_move_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); + list_del(&e->base.link); + drm_send_vblank_event(dev, -1, e); drm_vblank_put(dev, crtc); }

Hi Inki,

can you please check if this patch is still necessary, and apply it if so.

Dave.

On Tue, Oct 9, 2012 at 5:50 AM, Rob Clark rob.clark@linaro.org wrote:

From: Rob Clark rob@ti.com

The event wouldn't be on any list at this point, so nothing to delete it from.

Signed-off-by: Rob Clark rob@ti.com

drivers/gpu/drm/exynos/exynos_drm_crtc.c | 1 - 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index abb1e2f..d17419c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -225,7 +225,6 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc, ret = drm_vblank_get(dev, exynos_crtc->pipe); if (ret) { DRM_DEBUG("failed to acquire vblank counter\n");

•                   list_del(&event->base.link);
  
                    goto out;
            }
  

-- 1.7.9.5

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Oct 9, 2012 at 10:33 PM, Mario Kleiner mario.kleiner@tuebingen.mpg.de wrote:

On 08.10.12 21:50, Rob Clark wrote:

...

From: Rob Clark rob@ti.com

Signed-off-by: Rob Clark rob@ti.com

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.

I was actually debating whether or not to bother sending the omap patches, because I'm in middle of a re-write of the kms code in omapdrm that will (among many other things) give us proper vblank accounting..

There are a surprising # of other drivers that are just using do_gettimeofday() + seqn=0.. I guess, at least to be consistent, using seqn=0 is better than the completely bogus seqn. Esp. when you consider having multiple CRTCs, they would end up not even having successive sequence numbers with the existing scheme. But like I said, it's about to be replaced with something sane anyways :-P

BR, -R

-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)

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, 2012-10-09 at 12:35 +0300, Imre Deak wrote:

On Mon, 2012-10-08 at 14:50 -0500, Rob Clark wrote:

...

From: Rob Clark rob@ti.com

Userspace might not request a vblank event. So it is not an error for 'event' to be NULL, and we shouldn't use it to determine if there is a pending flip already.

Signed-off-by: Rob Clark rob@ti.com

drivers/staging/omapdrm/omap_crtc.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c index 74e019a..317b854 100644 --- a/drivers/staging/omapdrm/omap_crtc.c +++ b/drivers/staging/omapdrm/omap_crtc.c @@ -119,7 +119,6 @@ static void vblank_cb(void *arg) struct omap_crtc *omap_crtc = to_omap_crtc(crtc); unsigned long flags;

• WARN_ON(!event); spin_lock_irqsave(&dev->event_lock, flags);

  /* wakeup userspace */

@@ -127,6 +126,7 @@ static void vblank_cb(void *arg) drm_send_vblank_event(dev, -1, omap_crtc->event);

omap_crtc->event = NULL;

• omap_crtc->old_fb = NULL;

Is old_fb used anywhere? If not we could just remove it.

Otherwise nice work! On the series:

Reviewed-by: Imre Deak imre.deak@intel.com

...

spin_unlock_irqrestore(&dev->event_lock, flags); } @@ -138,8 +138,6 @@ static void page_flip_cb(void *arg) struct drm_framebuffer *old_fb = omap_crtc->old_fb; struct drm_gem_object *bo;

• omap_crtc->old_fb = NULL;

• omap_crtc_mode_set_base(crtc, crtc->x, crtc->y, old_fb);

  /* really we'd like to setup the callback atomically w/ setting the

@@ -162,7 +160,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,

DBG("%d -> %d", crtc->fb ? crtc->fb->base.id : -1, fb->base.id);

• if (omap_crtc->event) {

• if (omap_crtc->old_fb) {

Ah, just noticed this adds the use for it :) So ignore my comment above.

--Imre

...

dev_err(dev->dev, "already a pending flip\n");
return -EINVAL;

}