[PATCH 00/11] page-flip cleanups and fixes

[PATCH 00/11] page-flip cleanups and fixes

[Rob Clark]

From: Rob Clark <rob@ti.com>

Add a helper fxn to send vblank event after pageflip, plus a handful
of fixes for other issues that I noticed in the process.

Other than OMAP, the changes are just compile tested, so would be
good to get a quick look from the maintainers of various drivers.

Rob Clark (11):
  drm: add drm_send_vblank_event() helper
  drm/i915: use drm_send_vblank_event() helper
  drm/nouveau: use drm_send_vblank_event() helper
  drm/radeon: use drm_send_vblank_event() helper
  drm/exynos: use drm_send_vblank_event() helper
  drm/exynos: page flip fixes
  drm/shmob: use drm_send_vblank_event() helper
  drm/imx: use drm_send_vblank_event() helper
  drm/imx: page flip fixes
  drm/omap: use drm_send_vblank_event() helper
  drm/omap: page-flip fixes

 drivers/gpu/drm/drm_irq.c                 |   65 +++++++++++++++++++----------
 drivers/gpu/drm/exynos/exynos_drm_crtc.c  |    1 -
 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 +---
 drivers/gpu/drm/i915/intel_display.c      |   15 +------
 drivers/gpu/drm/nouveau/nouveau_display.c |   13 +-----
 drivers/gpu/drm/radeon/radeon_display.c   |   13 ++----
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c |   19 ++-------
 drivers/staging/imx-drm/ipuv3-crtc.c      |   32 ++++----------
 drivers/staging/omapdrm/omap_crtc.c       |   37 ++++------------
 include/drm/drmP.h                        |    2 +
 12 files changed, 78 insertions(+), 148 deletions(-)

-- 
1.7.9.5

[PATCH 01/11] drm: add drm_send_vblank_event() helper

[Rob Clark]

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

Re: [PATCH 01/11] drm: add drm_send_vblank_event() helper

[Marcin Slusarz]

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);
> -- 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/11] drm: add drm_send_vblank_event() helper

[Laurent Pinchart]

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.

> + */
> +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().

> +	}
> +	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

Re: [PATCH 01/11] drm: add drm_send_vblank_event() helper

[Rob Clark]

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

Re: [PATCH 01/11] drm: add drm_send_vblank_event() helper

[Mario Kleiner]

On 11.10.12 16:19, Laurent Pinchart wrote:
> Hi Rob,
>
> Thanks for the patch.
>
> On Monday 08 October 2012 14:50:39 Rob Clark wrote:
>> From: Rob Clark <rob@ti.com>
>>

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

At least nouveau could use it. Lucas Stach and me wrote patches for 
nouveau-kms, and they went through many iterations and missed many 
kernel merge windows due to slow review until i think both of us got 
tired of resubmitting with tiny changes. The latest iteration is posted 
by Lucas on nouveau-devel from 26. April 2012. Not sure if they'd still 
apply after the nouveau-kms rewrite. I'll probably give them another try 
once that has landed when i have some spare time.

In principle it's very simple to use drm_vblank_count_and_time(). A 
driver needs to

1. Call drm_handle_vblank() from its vblank irq handler.

2. Make sure that in the vblank of pageflip completion 
drm_handle_vblank() is called before drm_vblank_count_and_time(), so the 
latter picks up updated counts and timestamps.

3. Big bonus for high precision and robustness: Implement the 
driver->get_vblank_timestamp() hook to provide a precise vblank 
timestamp. One simple way to do that is like radeon-kms or intel-kms do 
it: Call back into drm_calc_vbltimestamp_from_scanoutpos() and provide 
the driver->get_scanout_position() function - a function that returns 
the current hardware scanline counter. This is precise down to ~ 10 
microseconds (at least confirmed by measurements on 
intel,radeon,nouveau) and robust against delayed vblank irq handling.

-mario

Re: [PATCH 01/11] drm: add drm_send_vblank_event() helper

[Laurent Pinchart]

Hi Mario,

