[PATCH] drm-vblank: Always return true vblank count of scheduled vblank event.

[Jesse Barnes]

On Mon, 13 Dec 2010 18:21:20 +0100
Mario Kleiner <mario.kleiner at 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 at 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 at tuebingen.mpg.de>
> >> To: airlied at gmail.com
> >> CC: jbarnes at virtuousgeek.org,	krh at bitplanet.net,	Mario Kleiner
> >> <mario.kleiner at 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 at 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);
> >
> > 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 at virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center