Kristian,
As we're starting to incorporate dri2 in our code, I've run into some
major concerns about the sarea and the event mechanisms:
1) The SAREA is likely to be used by mixed 32-bit / 64 -bit clients and
servers. (If I understand things correctly, the kernel never accesses
the SAREA). However the sarea structs are safe against this. The layout
of the sarea structures should must be identcal, and one should probably
use the fixed size types (uint64_t, uint32_t etc.) and be very careful
with alignment.
2) The event buffer mechanism doesn't appear to be safe against buffer
wraps: Consider the following code from dri_util.c:
/* Check for wraparound. */
if (pcp && psp->dri2.buffer->prealloc - pdp->dri2.tail >
psp->dri2.buffer->size) {
/* If prealloc overlaps into what we just parsed, the
* server overwrote it and we have to reset our tail
* pointer. */
DRM_UNLOCK(psp->fd, psp->lock, pcp->hHWContext);
(*psp->dri2.loader->reemitDrawableInfo)(pdp, &pdp->dri2.tail,
pdp->loaderPrivate);
DRM_LIGHT_LOCK(psp->fd, psp->lock, pcp->hHWContext);
}
a) First, the if statement should really be a while statement, since we
have no guarantee that the X server won't wrap the event buffer after it
emits our drawable info, but before we get the lock in DRM_LIGHT_LOCK()
b) This is the most serious concern. If the client using pdp sleeps for
a while, and the X server rushs ahead and wraps the event buffer more
than one time, the pdp->dri2.tail will be completely stale and cannot
even be used for the wrapaound test, since the outcome will be
unpredictable. I think what's needed is a single wrap stamp in the
sarea, which is bumped by the X server each time it wraps the event
buffer. The client can then compare a saved value against this stamp and
get a new tail pointer if there's a mismatch. If the stamp is made
64-bit there's only a microscopic chance that we get a false stamp match
due to stamp wraparound.
3) The code in dri_util.c:
if (psp->dri2.enabled) {
__driParseEvents(pcp, pdp);
__driParseEvents(pcp, prp);
On the second call to __driParseEvent(pcp, prp), we may release the
hardware lock, which may invalidate the pdp info, and have it use stale
buffers / cliprects. One remedy to this is to have a function
__driParseEventsTwoDrawables(pcp, pdp, prp)
That makes sure both drawables are up-to-date after the last
DRM_LIGHT_LOCK.
And yes, the DRI1 code is incorrect here too.
We can propably help fix these issues, but we need your input on the
SAREA changes required for 1) and 2).
Cheers,
Thomas
-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08
--
_______________________________________________
Dri-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel
