On Mar 4, 2010, at 7:45 PM, Jesse Barnes wrote:
I just pushed everything into my personal xserver tree, can you check
them out and make sure I got your patches right?
git://people.freedesktop.org/~jbarnes/xserver
--
Jesse Barnes, Intel Open Source Technology Center
Ok Jesse, i checked them. Our patches are happy with each other as
far as i can see :-) -- thanks for cleaning this up.
I went over the whole hw/xfree86/dri2/dri2.c file again and found a
couple of new bugs and problems. Don't have the time for preparing
patches myself now, so i'll just list them, from severe to harmless:
* In DRI2CreateDrawable(), in the new last_swap_target init code, you
call...
ret = (*ds->GetMSC)(pDraw, &ust, &pPriv->last_swap_target);
... without checking for if (!ds->GetMSC) ...
If this happens on an old DDX / non-Intel DDX, GetMSC == NULL -->
Crash! This wasn't a problem in the old implementation, but now needs
this check.
* In DRI2CreateDrawable(), there is pPriv->swap_interval = 1;
Default swap interval is set to 1 at init, ie., sync to retrace is
on. I'd suggest considering a 0 swap interval as default. The
glXSwapIntervalSGI() call only allows to set swap_interval to > 0 due
to this bit from the SGI_swap_control spec: "glXSwapIntervalSGI
returns GLX_BAD_VALUE if parameter <interval> is less than or equal
to zero."
That means once you have a non-zero swap_interval, applications can't
opt-out of vsyn'ed bufferswaps, they can only opt-in. Iow there ain't
no way for an app to start without vsync.
Related, in DRI2SwapInterval(), a check for...
if (pPriv == NULL)
return BadDrawable;
... seems to be missing. All other routines check for this, so this
one should probably do so as well.
* There seems to be some inconsistency or missing pieces in the way a
drawable is destroyed, but i may miss something.
In DRI2DestroyDrawable(), once the pPriv->refcount drops to zero, the
drawable is destroyed if there aren't any swaps pending. If swaps are
pending, according to the comments there, destruction is postponed
until DRI2SwapComplete() gets called. Makes sense.
But in DRI2SwapComplete() there isn't any logic that would check if
(pPriv->swapsPending == 0 && pPriv->refcount == 0) and then free the
drawable. Instead it checks for pPriv->refcount == 0 and if so,
throws an error and free's the drawable. It shouldn't throw an error
and it should only free it at the very end of the routine after
DRI2WakeClient() was called, if (pPriv->swapsPending == 0). Current
implementation has a swap limit of 1, so this is always true, but if
the allowable number of outstanding swaps would be increased at some
time, this would cause problems.
Another issue is DRI2WaitMsc() and DRI2WaitSbc(). They should
probably complete immediately or throw an error if called while the
drawable is already awaiting destruction with a pPriv->refcount == 0?
If a wait for a certain target_msc or target_sbc is already
scheduled, one should probably also defer the destruction of the
drawable until they complete?
Proposals:
DRI2WaitSBC and DRI2WaitMSC, maybe others as well (e.g.,
DRI2WaitSwap?) should probably check for pPriv->refcount == 0 and
return either an error or return immediately on such an "undead"
drawable. Error return is probably better, given that the
OML_sync_control spec requires an error return if no context/drawable
is bound at time of call, and the drawable has been kind'a destroyed
already if refcount == 0.
DRI2DestroyDrawable() should probably postpone destruction if (pPriv-
>swapsPending > 0 || pPriv->blockedClient != NULL), ie., if any
waitsbc/waitmsc is already scheduled.
DRI2SwapComplete at its end, and DRI2WaitMSCComplete should probably
check if it is time to destroy the drawable before returning and do
so if neccessary to avoid leaks.
Finally at a quick glance, there are more places in the code were
there are potential problems with CARD64 values vs. 32 bit integers.
The OML_sync_control spec wants CARD64 as return values for msc and
and as target_msc for glXWaitForMscOML and glXSwapbuffersMscOML(),
but the DRM in the kernel only operates with 32 bit vbl.request/
reply.sequence numbers and some of the DRI2 functions pass values as
32 bit signed integers. Assuming a video refresh rate of 200 Hz for
the fastest crt monitors i'm aware of, this could cause some overflow
problems after 2^31 / 200 / 3600 / 24 = 124 days of uptime - not that
unrealistic for a desktop machine. This will probably need fixes to
the DRM or some wraparound handling or range checking in the xserver
to make it really robust for unlucky applications that happen to get
launched at the 124th day of uptime.
best,
-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
