[PATCH] DRI2: fixup handling of last_swap_target

[Mario Kleiner]

On Mar 5, 2010, at 6:50 PM, Jesse Barnes wrote:

>
> Ok pushed fixes for these issues to the repo above.  I know you've put
> a lot of time into this already, but can you take another look to make
> sure I got everything?

No problem.

>   Note I left the swap interval default at 1
> since GLX_MESA_swap_control should let us set it to 0 to get
> unthrottled behavior, which I just fixed and tested.


Agreed, that's ok. GLX_MESA_swap_control is a way to do it.

The current state looks almost good, esp. your fixes to  
DRI2SwapBuffers for swap_interval == 0 and for target_msc == 0.

But a few more things:

In DRI2SwapBuffers, in the body of ...

     /* Old DDX or no swap interval, just blit */
     if (!ds->ScheduleSwap || !pPriv->swap_interval) {
	...

... it calls DRI2SwapComplete and passes target_msc as the 'msc'  
value of swap completion. Would be probably better to pass a zero  
value, just as it is done in the Intel DDX when the swap doesn't  
happen at all at the requested target_msc, but rather immediately due  
to some problems. If ds->ScheduleSwap isn't available then the 'msc'  
is basically undefined and unknown, so zero is a better return value.  
In the !pPriv->swap_interval case one could call a *ds->getMSC() to  
get a more meaningful return value for 'msc'. Don't know if it's  
worth the effort, but a zero return would be better than 'target_msc'  
imho.


We need to add another patch to Mesa. Mesa translates a glXSwapBuffers 
() call into a DRI2SwapBuffers() call with target_msc, divisor and  
remainder all zero and DRI2SwapBuffers takes this as hint that  
glXSwapBuffers behaviour is requested. However these are also legal  
arguments to a glXSwapBuffersMscOML(..., ,0,0,0) call, which has to  
behave differently than a glXSwapBuffers call  --> In this case,  
DRI2SwapBuffers would confuse this with a glXSwapBuffers request and  
do the wrong thing.

I'd propose adding this line to the __glXSwapBuffersMscOML() routine  
in mesa's /src/glx/x11/glxcmds.c file:

if (target_msc == 0 && divisor == 0 && remainder == 0) remainder = 1;

-> if target_msc and divisor are zero, then the remainder is ignored  
inside the DRI implementation, so the actual value of remainder  
doesn't matter for swap behaviour, but can be used to disambiguate  
glXSwapBuffers vs. glXSwapBuffersMsc...  for a glXSwapBuffersMscOML 
(0,0,0) call.


Your deferred pPriv deletion logic looks now good to me. But if  
deletion is deferred then i think almost all "public" functions  
inside dri2.c should return 'BadDrawable' not only for pPriv == NULL  
but also for pPriv->refCount == 0, e.g., DRI2SwapBuffers,  
DRI2GetMSC, ... From the viewpoint of client code, a refCount == 0 is  
the same as a destroyed drawable. For all practical purposes it is  
already gone. Maybe it would make sense to consolidate the (pPriv ==  
NULL || pPriv->refCount == 0) check into a macro or function,  
something like DRI2IsDrawableAlive(pPriv); and then call that in most  
DRI2xxx functions?


I believe there's also another problem with the pPriv->blockedClient  
logic. The implementation only keeps track of blockedClient, but not  
for the reason why the client blocked. This can cause trouble in  
DRI2WakeClient. Consider this example which i think would cause the  
client to hang:

1. Assume current msc is 1000 and there are 2 threads in the client.

// Thread 1: Request swap at target_msc 1010:
2. glXSwapBuffersMscOML(dpy, drawable, 1010, 0, 0);

// Thread 2: Request wait until target_msc 2000:
3. glXWaitForMscOML(dpu, drawable, 2000, 0, 0, ....);

4. Thread 1: Calls XDestroyWindow(dpy, drawable);

Step 2 will schedule a swap for msc = 1010.
Step 3 will pPriv->blockedClient = client and IgnoreClient(client)   
the client and schedule a wakeup at msc >= 2000.
The xconnection dpy is now blocked.

Step 4 doesn't execute yet, because the xconnection is blocked.

Now at msc = 1010 -> swap completes -> DRI2SwapComplete ->  
DRI2WakeClient -> executes the elseif branch:

...
     } else if (pPriv->target_sbc == -1) {
         if (pPriv->blockedClient)
             AttendClient(pPriv->blockedClient);
         pPriv->blockedClient = NULL;
...

-> this will AttendClient() the client and release pPriv- 
 >blockedClient, i.e., release at msc 1010 although the block was  
meant to be released at msc 2000. Thread 2 still waits for a _XReply  
before it can return from the glXWaitForMscOML...

Now step 4 can execute due to the "unfrozen" xconnection dpy.  
DRI2DestroyDrawable() executes and DRI2FreeDrawable() gets called  
becuase there are no pending flips or blockedClients anymore. pPriv  
is gone.

At msc = 2000, DRI2WaitMSCComplete() gets called due to the received  
vblank event. It finds that pPriv == NULL and returns immediately.

-> ProcDRI2WaitMSCReply() doesn't ever get called and thread 2 hangs  
(forever?) waiting for a _XReply.


In short, i think the implementation should keep track of why a  
client is blocked in pPriv->blockedClient and only unblock it for the  
reason it was blocked, otherwise there are a race conditions.


thanks,
-mario

*********************************************************************
Mario Kleiner
Max Planck Institute for Biological Cybernetics
Spemannstr. 38
72076 Tuebingen
Germany

e-mail: mario.kleiner at tuebingen.mpg.de
office: +49 (0)7071/601-1623
fax:    +49 (0)7071/601-616
www:    http://www.kyb.tuebingen.mpg.de/~kleinerm
*********************************************************************
"For a successful technology, reality must take precedence
over public relations, for Nature cannot be fooled."
(Richard Feynman)