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

[Eamon Walsh]

Tomas Carnecky wrote:
> 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.
>   

Thanks for catching this.


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

Thanks for helping out with this.  Please send me your patch and I will 
review it.


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

I agree.  Making DevPrivateKey point to intptr_t or void* would allow 
for future changes.


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

The ability to pre-allocate memory is a legacy of the old system.  Some 
object types offered this "feature," some didn't.  I think it would be 
great if this feature was dropped.  The callers of it would need to be 
changed to allocate their own space.  There would have to be a 
deprecation period so out-of-tree users could adjust.


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

The preallocation only supports fixed-size structures, and that is the 
most common use case.  But callers can use the private pointer for 
anything, including variable-sized stuff, if they allocate it themselves.

In the specific case of pixmap data, it is not kept in devPrivates.  
There is another opaque field in the PixmapRec structure used for that.


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

Having a key registration API would simplify things a little.  But the 
private code still can't assume that all keys are already known, because 
a new key can be registered after some objects are already created and 
in use.

There should probably also be a function dixCheckPrivate that checks for 
the existence of a private without actually allocating it.  I know there 
was some cleanup code that was touching privates and causing them to be 
allocated just so it could free them again.

> tom
>   


-- 
Eamon Walsh <ewalsh at tycho.nsa.gov>
National Security Agency