| Message ID | 1342788854-4872-4-git-send-email-jon.turney@dronecode.org.uk |
|---|---|
| State | Superseded, archived |
| Headers | show |
diff --git a/glx/glxdriswrast.c b/glx/glxdriswrast.c index b478398..d5cfaf1 100644 --- a/glx/glxdriswrast.c +++ b/glx/glxdriswrast.c @@ -75,6 +75,8 @@ struct __GLXDRIscreen { const __DRIcopySubBufferExtension *copySubBuffer; const __DRItexBufferExtension *texBuffer; const __DRIconfig **driConfigs; + + unsigned char glx_enable_bits[__GLX_EXT_BYTES]; }; struct __GLXDRIcontext { @@ -434,6 +436,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen) { const char *driverName = "swrast"; __GLXDRIscreen *screen; + size_t buffer_size; screen = calloc(1, sizeof *screen); if (screen == NULL) @@ -445,6 +448,26 @@ __glXDRIscreenProbe(ScreenPtr pScreen) screen->base.swapInterval = NULL; screen->base.pScreen = pScreen; + /* + Rather than calling __glXInitExtensionEnableBits, we explicitly enable a + specific set of extensions here to maintain the historical behaviour, which + is slightly different. + */ + __glXEnableExtension(screen->glx_enable_bits, "GLX_ARB_multisample"); + __glXEnableExtension(screen->glx_enable_bits, "GLX_EXT_visual_info"); + __glXEnableExtension(screen->glx_enable_bits, "GLX_EXT_visual_rating"); + __glXEnableExtension(screen->glx_enable_bits, "GLX_EXT_import_context"); + __glXEnableExtension(screen->glx_enable_bits, "GLX_EXT_texture_from_pixmap"); + __glXEnableExtension(screen->glx_enable_bits, "GLX_OML_swap_method"); + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGI_make_current_read"); +#ifndef __APPLE__ + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIS_multisample"); +#endif + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIX_fbconfig"); + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIX_pbuffer"); + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIX_visual_select_group"); + __glXEnableExtension(screen->glx_enable_bits, "GLX_MESA_copy_sub_buffer"); + screen->driver = glxProbeDriver(driverName, (void **) &screen->core, __DRI_CORE, __DRI_CORE_VERSION, @@ -473,6 +496,19 @@ __glXDRIscreenProbe(ScreenPtr pScreen) __glXScreenInit(&screen->base, pScreen); + /* The first call simply determines the length of the extension string. + * This allows us to allocate some memory to hold the extension string, + * but it requires that we call __glXGetExtensionString a second time. + */ + buffer_size = __glXGetExtensionString(screen->glx_enable_bits, NULL); + if (buffer_size > 0) { + free(screen->base.GLXextensions); + + screen->base.GLXextensions = xnfalloc(buffer_size); + (void) __glXGetExtensionString(screen->glx_enable_bits, + screen->base.GLXextensions); + } + screen->base.GLXmajor = 1; screen->base.GLXminor = 4; diff --git a/glx/glxscreens.c b/glx/glxscreens.c index 037b037..3ca2105 100644 --- a/glx/glxscreens.c +++ b/glx/glxscreens.c @@ -163,18 +163,6 @@ static const char GLServerExtensions[] = static char GLXServerVendorName[] = "SGI"; unsigned glxMajorVersion = SERVER_GLX_MAJOR_VERSION; unsigned glxMinorVersion = SERVER_GLX_MINOR_VERSION; -static char GLXServerExtensions[] = - "GLX_ARB_multisample " - "GLX_EXT_visual_info " - "GLX_EXT_visual_rating " - "GLX_EXT_import_context " - "GLX_EXT_texture_from_pixmap " - "GLX_OML_swap_method " "GLX_SGI_make_current_read " -#ifndef __APPLE__ - "GLX_SGIS_multisample " -#endif - "GLX_SGIX_fbconfig " - "GLX_SGIX_pbuffer " "GLX_MESA_copy_sub_buffer "; static Bool glxCloseScreen(ScreenPtr pScreen) @@ -328,7 +316,7 @@ __glXScreenInit(__GLXscreen * pGlxScreen, ScreenPtr pScreen) pGlxScreen->pScreen = pScreen; pGlxScreen->GLextensions = strdup(GLServerExtensions); pGlxScreen->GLXvendor = strdup(GLXServerVendorName); - pGlxScreen->GLXextensions = strdup(GLXServerExtensions); + pGlxScreen->GLXextensions = strdup(""); /* All GLX providers must support all of the functionality required for at * least GLX 1.2. If the provider supports a higher version, the GLXminor
I don't really like having redundant data. Is there really a need for us to store glx_enable_bits in the DRI screen record? It seems like we could just keep that local to __glXDRIscreenProbe. On Jul 20, 2012, at 05:54, Jon TURNEY <jon.turney@dronecode.org.uk> wrote: > Ensure this kind of bug doesn't occur in future, by generating the GLX extension > list for swrast, in the same way as dri and dri2 do, rather than using a fixed > list of GLX extensions for swrast. > > We explicity select the extensions reported by swrast rather than using > __glXInitExtensionEnableBits(), to maintain the historical behaviour, which is > slightly different: > > - GLX_SGIS_multisample is not reported on APPLE > - GLX_SGIX_visual_select_group isn't reported > > (How swrast handles GLX_MESA_copy_sub_buffer still looks a bit wonky: We always > enable it, but then subsequently also check if the loaded driver supports it, > and if it doesn't all glxCopySubBufferMESA() calls are just going to fail > GLXBadDrawable as the copySubBuffer function pointer is NULL. This probably > isn't a practical concern.) > > Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk> > --- > glx/glxdriswrast.c | 36 ++++++++++++++++++++++++++++++++++++ > glx/glxscreens.c | 14 +------------- > 2 files changed, 37 insertions(+), 13 deletions(-) > > diff --git a/glx/glxdriswrast.c b/glx/glxdriswrast.c > index b478398..d5cfaf1 100644 > --- a/glx/glxdriswrast.c > +++ b/glx/glxdriswrast.c > @@ -75,6 +75,8 @@ struct __GLXDRIscreen { > const __DRIcopySubBufferExtension *copySubBuffer; > const __DRItexBufferExtension *texBuffer; > const __DRIconfig **driConfigs; > + > + unsigned char glx_enable_bits[__GLX_EXT_BYTES]; > }; > > struct __GLXDRIcontext { > @@ -434,6 +436,7 @@ __glXDRIscreenProbe(ScreenPtr pScreen) > { > const char *driverName = "swrast"; > __GLXDRIscreen *screen; > + size_t buffer_size; > > screen = calloc(1, sizeof *screen); > if (screen == NULL) > @@ -445,6 +448,26 @@ __glXDRIscreenProbe(ScreenPtr pScreen) > screen->base.swapInterval = NULL; > screen->base.pScreen = pScreen; > > + /* > + Rather than calling __glXInitExtensionEnableBits, we explicitly enable a > + specific set of extensions here to maintain the historical behaviour, which > + is slightly different. > + */ > + __glXEnableExtension(screen->glx_enable_bits, "GLX_ARB_multisample"); > + __glXEnableExtension(screen->glx_enable_bits, "GLX_EXT_visual_info"); > + __glXEnableExtension(screen->glx_enable_bits, "GLX_EXT_visual_rating"); > + __glXEnableExtension(screen->glx_enable_bits, "GLX_EXT_import_context"); > + __glXEnableExtension(screen->glx_enable_bits, "GLX_EXT_texture_from_pixmap"); > + __glXEnableExtension(screen->glx_enable_bits, "GLX_OML_swap_method"); > + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGI_make_current_read"); > +#ifndef __APPLE__ > + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIS_multisample"); > +#endif > + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIX_fbconfig"); > + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIX_pbuffer"); > + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIX_visual_select_group"); > + __glXEnableExtension(screen->glx_enable_bits, "GLX_MESA_copy_sub_buffer"); > + > screen->driver = glxProbeDriver(driverName, > (void **) &screen->core, > __DRI_CORE, __DRI_CORE_VERSION, > @@ -473,6 +496,19 @@ __glXDRIscreenProbe(ScreenPtr pScreen) > > __glXScreenInit(&screen->base, pScreen); > > + /* The first call simply determines the length of the extension string. > + * This allows us to allocate some memory to hold the extension string, > + * but it requires that we call __glXGetExtensionString a second time. > + */ > + buffer_size = __glXGetExtensionString(screen->glx_enable_bits, NULL); > + if (buffer_size > 0) { > + free(screen->base.GLXextensions); > + > + screen->base.GLXextensions = xnfalloc(buffer_size); > + (void) __glXGetExtensionString(screen->glx_enable_bits, > + screen->base.GLXextensions); > + } > + > screen->base.GLXmajor = 1; > screen->base.GLXminor = 4; > > diff --git a/glx/glxscreens.c b/glx/glxscreens.c > index 037b037..3ca2105 100644 > --- a/glx/glxscreens.c > +++ b/glx/glxscreens.c > @@ -163,18 +163,6 @@ static const char GLServerExtensions[] = > static char GLXServerVendorName[] = "SGI"; > unsigned glxMajorVersion = SERVER_GLX_MAJOR_VERSION; > unsigned glxMinorVersion = SERVER_GLX_MINOR_VERSION; > -static char GLXServerExtensions[] = > - "GLX_ARB_multisample " > - "GLX_EXT_visual_info " > - "GLX_EXT_visual_rating " > - "GLX_EXT_import_context " > - "GLX_EXT_texture_from_pixmap " > - "GLX_OML_swap_method " "GLX_SGI_make_current_read " > -#ifndef __APPLE__ > - "GLX_SGIS_multisample " > -#endif > - "GLX_SGIX_fbconfig " > - "GLX_SGIX_pbuffer " "GLX_MESA_copy_sub_buffer "; > > static Bool > glxCloseScreen(ScreenPtr pScreen) > @@ -328,7 +316,7 @@ __glXScreenInit(__GLXscreen * pGlxScreen, ScreenPtr pScreen) > pGlxScreen->pScreen = pScreen; > pGlxScreen->GLextensions = strdup(GLServerExtensions); > pGlxScreen->GLXvendor = strdup(GLXServerVendorName); > - pGlxScreen->GLXextensions = strdup(GLXServerExtensions); > + pGlxScreen->GLXextensions = strdup(""); > > /* All GLX providers must support all of the functionality required for at > * least GLX 1.2. If the provider supports a higher version, the GLXminor > -- > 1.7.9 > > _______________________________________________ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel >
On 24/07/2012 19:11, Jeremy Huddleston Sequoia wrote: > I don't really like having redundant data. Is there really a need for us > to store glx_enable_bits in the DRI screen record? It seems like we could > just keep that local to __glXDRIscreenProbe. It's just written that way to make it the same as glxdri.c and glxdri2.c (There's probably some refactoring of duplicate code which could take place, also) > On Jul 20, 2012, at 05:54, Jon TURNEY wrote: >> Ensure this kind of bug doesn't occur in future, by generating the GLX extension >> list for swrast, in the same way as dri and dri2 do, rather than using a fixed >> list of GLX extensions for swrast. >> >> We explicity select the extensions reported by swrast rather than using >> __glXInitExtensionEnableBits(), to maintain the historical behaviour, which is >> slightly different: >> >> - GLX_SGIS_multisample is not reported on APPLE >> - GLX_SGIX_visual_select_group isn't reported >> >> (How swrast handles GLX_MESA_copy_sub_buffer still looks a bit wonky: We always >> enable it, but then subsequently also check if the loaded driver supports it, >> and if it doesn't all glxCopySubBufferMESA() calls are just going to fail >> GLXBadDrawable as the copySubBuffer function pointer is NULL. This probably >> isn't a practical concern.) >> >> Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk> >> --- >> glx/glxdriswrast.c | 36 ++++++++++++++++++++++++++++++++++++ >> glx/glxscreens.c | 14 +------------- >> 2 files changed, 37 insertions(+), 13 deletions(-) [...] >> +#ifndef __APPLE__ >> + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIS_multisample"); >> +#endif As far as I can tell, this conditional is entirely pointless (now, I'm guessing that there is some historical reason for it), as this GL provider is never used by Xquartz. Perhaps you can confirm that?
On Jul 25, 2012, at 6:45 AM, Jon TURNEY <jon.turney@dronecode.org.uk> wrote: >>> +#ifndef __APPLE__ >>> + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIS_multisample"); >>> +#endif > > As far as I can tell, this conditional is entirely pointless (now, I'm > guessing that there is some historical reason for it), as this GL provider is > never used by Xquartz. Perhaps you can confirm that? Correct, it's not used by XQuartz, but it is used by other servers (like Xorg) which can run on darwin. __APPLE__ just means that it's being built for OS X, not XQuartz. It originally entered xserver with e56e24af252bd3b8e58076adf0f8eabf1103f187, and I changed it from __DARWIN__ to __APPLE__ in 54654815fa5e59b25cfd1fa72610120b72c10175. But I don't know *why* it is there, and it doesn't really make sense to me. The e56e24af commit was http://lists.x.org/archives/xorg-commit/2004-June/001142.html: So that doesn't give much clue. Giving it a punt is the right thing to do. If it has fallout, I'm sure the 1 person who cares about it will say something. --Jeremy
On 07/25/2012 08:36 AM, Jeremy Huddleston Sequoia wrote: > > On Jul 25, 2012, at 6:45 AM, Jon TURNEY <jon.turney@dronecode.org.uk> wrote: >>>> +#ifndef __APPLE__ >>>> + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIS_multisample"); >>>> +#endif >> >> As far as I can tell, this conditional is entirely pointless (now, I'm >> guessing that there is some historical reason for it), as this GL provider is >> never used by Xquartz. Perhaps you can confirm that? > > Correct, it's not used by XQuartz, but it is used by other servers (like Xorg) which can run on darwin. > > __APPLE__ just means that it's being built for OS X, not XQuartz. > > It originally entered xserver with e56e24af252bd3b8e58076adf0f8eabf1103f187, and I changed it from __DARWIN__ to __APPLE__ in 54654815fa5e59b25cfd1fa72610120b72c10175. But I don't know *why* it is there, and it doesn't really make sense to me. The e56e24af commit was http://lists.x.org/archives/xorg-commit/2004-June/001142.html: > > So that doesn't give much clue. > > Giving it a punt is the right thing to do. If it has fallout, I'm sure the 1 person who cares about it will say something. I can't see how it would make any difference. Like I said in a previous post, for GLX, the SGIS and ARB extensions are identical.
On Jul 25, 2012, at 9:14 AM, Ian Romanick <idr@freedesktop.org> wrote: > On 07/25/2012 08:36 AM, Jeremy Huddleston Sequoia wrote: >> >> On Jul 25, 2012, at 6:45 AM, Jon TURNEY <jon.turney@dronecode.org.uk> wrote: >>>>> +#ifndef __APPLE__ >>>>> + __glXEnableExtension(screen->glx_enable_bits, "GLX_SGIS_multisample"); >>>>> +#endif >>> >>> As far as I can tell, this conditional is entirely pointless (now, I'm >>> guessing that there is some historical reason for it), as this GL provider is >>> never used by Xquartz. Perhaps you can confirm that? >> >> Correct, it's not used by XQuartz, but it is used by other servers (like Xorg) which can run on darwin. >> >> __APPLE__ just means that it's being built for OS X, not XQuartz. >> >> It originally entered xserver with e56e24af252bd3b8e58076adf0f8eabf1103f187, and I changed it from __DARWIN__ to __APPLE__ in 54654815fa5e59b25cfd1fa72610120b72c10175. But I don't know *why* it is there, and it doesn't really make sense to me. The e56e24af commit was http://lists.x.org/archives/xorg-commit/2004-June/001142.html: >> >> So that doesn't give much clue. >> >> Giving it a punt is the right thing to do. If it has fallout, I'm sure the 1 person who cares about it will say something. > > I can't see how it would make any difference. Like I said in a previous post, for GLX, the SGIS and ARB extensions are identical. Yeah, but someone at some point had a reason for doing this, and I just wish I knew what it was. I just like to unwrap puzzling mysteries ;) --Jeremy
Ensure this kind of bug doesn't occur in future, by generating the GLX extension list for swrast, in the same way as dri and dri2 do, rather than using a fixed list of GLX extensions for swrast. We explicity select the extensions reported by swrast rather than using __glXInitExtensionEnableBits(), to maintain the historical behaviour, which is slightly different: - GLX_SGIS_multisample is not reported on APPLE - GLX_SGIX_visual_select_group isn't reported (How swrast handles GLX_MESA_copy_sub_buffer still looks a bit wonky: We always enable it, but then subsequently also check if the loaded driver supports it, and if it doesn't all glxCopySubBufferMESA() calls are just going to fail GLXBadDrawable as the copySubBuffer function pointer is NULL. This probably isn't a practical concern.) Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk> --- glx/glxdriswrast.c | 36 ++++++++++++++++++++++++++++++++++++ glx/glxscreens.c | 14 +------------- 2 files changed, 37 insertions(+), 13 deletions(-)