Bug 29910 - Mesa advertises bogus GL_ARB_shading_language_120
Mesa advertises bogus GL_ARB_shading_language_120
Product: Mesa
Classification: Unclassified
Component: Mesa core
All All
: medium minor
Assigned To: mesa-dev
Depends on:
  Show dependency treegraph
Reported: 2010-08-31 14:12 UTC by Bruce Merry
Modified: 2010-09-22 09:51 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

proposed patch (8.91 KB, patch)
2010-09-03 14:11 UTC, Brian Paul
Details | Splinter Review
second patch (8.75 KB, patch)
2010-09-03 17:04 UTC, Brian Paul
Details | Splinter Review
Patch that compiles, but doesn't work (1.45 KB, patch)
2010-09-22 01:00 UTC, Kenneth Graunke
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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

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 <krh@bitplanet.net>
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.