I've been looking at the texmem branch and have found a few problems. I
wanted to explain the problems I found and get some feedback before
commiting any changes or putting a patch together.
The first item is the cause of the texture corruption with multiple
contexts that I have seen with both Radeon and Rage 128. In the
DRI_AGE_TEXTURES macro in texmem.h, the age test is reversed:
#define DRI_AGE_TEXTURES( heap ) \
do { \
if ( ((heap) != NULL) \
&& ((heap)->local_age > (heap)->global_age[0]) ) \
driAgeTextures( heap ); \
} while( 0 )
Here global_age[0] would be _larger_ than local_age after a context's
textures have been kicked out (global_age is incremented at each upload).
Changing the test to != or < fixes the problem. However, there is another
problem here. In the old driver-specific code, the local age was a signed
int, which was initialized to -1, and the global age was/is initialized to
0 (by memset-ing the SAREA). This caused the first context in a new
server generation to initialize the global LRU since the local and global
ages differed. With both now being initially 0, the global LRU is not
initialized before it is updated when the first texture upload happens.
One workaround would be to use local != global in the age test and
initialize the local age to anything > 0 if the global age is 0 when the
context is created.
Also, I think it would cause less confusion if the drivers' SAREAs used
unsigned ints where the pointers in the heap/region structs in texmem.h
do. It should be safe to assume sizeof(int) == sizeof(unsigned int),
right? Actually, even better would be if all the drivers used the
drmTextureRegion struct (mga does) defined in xf86drm.h, since the code to
manipulate the struct is now being made common to all drivers. The
difference between that struct and the current driver private versions is
that drmTextureRegion uses an explicit padding byte between the first
three bytes (unsigned chars) and the age (unsigned int). Would there be
compatibility problems in moving from a struct without the explicit
padding to one with it? :
typedef struct _drmTextureRegion {
unsigned char next;
unsigned char prev;
unsigned char in_use;
unsigned char padding; /* Explicitly pad this out */
unsigned int age;
} drmTextureRegion, *drmTextureRegionPtr;
vs.
typedef struct {
unsigned char next, prev; /* indices to form a circular LRU */
unsigned char in_use; /* owned by a client, or free? */
int age; /* tracked by clients to update local LRU's */
} radeon_tex_region_t;
There are also a few failing assertions related to placeholder texture
objects. In driTexturesGone, we need to set t->heap = heap or else the
assertion that t->heap != NULL fails in driDestroyTextureObject when
destroying a placeholder. The other assertions that I don't think are
valid are in the drivers' DestroyContext, where it's assumed that the
swapped list and texture object lists are empty. Even if Mesa calls the
driver's DeleteTexture for all the real texture objects at context
teardown (does this happen?), there may still be placeholder objects on
the swapped or texture_objects lists. I think it is safer to have
driDestroyTextureHeap iterate both lists and destroy any remaining texture
objects and remove the assertions.
The last problem I found is the drivers' call to driCreateTextureHeap in
CreateContext. Passing pointers to the texList and texAge arrays without
an index results in texList[0] and texAge[0] being passed for both texture
heaps (if the driver supports the AGP heap), so those should be indexed as
texList[i] and texAge[i] where i is the heapId.
Also, I think there's a small optimization we could make, but I need to
make sure this is valid. I observed two contexts (texobj and tunnel
demos) repeatedly deleting and creating the same placeholder in
driTexturesGone when switching contexts. I think it would be possible to
keep an existing placeholder if the offset and size of the placeholder in
the texture_objects list matches the global region which has been updated.
For debugging the LRUs, I added the printLocal/GlobalLRU functions from
the old driver code to texmem.c in my tree. I think it's useful to have
these at least as static functions to use for debugging the common texmem
code.
At any rate, I can put a patch together for review, but I wanted to see if
there's anything I'm missing here.
--
Leif Delgass
http://www.retinalburn.net
-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
_______________________________________________
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel
