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

[Mario Kleiner]

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.

-mario