On Mon, 13 Dec 2010 18:21:20 +0100
Mario Kleiner <mario.kleiner@xxxxxxxxxxxxxxxx> 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@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.
Right; thanks for the additional explanation, it looks good to me.
Acked-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
--
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
