New handling of privates [was: Re: xf86-video-intel 2.5 release planning]
tom at dbservice.com
Fri Aug 1 12:01:19 PDT 2008
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) >= *key &&
(*privates)[*key] != NULL)
pointer *ptr = dixAllocatePrivate(privates, key);
return ptr ? *ptr : NULL;
Same with dixLookupPrivateAddr(). dixSetPrivate() also needed additional
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
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
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 6971 bytes
Desc: not available
More information about the xorg