On Saturday 13 October 2012 02:28:03 Mario Kleiner wrote:
> On 11.10.12 16:19, Laurent Pinchart wrote:
> > On Monday 08 October 2012 14:50:39 Rob Clark wrote:
> >> From: Rob Clark <rob@ti.com>
> 
> ...
> 
> > 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().
> 
> At least nouveau could use it. Lucas Stach and me wrote patches for
> nouveau-kms, and they went through many iterations and missed many
> kernel merge windows due to slow review until i think both of us got
> tired of resubmitting with tiny changes.

I totally understand the feeling, but please don't give up. You can CC me on 
the next iteration, I'll make sure to review the patches (even though I have 
no experience with the nouveau driver).

> The latest iteration is posted by Lucas on nouveau-devel from 26. April
> 2012. Not sure if they'd still apply after the nouveau-kms rewrite. I'll
> probably give them another try once that has landed when i have some spare
> time.
> 
> In principle it's very simple to use drm_vblank_count_and_time(). A
> driver needs to
> 
> 1. Call drm_handle_vblank() from its vblank irq handler.
> 
> 2. Make sure that in the vblank of pageflip completion
> drm_handle_vblank() is called before drm_vblank_count_and_time(), so the
> latter picks up updated counts and timestamps.
> 
> 3. Big bonus for high precision and robustness: Implement the
> driver->get_vblank_timestamp() hook to provide a precise vblank
> timestamp. One simple way to do that is like radeon-kms or intel-kms do
> it: Call back into drm_calc_vbltimestamp_from_scanoutpos() and provide
> the driver->get_scanout_position() function - a function that returns
> the current hardware scanline counter. This is precise down to ~ 10
> microseconds (at least confirmed by measurements on
> intel,radeon,nouveau) and robust against delayed vblank irq handling.
> 
> -mario
-- 
Regards,

Laurent Pinchart

[PATCH 02/11] drm/i915: use drm_send_vblank_event() helper

[Rob Clark]

From: Rob Clark <rob@ti.com>

Signed-off-by: Rob Clark <rob@ti.com>
---
 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

Re: [PATCH 02/11] drm/i915: use drm_send_vblank_event() helper

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 02/11] drm/i915: use drm_send_vblank_event() helper

[Daniel Vetter]

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>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 03/11] drm/nouveau: use drm_send_vblank_event() helper

[Rob Clark]

From: Rob Clark <rob@ti.com>

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/gpu/drm/nouveau/nouveau_display.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 7e16dc5..b1a32b9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -566,17 +566,8 @@ nouveau_finish_page_flip(struct nouveau_channel *chan,
 	}
 
 	s = list_first_entry(&swch->flip, struct nouveau_page_flip_state, head);
-	if (s->event) {
-		struct drm_pending_vblank_event *e = s->event;
-		struct timeval now;
-
-		do_gettimeofday(&now);
-		e->event.sequence = 0;
-		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);
-	}
+	if (s->event)
+		drm_send_vblank_event(dev, -1, s->event);
 
 	list_del(&s->head);
 	if (ps)
-- 
1.7.9.5

[PATCH 04/11] drm/radeon: use drm_send_vblank_event() helper

[Rob Clark]

From: Rob Clark <rob@ti.com>

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/gpu/drm/radeon/radeon_display.c |   13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 7ddef8f..8dad9c2 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -271,8 +271,6 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 {
 	struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
 	struct radeon_unpin_work *work;
-	struct drm_pending_vblank_event *e;
-	struct timeval now;
 	unsigned long flags;
 	u32 update_pending;
 	int vpos, hpos;
@@ -328,14 +326,9 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 	radeon_crtc->unpin_work = NULL;
 
 	/* wakeup userspace */
-	if (work->event) {
-		e = work->event;
-		e->event.sequence = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
-		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);
-	}
+	if (work->event)
+		drm_send_vblank_event(rdev->ddev, crtc_id, work->event);
+
 	spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
 
 	drm_vblank_put(rdev->ddev, radeon_crtc->crtc_id);
-- 
1.7.9.5

[PATCH 05/11] drm/exynos: use drm_send_vblank_event() helper

[Rob Clark]

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

Re: [PATCH 05/11] drm/exynos: use drm_send_vblank_event() helper

[Dave Airlie]

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

RE: [PATCH 05/11] drm/exynos: use drm_send_vblank_event() helper

