On Mon, 11 Aug 2003 12:14:31 -0700
Ian Romanick <[EMAIL PROTECTED]> wrote:
> Felix K�hling wrote:
>
> > So I moved the glx extension data to the end of __DRIscreenRec and added
> > a pointer to __DRIscreenPrivateRec that points back to the
> > __DRIscreenRec. This pointer is then passed to
> > __glXScrEnable/DisableExtension by the driver's createScreen function.
> > There may be binary compatibility issues as __DRIscreenRec is accessed
> > by both libGL and the drivers. But adding new stuff to the end should be
> > fine. Those added entries are only accessed by libGL.
>
> If nothing else, you should bump the GLX interface version and add a
> comment in the structure. Other than that, I think it should be fine.
Ok. I was wondering why the drivers have to check the GLX interface
version, though. Is the result of glXGetProcAddress not sufficient?
>
> > Then it may be better to move that data back to __GLXscreenConfigs and
> > add a pointer to __DRIscreenRec that points back to __GLXscreenConfigs.
> > Thoughts?
>
> Hmm...__DRIscreen seems like the right place, but putting the data in
> __GLXscreenConfigsRec would eliminate some tests in the code. I'm
> leaning towards leaving it in __DRIscreen, but I could probably be
> swayed the other way. :)
>
> Sorry if the formatting is funky below. Your patches show up as
> application/octet-stream, so Mozilla Mail doesn't put them in-line. I
> had to cut-n-paste to get it in.
I'll try to make it something like text/plain next time.
>
> > Index: glx/glxextensions.c
> > ===================================================================
> > RCS file: /cvsroot/dri/xc/xc/lib/GL/glx/glxextensions.c,v
> > retrieving revision 1.3
> > diff -u -r1.3 glxextensions.c
> > --- glx/glxextensions.c 3 May 2003 05:19:30 -0000 1.3
> > +++ glx/glxextensions.c 9 Aug 2003 20:58:53 -0000
> > @@ -116,18 +116,23 @@
> > { NULL }
> > };
> >
> > +/* global list of available extensions */
> > static struct glx_extension ext_list;
> > static GLboolean ext_list_first_time = GL_TRUE;
> > static GLuint next_bit = 0;
>
> I think that, since we can no longer add new extension strings, we can
> eliminate next_bit as well.
>
> > +/* global bit-fields of available extensions and their
> characteristics */
> > static unsigned char client_support[8];
> > -static unsigned char direct_support[8];
> > static unsigned char client_only[8];
> > static unsigned char direct_only[8];
> > +/* extensions enabled by default on all screens */
> > +static unsigned char direct_support[8];
>
> After this patch is committed, there is another change we could make
> here. All of these values (and __glXGLXClientExtensions) are,
> essentially, compile-time constants. We could then:
>
> 1. Change these to use static initializers & make them const.
> 2. Change default_glx_extensions to a new name, such as
> known_glx_extensions & make known_glx_extensions const.
> 3. Remove the *_support & *_only fields from known_glx_extensions.
> 4. Eliminate ext_list, add_extension, & __glXExtensionsCtr.
Yeah, things would get a lot simpler, but the static initializers would
be quite unreadable, ..., unless we do it like:
1 << ARB_get_proc_address |
1 << ARB_multisample |
0 << ARB_render_texture |
...
Unfortunately there are more than 32 extensions, so we can't OR
everything together to one unsigned long.
>
> [snip]
>
> > /**
> > - * Disable a named GLX extension.
> > + * Disable a named GLX extension on a given screen.
> > *
> > * \param name Name of the extension to disable.
> > * \sa __glXEnableExtension
> > */
> > void
> > -__glXDisableExtension( const char * name )
> > +__glXScrDisableExtension( __DRIscreen *psc, const char * name )
> > {
> > if ( __glXGLXClientExtensions == NULL ) {
> > __glXExtensionsCtr();
> > - set_glx_extension( name, strlen( name ), GL_FALSE,
> direct_support );
> > + __glXExtensionsCtrScreen(psc);
> > + set_glx_extension( name, strlen( name ), GL_FALSE,
> psc->direct_support );
> > }
>
> I think we can relax this restriction a bit now. Querying
> glXGetClientString( dpy, GLX_EXTENSIONS ) used to be the condition
> because drivers could add new extensions. Since we've taken that
> ability away, can we relax the condition? Perhaps check to see if
> psc->effectiveGLXexts is still NULL instead?
There is no such variable (yet), but it would be worth adding it.
__glXGetUsableExtensions would compute it only the first time.
>
> [snip]
>
> > @@ -451,13 +430,15 @@
> > * \returns A pointer to the string.
> > */
>
> The psc parameter should be documented here. The important thing to
> document is when it is valid for pcs to be NULL. It *appears* to me
> that psc will be NULL iff GLX_DIRECT_RENDERING is not defined. If that
> is the case, then should we have some conditionally compiled asserts?
> Something like:
I just noticed that this code won't compile if GLX_DIRECT_RENDERING is
not defined because DRIscreen is not even defined in that case. So there
are two choices here: put all the function definitions in #ifdefs
(Enable/DisableExtension aren't needed at all without direct rendering)
or leave the extension-related data in __GLXscreenConfigs which is
always defined.
>
> #ifdef GLX_DIRECT_RENDERING
> assert( psc != NULL );
> #else
> assert( psc == NULL );
> #endif
>
> In that case we could also conditionally compile some code in the loop.
> See my comments below.
>
> > char *
> > -__glXGetUsableExtensions( const char * server_string )
> > +__glXGetUsableExtensions( __DRIscreen *psc, const char * server_string )
> > {
> > unsigned char server_support[8];
> > unsigned char usable[8];
> > unsigned i;
> >
> > __glXExtensionsCtr();
> > + if ( psc )
> > + __glXExtensionsCtrScreen( psc );
> > __glXProcessServerString( server_string, server_support );
> >
> > /* An extension is supported if the client-side (i.e., libGL)
> supports
> > @@ -466,9 +447,11 @@
> > * the direct rendering driver supports it.
> > */
> > for ( i = 0 ; i < 8 ; i++ ) {
> > - usable[i] = (client_support[i] & client_only[i])
> > - | (client_support[i] & (direct_support[i] & server_support[i]))
> > - | (client_support[i] & (direct_support[i] & direct_only[i]));
> > + if ( psc )
> > + usable[i] = client_support[i] & (client_only[i]
> > + | (psc->direct_support[i] & (server_support[i] | direct_only[i])));
> > + else
> > + usable[i] = client_support[i] & client_only[i];
>
> Gak. This troublesome bit of code...again. :) No matter what,
> server_support has to be considered. I think the else-case should be:
>
> usable[i] = client_support[i] & (client_only[i]|server_support[i]);
>
> This loop could be cleaned up in one of two ways. The first would be to
> conditionally compile the different paths based on GLX_DIRECT_RENDERING.
> The other would be to define a local variable called
> temp_direct_support and either copy the contents of psc->direct_support
> to it or memset it to all 0x0ff. Then there would be no need for the
> if-statement in the loop.
Ok. I'd do it like this if we still have to (depending on the decision above):
#ifdef GLX_DIRECT_RENDERING
char *direct_support = psc->direct_support;
#else
char direct_support[8] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
#endif
Felix
------------ __\|/__ ___ ___ -------------------------
Felix ___\_e -_/___/ __\___/ __\_____ You can do anything,
K�hling (_____\�/____/ /_____/ /________) just not everything
[EMAIL PROTECTED] \___/ \___/ U at the same time.
-------------------------------------------------------
This SF.Net email sponsored by: Free pre-built ASP.NET sites including
Data Reports, E-commerce, Portals, and Forums are available now.
Download today and enter to win an XBOX or Visual Studio .NET.
http://aspnet.click-url.com/go/psa00100003ave/direct;at.aspnet_072303_01/01
_______________________________________________
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel
