Merging aiglx

[David Reveman]

On Thu, 2006-03-09 at 19:08 -0500, Kristian Høgsberg wrote:
> 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.

I think that just having GLX_EXT_tfp require GLX 1.3 (as currently in
the spec) is best.

> 
> > 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
> 
> _______________________________________________
> xorg mailing list
> xorg at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xorg

-David