[Inki Dae]

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

[PATCH] drm/exynos: use drm_send_vblank_event() helper

[Inki Dae]

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);
 	}
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
-- 
1.7.5.4

Re: [PATCH] drm/exynos: use drm_send_vblank_event() helper

[Joonyoung Shim]

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.

Re: [PATCH] drm/exynos: use drm_send_vblank_event() helper

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 2072 bytes --]

2013/5/22 Joonyoung Shim <jy0922.shim@samsung.com>

> 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.
>
>
Good point. :) Will resend it.

Thanks,
Inki Dae


> Thanks.
>
> ______________________________**_________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.**org <dri-devel@lists.freedesktop.org>
> http://lists.freedesktop.org/**mailman/listinfo/dri-devel<http://lists.freedesktop.org/mailman/listinfo/dri-devel>
>

[-- Attachment #1.2: Type: text/html, Size: 3217 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH RESEND] drm/exynos: use drm_send_vblank_event() helper

[Inki Dae]

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

[PATCH 06/11] drm/exynos: page flip fixes

[Rob Clark]

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

Re: [PATCH 06/11] drm/exynos: page flip fixes

[Dave Airlie]

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

RE: [PATCH 06/11] drm/exynos: page flip fixes

[Inki Dae]

> -----Original Message-----
> From: Dave Airlie [mailto:airlied@gmail.com]
> Sent: Wednesday, May 22, 2013 8:17 AM
> To: Inki Dae
> Cc: dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH 06/11] drm/exynos: page flip fixes
> 
> Hi Inki,
> 
> can you please check if this patch is still necessary, and apply it if so.
> 

I missed it. Applied. And exynos-drm-fixes has one patch being reviewed yet
so we will request pull after reviewed.

Thanks,
Inki Dae

> 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

[PATCH 07/11] drm/shmob: use drm_send_vblank_event() helper

[Rob Clark]

From: Rob Clark <rob@ti.com>

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c |   19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
index 0e7a930..3ecb702 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
@@ -451,27 +451,16 @@ void shmob_drm_crtc_finish_page_flip(struct shmob_drm_crtc *scrtc)
 {
 	struct drm_pending_vblank_event *event;
 	struct drm_device *dev = scrtc->crtc.dev;
-	struct timeval vblanktime;
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev->event_lock, flags);
 	event = scrtc->event;
 	scrtc->event = NULL;
+	if (event) {
+		drm_send_vblank_event(dev, 0, event);
+		drm_vblank_put(dev, 0);
+	}
 	spin_unlock_irqrestore(&dev->event_lock, flags);
