[PATCH] DRI2/xserver: Don't hang in glXSwapBuffers if drawable moves between crtc's

[Mario Kleiner]

Hi Keith and Peter,

this patch has been reviewed by Jesse Barnes and now successfully  
tested by the guy who submitted the bug report and by me. Fixes  
Bugzilla bug #28383, a quite ugly hang of application windows  
bufferswapping, including crashes when moving them between different  
crtc's of different refresh rate.

It would be good to apply it to 1.9 and 1.8.2 if possible, for a more  
enjoyable multi-display OpenGL experience.

thanks,
-mario

On Jun 14, 2010, at 6:13 PM, Jesse Barnes wrote:

> On Sun, 13 Jun 2010 18:05:26 +0200
> Mario Kleiner <mario.kleiner at tuebingen.mpg.de> wrote:
>
>> Detect if a drawable has been moved from an original crtc to a new  
>> crtc
>> with a lower current vblank count than the original crtc inbetween
>> glXSwapBuffers() calls. Reinitialize drawable's last_swap_target
>> before scheduling next swap if such a move has taken place.
>>
>> last_swap_target defines the baseline for scheduling the next swap.
>> If a movement between crtc's is not taken into account, the swap may
>> schedule for a vblank count on the new crtc far in the future,  
>> resulting
>> in a apparent "hang" of the drawable for a long time.
>>
>> Fixes Bugzilla bug #28383.
>>
>> Signed-off-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
>> ---
>>  hw/xfree86/dri2/dri2.c |   15 +++++++++++++++
>>  1 files changed, 15 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
>> index d33b0d1..1f80022 100644
>> --- a/hw/xfree86/dri2/dri2.c
>> +++ b/hw/xfree86/dri2/dri2.c
>> @@ -756,6 +756,7 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr  
>> pDraw, CARD64 target_msc,
>>      DRI2DrawablePtr pPriv;
>>      DRI2BufferPtr   pDestBuffer = NULL, pSrcBuffer = NULL;
>>      int             ret, i;
>> +    CARD64          ust, current_msc;
>>
>>      pPriv = DRI2GetDrawable(pDraw);
>>      if (pPriv == NULL) {
>> @@ -800,12 +801,26 @@ DRI2SwapBuffers(ClientPtr client,  
>> DrawablePtr pDraw, CARD64 target_msc,
>>       * need to schedule a swap for the last swap target + the  
>> swap interval.
>>       */
>>      if (target_msc == 0 && divisor == 0 && remainder == 0) {
>> +	/* If the current vblank count of the drawable's crtc is lower
>> +	 * than the count stored in last_swap_target from a previous swap
>> +	 * then reinitialize last_swap_target to the current crtc's msc,
>> +	 * otherwise the swap will hang. This will happen if the drawable
>> +	 * is moved to a crtc with a lower refresh rate, or a crtc that  
>> just
>> +	 * got enabled.
>> +	 */
>> +	if (!(*ds->GetMSC)(pDraw, &ust, &current_msc))
>> +	    pPriv->last_swap_target = 0;
>> +
>> +	if (current_msc < pPriv->last_swap_target)
>> +	    pPriv->last_swap_target = current_msc;
>> +
>>  	/*
>>  	 * Swap target for this swap is last swap target + swap interval  
>> since
>>  	 * we have to account for the current swap count, interval, and the
>>  	 * number of pending swaps.
>>  	 */
>>  	*swap_target = pPriv->last_swap_target + pPriv->swap_interval;
>> +
>>      } else {
>>  	/* glXSwapBuffersMscOML could have a 0 target_msc, honor it */
>>  	*swap_target = target_msc;
>
> Yeah, this looks ok.  Really we should write up GLX extension that
> clarifies behavior in multi-head and display on/off situations  
> too.  Is
> that something you could do?
>
> Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
>
> -- 
> Jesse Barnes, Intel Open Source Technology Center