-------- 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@tuebingen.mpg.de To: airlied@gmail.com CC: jbarnes@virtuousgeek.org, krh@bitplanet.net, Mario Kleiner mario.kleiner@tuebingen.mpg.de
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@tuebingen.mpg.de --- 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); } else { list_add_tail(&e->base.link, &dev->vblank_event_list); + vblwait->reply.sequence = vblwait->request.sequence; }
spin_unlock_irqrestore(&dev->event_lock, flags);
On Fri, 10 Dec 2010 16:00:19 +0100 Mario Kleiner mario.kleiner@tuebingen.mpg.de 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@tuebingen.mpg.de To: airlied@gmail.com CC: jbarnes@virtuousgeek.org, krh@bitplanet.net, Mario Kleiner mario.kleiner@tuebingen.mpg.de
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@tuebingen.mpg.de
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;vblwait->reply.sequence = seq;
But this actually causes us to return the current count rather than the requested count if the event requested is in the past, right?
} 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).
On Dec 10, 2010, at 6:45 PM, Jesse Barnes wrote:
On Fri, 10 Dec 2010 16:00:19 +0100 Mario Kleiner mario.kleiner@tuebingen.mpg.de 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@tuebingen.mpg.de To: airlied@gmail.com CC: jbarnes@virtuousgeek.org, krh@bitplanet.net, Mario Kleiner mario.kleiner@tuebingen.mpg.de
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@tuebingen.mpg.de
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;vblwait->reply.sequence = seq;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
On Mon, 13 Dec 2010 18:21:20 +0100 Mario Kleiner mario.kleiner@tuebingen.mpg.de wrote:
On Dec 10, 2010, at 6:45 PM, Jesse Barnes wrote:
On Fri, 10 Dec 2010 16:00:19 +0100 Mario Kleiner mario.kleiner@tuebingen.mpg.de 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@tuebingen.mpg.de To: airlied@gmail.com CC: jbarnes@virtuousgeek.org, krh@bitplanet.net, Mario Kleiner mario.kleiner@tuebingen.mpg.de
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@tuebingen.mpg.de
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;vblwait->reply.sequence = seq;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.
Right; thanks for the additional explanation, it looks good to me.
Acked-by: Jesse Barnes jbarnes@virtuousgeek.org
dri-devel@lists.freedesktop.org
-
Jesse Barnes -
Mario Kleiner