-
-	if (event == NULL)
-		return;
-
-	event->event.sequence = drm_vblank_count_and_time(dev, 0, &vblanktime);
-	event->event.tv_sec = vblanktime.tv_sec;
-	event->event.tv_usec = vblanktime.tv_usec;
-
-	spin_lock_irqsave(&dev->event_lock, flags);
-	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);
-
-	drm_vblank_put(dev, 0);
 }
 
 static int shmob_drm_crtc_page_flip(struct drm_crtc *crtc,
-- 
1.7.9.5

[PATCH 08/11] drm/imx: use drm_send_vblank_event() helper

[Rob Clark]

From: Rob Clark <rob@ti.com>

Also, slightly changes the behavior to always put the vblank irq,
even if userspace did not request a vblank event.  As far as I
can tell, the previous code would leak a vblank irq refcnt if
userspace requested a pageflip without event.

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/staging/imx-drm/ipuv3-crtc.c |   21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c
index 78d3eda..8fa0f4d 100644
--- a/drivers/staging/imx-drm/ipuv3-crtc.c
+++ b/drivers/staging/imx-drm/ipuv3-crtc.c
@@ -311,31 +311,14 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc,
 
 static void ipu_crtc_handle_pageflip(struct ipu_crtc *ipu_crtc)
 {
-	struct drm_pending_vblank_event *e;
-	struct timeval now;
 	unsigned long flags;
 	struct drm_device *drm = ipu_crtc->base.dev;
 
 	spin_lock_irqsave(&drm->event_lock, flags);
-
-	e = ipu_crtc->page_flip_event;
-	if (!e) {
-		spin_unlock_irqrestore(&drm->event_lock, flags);
-		return;
-	}
-
-	do_gettimeofday(&now);
-	e->event.sequence = 0;
-	e->event.tv_sec = now.tv_sec;
-	e->event.tv_usec = now.tv_usec;
+	if (ipu_crtc->page_flip_event)
+		drm_send_vblank_event(drm, -1, ipu_crtc->page_flip_event);
 	ipu_crtc->page_flip_event = NULL;
-
 	imx_drm_crtc_vblank_put(ipu_crtc->imx_crtc);
-
-	list_add_tail(&e->base.link, &e->base.file_priv->event_list);
-
-	wake_up_interruptible(&e->base.file_priv->event_wait);
-
 	spin_unlock_irqrestore(&drm->event_lock, flags);
 }
 
-- 
1.7.9.5

[PATCH 09/11] drm/imx: page flip fixes

[Rob Clark]

From: Rob Clark <rob@ti.com>

The 'event' could be null, so don't list_del(&event->base.link)..  and
in fact even if it wasn't null there is no reason to do this.

Also, this looks racy with the irq handler, so throw in a spinlock for
good measure.

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/staging/imx-drm/ipuv3-crtc.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c
index 8fa0f4d..6745766 100644
--- a/drivers/staging/imx-drm/ipuv3-crtc.c
+++ b/drivers/staging/imx-drm/ipuv3-crtc.c
@@ -135,23 +135,26 @@ static int ipu_page_flip(struct drm_crtc *crtc,
 		struct drm_pending_vblank_event *event)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
+	struct drm_device *drm = ipu_crtc->base.dev;
+	unsigned long flags;
 	int ret;
 
 	if (ipu_crtc->newfb)
 		return -EBUSY;
 
+	spin_lock_irqsave(&drm->event_lock, flags);
 	ret = imx_drm_crtc_vblank_get(ipu_crtc->imx_crtc);
 	if (ret) {
 		dev_dbg(ipu_crtc->dev, "failed to acquire vblank counter\n");
-		list_del(&event->base.link);
-
-		return ret;
+		goto out;
 	}
 
 	ipu_crtc->newfb = fb;
 	ipu_crtc->page_flip_event = event;
 
-	return 0;
+out:
+	spin_unlock_irqrestore(&drm->event_lock, flags);
+	return ret;
 }
 
 static const struct drm_crtc_funcs ipu_crtc_funcs = {
-- 
1.7.9.5

Re: [PATCH 09/11] drm/imx: page flip fixes

[Dave Airlie]

Hi Sascha,

please review and apply if still applicable, I've already applied the
imx use drm_send_vblank_event to my drm-fixes tree.

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' could be null, so don't list_del(&event->base.link)..  and
> in fact even if it wasn't null there is no reason to do this.
>
> Also, this looks racy with the irq handler, so throw in a spinlock for
> good measure.
>
> Signed-off-by: Rob Clark <rob@ti.com>
> ---
>  drivers/staging/imx-drm/ipuv3-crtc.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c
> index 8fa0f4d..6745766 100644
> --- a/drivers/staging/imx-drm/ipuv3-crtc.c
> +++ b/drivers/staging/imx-drm/ipuv3-crtc.c
> @@ -135,23 +135,26 @@ static int ipu_page_flip(struct drm_crtc *crtc,
>                 struct drm_pending_vblank_event *event)
>  {
>         struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
> +       struct drm_device *drm = ipu_crtc->base.dev;
> +       unsigned long flags;
>         int ret;
>
>         if (ipu_crtc->newfb)
>                 return -EBUSY;
>
> +       spin_lock_irqsave(&drm->event_lock, flags);
>         ret = imx_drm_crtc_vblank_get(ipu_crtc->imx_crtc);
>         if (ret) {
>                 dev_dbg(ipu_crtc->dev, "failed to acquire vblank counter\n");
> -               list_del(&event->base.link);
> -
> -               return ret;
> +               goto out;
>         }
>
>         ipu_crtc->newfb = fb;
>         ipu_crtc->page_flip_event = event;
>
> -       return 0;
> +out:
> +       spin_unlock_irqrestore(&drm->event_lock, flags);
> +       return ret;
>  }
>
>  static const struct drm_crtc_funcs ipu_crtc_funcs = {
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/11] drm/omap: use drm_send_vblank_event() helper

[Rob Clark]

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);
-		 */
-		event->event.sequence = sequence++;
-		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)
-- 
1.7.9.5

