On Thu, Oct 11, 2012 at 9:19 AM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hi Rob,
>
> Thanks for the patch.
>
> On Monday 08 October 2012 14:50:39 Rob Clark wrote:
>> From: Rob Clark <rob@xxxxxx>
>>
>> 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@xxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
