Menu
▾
▴
Re: [Dri-devel] [patch] dynamic glx extensions cleanup (the 2nd)
|
From: Ian R. <id...@us...> - 2003-08-11 19:21:40
|
Felix K=FChling wrote:
> So I moved the glx extension data to the end of __DRIscreenRec and adde=
d
> 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 b=
e
> fine. Those added entries are only accessed by libGL.
If nothing else, you should bump the GLX interface version and add a=20
comment in the structure. Other than that, I think it should be fine.
> 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=20
__GLXscreenConfigsRec would eliminate some tests in the code. I'm=20
leaning towards leaving it in __DRIscreen, but I could probably be=20
swayed the other way. :)
Sorry if the formatting is funky below. Your patches show up as=20
application/octet-stream, so Mozilla Mail doesn't put them in-line. I=20
had to cut-n-paste to get it in.
> Index: glx/glxextensions.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> 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 =3D GL_TRUE;
> static GLuint next_bit =3D 0;
I think that, since we can no longer add new extension strings, we can=20
eliminate next_bit as well.
> +/* global bit-fields of available extensions and their=20
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=20
here. All of these values (and __glXGLXClientExtensions) are,=20
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=20
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.
[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 =3D=3D NULL ) {
> __glXExtensionsCtr();
> - set_glx_extension( name, strlen( name ), GL_FALSE,=20
direct_support );
> + __glXExtensionsCtrScreen(psc);
> + set_glx_extension( name, strlen( name ), GL_FALSE,=20
psc->direct_support );
> }
I think we can relax this restriction a bit now. Querying=20
glXGetClientString( dpy, GLX_EXTENSIONS ) used to be the condition=20
because drivers could add new extensions. Since we've taken that=20
ability away, can we relax the condition? Perhaps check to see if=20
psc->effectiveGLXexts is still NULL instead?
[snip]
> @@ -451,13 +430,15 @@
> * \returns A pointer to the string.
> */
The psc parameter should be documented here. The important thing to=20
document is when it is valid for pcs to be NULL. It *appears* to me=20
that psc will be NULL iff GLX_DIRECT_RENDERING is not defined. If that=20
is the case, then should we have some conditionally compiled asserts?=20
Something like:
#ifdef GLX_DIRECT_RENDERING
assert( psc !=3D NULL );
#else
assert( psc =3D=3D NULL );
#endif
In that case we could also conditionally compile some code in the loop.=20
See my comments below.
> char *
> -__glXGetUsableExtensions( const char * server_string )
> +__glXGetUsableExtensions( __DRIscreen *psc, const char * server_strin=
g )
> {
> 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)=20
supports
> @@ -466,9 +447,11 @@
> * the direct rendering driver supports it.
> */
> for ( i =3D 0 ; i < 8 ; i++ ) {
> - usable[i] =3D (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] =3D client_support[i] & (client_only[i]
> + | (psc->direct_support[i] & (server_support[i] | direct_only[i])=
));
> + else
> + usable[i] =3D client_support[i] & client_only[i];
Gak. This troublesome bit of code...again. :) No matter what,=20
server_support has to be considered. I think the else-case should be:
usable[i] =3D 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=20
conditionally compile the different paths based on GLX_DIRECT_RENDERING.=20
The other would be to define a local variable called=20
temp_direct_support and either copy the contents of psc->direct_support=20
to it or memset it to all 0x0ff. Then there would be no need for the=20
if-statement in the loop.
|