Re: [PATCH 10/11] drm/omap: use drm_send_vblank_event() helper

[Mario Kleiner]

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.

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

Re: [PATCH 10/11] drm/omap: use drm_send_vblank_event() helper

[Rob Clark]

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

Re: [PATCH 10/11] drm/omap: use drm_send_vblank_event() helper

[Mario Kleiner]

On 10.10.12 13:03, Rob Clark wrote:
> 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..
>

Cool, that makes me happy :)

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

Yes, zero is better in the meantime. E.g., to cope with nouveau's 
deficiency there, my app takes zero as "vblank count unsupported" and 
uses a fallback path, instead of getting confused.

Thanks for this nice cleanup.
-mario

[PATCH 11/11] drm/omap: page-flip fixes

[Rob Clark]

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;
 
 	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) {
 		dev_err(dev->dev, "already a pending flip\n");
 		return -EINVAL;
 	}
-- 
1.7.9.5

Re: [PATCH 11/11] drm/omap: page-flip fixes

[Imre Deak]

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) {
>  		dev_err(dev->dev, "already a pending flip\n");
>  		return -EINVAL;
>  	}

Re: [PATCH 11/11] drm/omap: page-flip fixes

[Imre Deak]

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

Re: [PATCH 00/11] page-flip cleanups and fixes

[Alex Deucher]

On Mon, Oct 8, 2012 at 3:50 PM, Rob Clark <rob.clark@linaro.org> wrote:
> From: Rob Clark <rob@ti.com>
>
> Add a helper fxn to send vblank event after pageflip, plus a handful
> of fixes for other issues that I noticed in the process.

Looks like a nice clean-up to me.  Patches 1 and 4 are:

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Other than OMAP, the changes are just compile tested, so would be
> good to get a quick look from the maintainers of various drivers.
>
> Rob Clark (11):
>   drm: add drm_send_vblank_event() helper
>   drm/i915: use drm_send_vblank_event() helper
>   drm/nouveau: use drm_send_vblank_event() helper
>   drm/radeon: use drm_send_vblank_event() helper
>   drm/exynos: use drm_send_vblank_event() helper
>   drm/exynos: page flip fixes
>   drm/shmob: use drm_send_vblank_event() helper
>   drm/imx: use drm_send_vblank_event() helper
>   drm/imx: page flip fixes
>   drm/omap: use drm_send_vblank_event() helper
>   drm/omap: page-flip fixes
>
>  drivers/gpu/drm/drm_irq.c                 |   65 +++++++++++++++++++----------
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c  |    1 -
>  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 +---
>  drivers/gpu/drm/i915/intel_display.c      |   15 +------
>  drivers/gpu/drm/nouveau/nouveau_display.c |   13 +-----
>  drivers/gpu/drm/radeon/radeon_display.c   |   13 ++----
>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |   19 ++-------
>  drivers/staging/imx-drm/ipuv3-crtc.c      |   32 ++++----------
>  drivers/staging/omapdrm/omap_crtc.c       |   37 ++++------------
>  include/drm/drmP.h                        |    2 +
>  12 files changed, 78 insertions(+), 148 deletions(-)
>
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/11] page-flip cleanups and fixes

[Marcin Slusarz]

On Mon, Oct 08, 2012 at 02:50:38PM -0500, Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
> 
> Add a helper fxn to send vblank event after pageflip, plus a handful
> of fixes for other issues that I noticed in the process.

FWIW, patches 1-5 and 7 (with a fix to 1st) are:
Reviewed-by: Marcin Slusarz <marcin.slusarz@gmail.com>

