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: [email protected]
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)
_______________________________________________
xorg-devel mailing list
[email protected]
http://lists.x.org/mailman/listinfo/xorg-devel
