Re: [Dri-devel] texmem-0-0-1 branch

[Ian Romanick]

Leif Delgass wrote:

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.

Thanks for looking over the code. Everyone's been so busy lately that I don't think many people have had much of a chance to delve into it much.

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.

I believe your analysis is essentially correct. Interestingly, the test in driAgeTextures IS correct. As far as initializing the global LRU goes, when there are no textures allocated, there is nothing in the LRU.

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;

I cannot think of a case on any of the supported architectures where sizeof( unsigned int ) != sizeof( int ). I'm not sure that ANSI C99 permits such an implementation.

As far as replacing the device specific *_tex_region_t structures with drmTextureRegion, that would probably be a good idea. In fact, dri_texture_region (in texmem.h) should be replaced as well. I didn't notice drmTextureRegion when I was doing that part of the work (about 10 months ago).

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

How could a texture get into a heap's list that was not allocated from that heap? If such a case exists, it is a bug.

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.

Mesa is supposed to destroy all the real textures before it gets to DestroyContext. The textures on the swapped list and on each heap's texture_objects list are textures that had actual texture data / memory associated with them. The place holder textures should NEVER be on any of these lists.

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.

I believe you are correct again. In fact, if all the various *_tex_region_t structures are replaced with drmTextureRegion and this problem is fixed, we can get rid of the ugly type-cast in this call.

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.

That optimization sounds fine.

Adding the debug functions back in texmem.c is not a bad idea. Just make sure that we don't get any compiler warnings when they aren't called (i.e., static function not used, etc.). :)

At any rate, I can put a patch together for review, but I wanted to see if there's anything I'm missing here.

I'll look at it when you send it. It sounds like you've done some good work. :) How is the r128 driver in texmem-0-0-1 working these days?



-------------------------------------------------------
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