On Dec 10, 2010, at 6:45 PM, Jesse Barnes wrote:
On Fri, 10 Dec 2010 16:00:19 +0100
Mario Kleiner <mario.kleiner@xxxxxxxxxxxxxxxx> wrote:
-------- Original Message --------
Subject: [PATCH] drm-vblank: Always return true vblank count of
scheduled vblank event.
Date: Fri, 10 Dec 2010 15:58:10 +0100
From: Mario Kleiner <mario.kleiner@xxxxxxxxxxxxxxxx>
To: airlied@xxxxxxxxx
CC: jbarnes@xxxxxxxxxxxxxxxx, krh@xxxxxxxxxxxxx, Mario Kleiner
<mario.kleiner@xxxxxxxxxxxxxxxx>
This patch tries to make sure that the vbl.reply.sequence
vblank count for a queued or emitted vblank event always
corresponds to the true vblank count of queueing/emission, so
the ddx can rely on the returned target_msc for consistency
checks and implementation of swap_intervals in glXSwapBuffers().
Without this there is a small race-condition between the
userspace ddx queueing a vblank event and the vblank
counter incrementing before the event gets queued in
the kernel.
Signed-off-by: Mario Kleiner <mario.kleiner@xxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/drm_irq.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 4e82d0d..55160d7 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1086,15 +1086,18 @@ 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, e->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);
But this actually causes us to return the current count rather than
the
requested count if the event requested is in the past, right?
Yes. But i think that's what the spec wants. The wording in the spec
is usually "...and then will return the current values for UST, MSC,
and SBC...". If it just returned what was passed to it, it
wouldn't provide any new information to the calling client.
Currently there are three uses for queuing of vblank events:
a) Copy-Swaps: Queue a vblank event for target_msc to trigger a blit
once the event is received. Because we set the
DRM_VBLANK_NEXT_ON_MISS flag when queuing this event, the requested
target_msc is automatically adapted by the drm to current_msc + 1 and
this new adapted value is returned to the ddx. So for copy swaps the
kernel returns the true expected count and time of the swap. The
above path isn't ever taken.
b) Pageflip-Swaps: Queue a vblank event for target_msc - 1, even if
target_msc -1 is already over. This will deliver the event
immediately if current_msc >= target_msc - 1, but without the patch
it would deliver it with a vbl.reply.sequence that has nothing to do
with the real count at swap.
So a) and b) with the old code don't have a negative effect on
scheduling a swap, but b) can have a a negative effect for the
scheduling of the next pageflipped swap: The ddx uses the returned
value as last_swap_target to implement swap_interval correctly, so
the next swap may not obey the swap_interval correctly if case b)
delivered an outdated vbl.reply.sequence number.
c) WaitForMscOML: Like b), but here it would return the "wrong" count
for a target_msc in the past. The client would get the information
back that it passed into the call, instead of the information that
reflects reality.
d) Some correctness check for pageflipping in the ddx that can work
more reliable / detect more possible errors if the returned value is
always precise.
} else {
list_add_tail(&e->base.link, &dev->vblank_event_list);
+ vblwait->reply.sequence = vblwait->request.sequence;
}
This one makes sense; we want to make sure the returned sequence is
the
one requested (assuming it was in the future at the time of the
request
anyway).
Yes, but the requested number gets adapted by the function if
DRM_VBLANK_NEXT_ON_MISS is set. Also this statement is a bit
redundant, because vblwait is a union iirc, so vblwait-
>reply.sequence and vblwait->request.sequence are actually the same
field. It's just added for clarity.
-mario
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