> 
> Other than OMAP, the changes are just compile tested, so would be
> good to get a quick look from the maintainers of various drivers.
> 
> Rob Clark (11):
>   drm: add drm_send_vblank_event() helper
>   drm/i915: use drm_send_vblank_event() helper
>   drm/nouveau: use drm_send_vblank_event() helper
>   drm/radeon: use drm_send_vblank_event() helper
>   drm/exynos: use drm_send_vblank_event() helper
>   drm/exynos: page flip fixes
>   drm/shmob: use drm_send_vblank_event() helper
>   drm/imx: use drm_send_vblank_event() helper
>   drm/imx: page flip fixes
>   drm/omap: use drm_send_vblank_event() helper
>   drm/omap: page-flip fixes
> 
>  drivers/gpu/drm/drm_irq.c                 |   65 +++++++++++++++++++----------
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c  |    1 -
>  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 +---
>  drivers/gpu/drm/i915/intel_display.c      |   15 +------
>  drivers/gpu/drm/nouveau/nouveau_display.c |   13 +-----
>  drivers/gpu/drm/radeon/radeon_display.c   |   13 ++----
>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |   19 ++-------
>  drivers/staging/imx-drm/ipuv3-crtc.c      |   32 ++++----------
>  drivers/staging/omapdrm/omap_crtc.c       |   37 ++++------------
>  include/drm/drmP.h                        |    2 +
>  12 files changed, 78 insertions(+), 148 deletions(-)
> 
> -- 

Re: [PATCH 00/11] page-flip cleanups and fixes

[Inki Dae]

2012/10/9 Rob Clark <rob.clark@linaro.org>:
> From: Rob Clark <rob@ti.com>
>
> Add a helper fxn to send vblank event after pageflip, plus a handful
> of fixes for other issues that I noticed in the process.
>

patches 1, 5 and 6  are:
Reviewed-by: Inki Dae <inki.dae@samsung.com>

Re: [PATCH 00/11] page-flip cleanups and fixes

[Greg KH]

On Mon, Oct 08, 2012 at 02:50:38PM -0500, Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
> 
> Add a helper fxn to send vblank event after pageflip, plus a handful
> of fixes for other issues that I noticed in the process.
> 
> Other than OMAP, the changes are just compile tested, so would be
> good to get a quick look from the maintainers of various drivers.
> 
> Rob Clark (11):
>   drm: add drm_send_vblank_event() helper
>   drm/i915: use drm_send_vblank_event() helper
>   drm/nouveau: use drm_send_vblank_event() helper
>   drm/radeon: use drm_send_vblank_event() helper
>   drm/exynos: use drm_send_vblank_event() helper
>   drm/exynos: page flip fixes
>   drm/shmob: use drm_send_vblank_event() helper
>   drm/imx: use drm_send_vblank_event() helper
>   drm/imx: page flip fixes
>   drm/omap: use drm_send_vblank_event() helper
>   drm/omap: page-flip fixes

Are these patches going through the drm tree?  I'd recommend doing that,
and feel free to add:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

by any drivers/staging/ patches.

thanks,

greg k-h

Re: [PATCH 00/11] page-flip cleanups and fixes

[Rob Clark]

On 10/22/2012 05:39 PM, Greg KH wrote:
> On Mon, Oct 08, 2012 at 02:50:38PM -0500, Rob Clark wrote:
>> From: Rob Clark <rob@ti.com>
>>
>> Add a helper fxn to send vblank event after pageflip, plus a handful
>> of fixes for other issues that I noticed in the process.
>>
>> Other than OMAP, the changes are just compile tested, so would be
>> good to get a quick look from the maintainers of various drivers.
>>
>> Rob Clark (11):
>>    drm: add drm_send_vblank_event() helper
>>    drm/i915: use drm_send_vblank_event() helper
>>    drm/nouveau: use drm_send_vblank_event() helper
>>    drm/radeon: use drm_send_vblank_event() helper
>>    drm/exynos: use drm_send_vblank_event() helper
>>    drm/exynos: page flip fixes
>>    drm/shmob: use drm_send_vblank_event() helper
>>    drm/imx: use drm_send_vblank_event() helper
>>    drm/imx: page flip fixes
>>    drm/omap: use drm_send_vblank_event() helper
>>    drm/omap: page-flip fixes
> Are these patches going through the drm tree?  I'd recommend doing that,

This is what I'd prefer, if it is ok with Dave

> and feel free to add:
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> by any drivers/staging/ patches.

thanks

BR,
-R


> thanks,
>
> greg k-h