Merging aiglx

[Kristian Høgsberg]

Ian Romanick wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Kristian Høgsberg wrote:
>> Hello,
>>
>> It's been a couple of weeks since I posted my aiglx patch and there
>> hasn't been a lot of feedback, so I'm going to go ahead and assume
>> everybody is happy with the approach.  In the meantime, we've fixed a
>> number of crashers, added better logging and added hooks to allow the
>> GLXprovider to implement the GLX_EXT_tfp extension.
>>
>> Unless there are any objections to this, I'll merge the aiglx branch to
>> head this friday (yay, one less branch to keep in sync with mesa).
> 
> I do have a couple questions / comments about the code in the branch.  I
> don't think that it's anything that should block merging the code to
> HEAD, though.

Sounds good, thanks for reviewing.

> 1. Why is just glXGetDrawableAttributesSGIX implemented?
> 
> This function is part of GLX_SGIX_pbuffers (and glXGetDrawableAttributes
> is part of GLX 1.3).  Without that extension being advertised by the
> implementation, an application won't know it is available.  It should be
> easy enough to add the other functions but not make any fbconfigs that
> support pbuffers available.

I implemented glXGetDrawableAttributesSGIX() just so that I could report 
back the texture target the pixmap was bound to.  Maybe we could update 
the GLX_EXT_tfp specification to also pull in glXGetDrawableAttributes 
so that if you have GLX_SGIX_pbuffers, GLX_EXT_tfp or GLX 1.3 you know 
you have glXGetDrawableAttributes.

> 2. Why is __GLXtextureFromPixmap a stand alone structure?
> 
> It seems like the two function pointers in __GLXtextureFromPixmap should
> just be part of __GLXcontext.

I like the idea that there is one place to plug in support for an 
extension.  I was planning to make similar structs for the swap barrier 
and hyper pipe extensions.  Grouping the functions together gives a good 
overview of which callbacks are required to implement a given extension.

> 3. DrawableGone is coded as though setting __GLX_PENDING_DESTROY may
> automatically free the object.  Is that the case?  If so, there's a
> problem when the same drawable is bound for both reading and drawing.

I should probably go through that code again, I believe I should be able 
to get rid of the pending flags entirely.

> 4. Any idea why __glXDispatch had a test for whether or not we were
> expecting a follow-on RenderLarge request but __glXSwapDispatch did not?
> 
> In any case, I like it that those two functions were merged.
> Refactoring like that makes me happy. :)

My guess is that it's a bugfix that only went into the non-swapping code 
path.  So much the better that the two functions are now merged.

> 5. There's a (tivial) merge conflict in GL/mesa/swrast/Makefile.am.

Ok, I'll fix that up as I merge to head.

> 6. I'd recommend caching whether or not a context can support
> non-power-of-2 textures for GL_TEXTURE_2D in the GLXcontext structure.
> I see that the code is commented out, but using GL_TEXTURE_2D, if
> available, is the right way to go.  Since GL_TEXTURE_RECTANGLE_ARB uses
> unnormalized coordinates (i.e., [0,width] vs. [0,1]), how is this handled?

Right, and the glxdri.c implementation of tfp should proabably be move 
into glxcmds.c since there nothing DRI specific in it and could be used 
from the Mesa provider.  The difference between normalized and 
unnormalized coordinates should be handled by the application.  It's one 
of the unresolved issues in the tfp spec - current concensus seems to be 
that we query the drawable using glXGetDrawableAttributes() to figure 
out the texture target.  Another suggestion is that there's a consistent 
way to determine the texture target (based on pixmap dimensions and 
presense of various extensions) so the application can infer this 
without doing a server roundtrip.

> 7. How are GLX extensions handled?
> 
> On the client-side, the driver tells the loader which GLX extensions it
> can support.  It appears that right now all of the extensions that have
> GLX protocol supported are always advertised.  This is a problem as some
> GLX extesions (e.g., SGI_make_current_read, GLX_SGIX_hyperpipe, and
> GLX_SGIX_swap_barrier) are *not* supported by all DRI drivers.

I guess this is one thing I've skipped over.  The right way is to add a 
function pointer in the __GLXscreen struct to let the provider, eh, 
provide the list of extensions.

But I don't think any of these issues should hold up the merge either, 
we can continue work on this on head.

thanks,
Kristian