[Piglit] [PATCH 1/6] shader_runner and glslparsertest: choose GL version more accurately.

[Chad Versace]

On 02/20/2013 07:13 AM, Paul Berry wrote:
> On 19 February 2013 23:11, Michel Dänzer <michel at daenzer.net
> <mailto:michel at daenzer.net>> wrote:
> 
>     On Die, 2013-02-19 at 12:52 +0100, Michel Dänzer wrote:
>     > On Mon, 2013-02-18 at 10:14 -0800, Paul Berry wrote:
>     > > On 18 February 2013 07:53, Michel Dänzer <michel at daenzer.net
>     <mailto:michel at daenzer.net>> wrote:
>     > >         On Don, 2013-02-14 at 08:50 -0800, Paul Berry wrote:
>     > >         > Previously, both shader_runner and glslparsertest contained
>     > >         switch
>     > >         > statements that attempted to choose a GL version for testing
>     > >         based on
>     > >         > a GLSL version requirement, and those switch statements
>     > >         didn't choose
>     > >         > a GL version very accurately (for example, they converted
>     > >         GLSL version
>     > >         > 1.50 to GL version 3.1, which doesn't work).
>     > >         >
>     > >         > This patch replaces the switch statements with a single
>     > >         common
>     > >         > function (in piglit-util-gl-common) that does the right
>     > >         thing.
>     > >
>     > >
>     > >         This change caused quick-driver.tests to skip more than 2000
>     > >         tests on
>     > >         radeonsi (GL 2.1, GLSL 1.2). Almost all of the skipped tests
>     > >         were
>     > >         previously passing.
>     > >
>     > >         Was this intended?
>     > >
>     > >
>     > > No, this wasn't expected, and probably indicates a bug in the driver.
>     > >
>     > > The specific behavioural change in Piglit is that previously, when
>     > > running a test that required GLSL 1.20, Piglit would request (via
>     > > Waffle) a GL 1.0 context.  Then it would query the context to see if
>     > > it supported GLSL 1.20.  After this patch, when running a test that
>     > > requires GLSL 1.20, Piglit requests a GL 2.1 context, and then skips
>     > > if a GL 2.1 context can't be created.  On an implementation that
>     > > supports GL 2.1 and GLSL 1.20, both methods should yield the same
>     > > results.
>     > >
>     > >
>     > > Note that there is code in core Mesa to determine, after context
>     > > creation, which versions of GL and GLSL are supported.  But at the
>     > > time of context creation, it is the responsibility of driver-specific
>     > > code to determine whether the requested context version is supported
>     > > (see, for instance, validate_context_version() in
>     > > src/mesa/drivers/dri/intel/intel_context.c).  This patch caused piglit
>     > > to base its decision for whether to skip on the driver-specific code
>     > > rather than the core Mesa code, so if there was a lurking bug in the
>     > > driver-specific code, this patch would have exposed it.
>     > >
>     > >
>     > > So my suspicion is that there's a bug in the driver-specific context
>     > > creation code causing it to incorrectly reject requests for GL 2.1
>     > > contexts.  Can you check if that's what's happening?  I don't know my
>     > > way around the radeon context creation code well enough to check it
>     > > myself.
>     >
>     > AFAICT the Gallium DRI/OpenGL state trackers should handle that
>     > correctly. The problem seems to be that I'm testing on xserver 1.12,
>     > which doesn't support GLX_ARB_create_context. Is it really no longer
>     > feasible to run those tests without it?
> 
>     In particular, creating a 2.1 context doesn't really require
>     GLX_ARB_create_context, does it?
> 
> 
> Sorry for not responding sooner--yesterday was unexpectedly busy.  You are
> right, creating a 2.1 context shouldn't require GLX_ARB_create_context.
> 
> It seems to me that the correct fix here is to have either the Piglit framework
> or Waffle (I'm not sure which) detect the situation where GLX_ARB_create_context
> is unsupported, and in that case do the following:
> 
> (a) If the test is trying to create a "compatibility" context with GL version
> 1.0, go ahead and create an ordinary context without using GLX_ARB_create_context.
> 
> (b) If the test is trying to create a "compatibility" context with GL version
> other than 1.0, go ahead and create an ordinary context without using
> GLX_ARB_create_context, and then check using glGetString() whether a
> sufficiently high version of GL was returned.  If not, skip the test.
> 
> (c) If the test is trying tocreate a core context, skip.
> 
> The Piglit framework and Waffle collectively implement behaviours (a) and (c)
> already; all that would need to be added is (b).
> 
> Chad, does this seem like a reasonable proposal to you?  If so, where do you
> think it's appropriate to implement (Piglit framework or Waffle)?

It seems reasonable. Part b must be implemented in Piglit because to call
glGetString we must first call glXMakeCurrent, which waffle shouldn't do.

And this should fix regressions for everyone, both Mesa and proprietary drivers.