diff mbox

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

Message ID 1349725849-22433-2-git-send-email-rob.clark@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Rob Clark Oct. 8, 2012, 7:50 p.m. UTC
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(-)

Comments

Marcin Ślusarz Oct. 8, 2012, 9:26 p.m. UTC | #1
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);
> --
Laurent Pinchart Oct. 11, 2012, 2:19 p.m. UTC | #2
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);
Rob Clark Oct. 11, 2012, 2:43 p.m. UTC | #3
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
Mario Kleiner Oct. 13, 2012, 12:28 a.m. UTC | #4
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
Laurent Pinchart Oct. 16, 2012, 12:53 p.m. UTC | #5
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
diff mbox

Patch

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