|Summary:||Mesa advertises bogus GL_ARB_shading_language_120|
|Product:||Mesa||Reporter:||Bruce Merry <bmerry>|
|Status:||RESOLVED FIXED||QA Contact:|
|i915 platform:||i915 features:|
Patch that compiles, but doesn't work
Description Bruce Merry 2010-08-31 14:12:09 UTC
I noticed this in OpenGL renderer string: Software Rasterizer OpenGL version string: 2.1 Mesa 7.8.2 OpenGL shading language version string: 1.20 but grepping the last git master (mesa_7_6_1_rc1-12198-g99f3c9c) shows mention of this extension in multiple places. It is not a listed ARB extension at http://www.opengl.org/registry/ - the correct way to indicate support for GLSL 1.20 is through the SHADING_LANGUAGE_VERSION query (as Mesa does).
Comment 1 Ian Romanick 2010-08-31 15:12:26 UTC
That is bogus. I believe the reason Mesa does this is there is no other way for a driver to advertise to the core that it supports 1.20 than enabling an extension. As far as I'm aware 1.20 doesn't require any extra hardware support, so we could probably fix this by always advertising 1.20 if GLSL is supported by the driver.
Comment 2 Brian Paul 2010-08-31 18:30:55 UTC
Bruce, are you talking about seeing GL_ARB_shading_language_120 in the list of extensions? Ian is right that drivers would enable ctx->Extensions.ARB_shading_language_120 if they support it. I see that NVIDIA doesn't list GL_ARB_shading_language_120 in the extension string. I guess we need some way to make this extension "invisible".
Comment 3 Bruce Merry 2010-09-01 00:25:15 UTC
> Bruce, are you talking about seeing GL_ARB_shading_language_120 in the list of extensions? Sorry yes, that's what I meant: it's the fact that it's showing up in GL_EXTENSIONS that is a bug. I haven't looked at the internal interfaces between drivers and the core.
Comment 4 Brian Paul 2010-09-03 14:11:29 UTC
Created attachment 38410 [details] [review] proposed patch This patch does away with the internal GL_ARB_shading_language_120 extension and replaces it with a new ctx->Const.GLSLVersion field. We can/should eventually pass this into the GLSL compiler. If there aren't any concerns with this patch I'll commit it in a few days.
Comment 5 Ian Romanick 2010-09-03 15:22:38 UTC
Review of attachment 38410 [details] [review]: I see two minor problems with the patch. 1. I think we still need GL_VERSION_2_1_functions to show up in the extensions list. Otherwise there are cases where the new functions won't be available from GetProcAddress. The r300 driver handles this a different way, so i965 could just copy that. 2. GLSL ES needs 100 to be a valid GLSL version. Other than that, it looks great to me.
Comment 6 Brian Paul 2010-09-03 17:04:53 UTC
Created attachment 38415 [details] [review] second patch Thanks for reviewing, Ian. Here's an updated patch. I'm unable to test the Intel driver at this time. Could you give it a try? The change for ES is also untested but if there's anything wrong I'm sure it can be easily fixed up.
Comment 7 Bruce Merry 2010-09-04 01:38:28 UTC
The addition to r600_context.c seems to be mis-indented, although that might just be my browser.
Comment 8 Brian Paul 2010-09-21 17:15:57 UTC
I've committed the second patch (e7087175f8a04f777403366fb34b58edd00f4d60)
Comment 9 Kenneth Graunke 2010-09-22 01:00:15 UTC
Created attachment 38867 [details] [review] Patch that compiles, but doesn't work
Comment 10 Kenneth Graunke 2010-09-22 01:03:57 UTC
Unfortunately, the patch breaks the build for me: brw_context.c:45:35: error: array type has incomplete element type brw_context.c:46:22: error: ‘GL_VERSION_2_1_functions’ undeclared here (not in a function) brw_context.c: In function ‘brwCreateContext’: brw_context.c:163:4: warning: implicit declaration of function ‘driInitExtensions’ brw_context.c: At top level: brw_context.c:45:35: warning: ‘gl_21_extension’ defined but not used I attached a patch which moves this to intel_extensions.c, where it probably belongs...unfortunately, it doesn't /work/...running a random piglit test gives: Mesa 7.10-devel implementation error: Trying to enable unknown extension: GL_VERSION_2_1 Perhaps you really just want to call _mesa_enable_2_1_extensions?
Comment 11 Kristian Høgsberg 2010-09-22 08:09:30 UTC
commit b91dba49e0b08b18dbd6c477facdcc7b5472c8c7 Author: Kristian Høgsberg <firstname.lastname@example.org> Date: Wed Sep 22 11:01:11 2010 -0400 intel: Fix GL_ARB_shading_language_120 commit Fix commit e7087175f8a04f777403366fb34b58edd00f4d60. Move the reference to GL_VERSION_2_1_functions to intel_extensions.c where it's available, don't try to enable a non-existing extension and advertise 1.20 for all intel chipsets, not just GEN4 and up.
Comment 12 Ian Romanick 2010-09-22 09:51:08 UTC
This looks fine for now, and I'm going to cherry-pick both patches to 7.9. In the future we will want to expose the supported versions as a bitmask. In later versions of OpenGL core profile the older shading languages cease to be supported. This may also allow a more unified method for testing support for GLSL ES (which is version 1.00). There will be a lot of other changes required to support this, so we can cross that bridge when we come to it.