New handling of privates [was: Re: xf86-video-intel 2.5 release planning]

[Tomas Carnecky]

Eamon Walsh wrote:
>> The 160 are a bit exaggerated, not all keys will be active at the same 
>> time. And the number could be cut down by a) ripping old code out 
>> (dpms, xvidmode etc) and b) changing existing code to use less private 
>> keys (dri2 for example uses three private keys: screen, window and 
>> pixmap, damage even four!).
> 
> Yes, this consolidation should be done.  I also have the OK from Peter 
> to remove some DIX-level keys associated with the MPX merge.  And 
> lately, a lot of code has been dropped from master which has cut down on 
> the number of keys.
> 
> The 160 number mentioned above seems exaggerated - with just one DDX and 
> driver active it should be much less.  But, since the lookup functions 
> are not inlined any more, it should be possible in the future to 
> implement a hash table or whatever mechanism.  As long as the keys are 
> still unique values it will be backwards-compatible.

I took your implementation as a base but changed it slightly. I also 
changed all DevPrivateKey occurrences in the code to be like you 
described (int blah; key=&blah). Still, xnest doesn't run, it crashes in 
xnestSetCursor(). Don't know where the problem is.
To summarize my changes: privates is just an array of pointers, nothing 
fancy. The first pointer isn't actually a pointer but the number of 
allocated elements in the array. Starting with index=1, the actual 
pointers to the private data follow. (my privates.c is attached).

Some additional comments to my code and the needed modifications

dixLookupPrivate(PrivateRec **privates, const DevPrivateKey key)
{

 >>> At this point *privates may be uninitialized. You also have to make 
sure that the index is within the array. Also, if the code used 
dixRequestPrivate() then it expects that this function allocates the 
memory, so return only when the pointer isn't NULL.

     if (*privates && *key &&
        (*privates)[0] >= *key &&
        (*privates)[*key] != NULL)
         return (*privates)[*key];

     pointer *ptr = dixAllocatePrivate(privates, key);
     return ptr ? *ptr : NULL;
}

Same with dixLookupPrivateAddr(). dixSetPrivate() also needed additional 
checks.

If you want the patch that changes all the uses of DevPrivateKey, I can 
give it to you. But it's incomplete and some parts of it need to be more 
thoroughly reviewed (especially the parts that used 'static 
DevPrivateKey key; and then initialize the key later on. The dpms code 
for example).


A couple more thoughts about this change. You are basically forcing 
DevPrivateKey to be 'int *'. If you later decide that an int isn't good 
enough, you'll have to touch all parts that use privates again - and 
that's no fun. Making DevPrivateKey an opaque pointer would solve this 
problem.
There are a lot things that happen implicitly (LookupPrivate() under 
some circumstances allocates memory etc) and that makes the code hard to 
understand and modify. It would be much better to either allocate the 
memory only within dix/privates.c or require each 'user' to allocate the 
memory and then pass the pointer to dix/private.c. But don't mix the two 
approaches. Are there users who dynamically allocate private memory with 
different sizes (say, based on the pixmap dimensions)? Or do all users 
allocate fixed structures (sizeof(foo))?
Also, requiring each 'user' to register the private keys beforehand 
would make the code much clearer (and easier to debug). Right now the 
key is first initialized at first use (which may as late as when 
dixLookupPrivate() is called), until then the private code doesn't even 
know such private data index will exist.

tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: privates.c
Type: text/x-csrc
Size: 6971 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg/attachments/20080801/8f390d76/attachment.c>