[v2,1/6] texturing: make sure s3tc tests require GL 1.1

Submitted by Erik Faye-Lund on Oct. 26, 2018, 1:03 p.m.

Details

Message ID 20181026130308.12519-2-erik.faye-lund@collabora.com
State New
Headers show

Not browsing as part of any series.

Patch hide | download patch | download mbox

diff --git a/tests/texturing/s3tc-errors.c b/tests/texturing/s3tc-errors.c
index 81fee3e21..ccedf8a22 100644
--- a/tests/texturing/s3tc-errors.c
+++ b/tests/texturing/s3tc-errors.c
@@ -36,7 +36,7 @@ 
 
 PIGLIT_GL_TEST_CONFIG_BEGIN
 
-    config.supports_gl_compat_version = 10;
+    config.supports_gl_compat_version = 11;
 
     config.window_width = 200;
     config.window_height = 200;
diff --git a/tests/texturing/s3tc-teximage.c b/tests/texturing/s3tc-teximage.c
index 07e04f729..a08feaa69 100644
--- a/tests/texturing/s3tc-teximage.c
+++ b/tests/texturing/s3tc-teximage.c
@@ -36,7 +36,7 @@ 
 
 PIGLIT_GL_TEST_CONFIG_BEGIN
 
-	config.supports_gl_compat_version = 10;
+	config.supports_gl_compat_version = 11;
 
 	config.window_width = 500;
 	config.window_height = 600;
diff --git a/tests/texturing/s3tc-texsubimage.c b/tests/texturing/s3tc-texsubimage.c
index 17d739a39..39b94938e 100644
--- a/tests/texturing/s3tc-texsubimage.c
+++ b/tests/texturing/s3tc-texsubimage.c
@@ -36,7 +36,7 @@ 
 
 PIGLIT_GL_TEST_CONFIG_BEGIN
 
-	config.supports_gl_compat_version = 10;
+	config.supports_gl_compat_version = 11;
 
 	config.window_width = 500;
 	config.window_height = 600;

Comments

On Fri, 26 Oct 2018 at 14:03, Erik Faye-Lund
<erik.faye-lund@collabora.com> wrote:
>
> The EXT_texture_compression_s3tc spec lists that it requires
> OpenGL 1.1, not 1.0 like some of these list.
>
> In reality, this probably doesn't make a huge difference, as
> OpenGL 1.0 hardware/drivers are pretty much extinct, but let's
> just get it right to avoid confusion.
>
> Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
> ---
>  tests/texturing/s3tc-errors.c      | 2 +-
>  tests/texturing/s3tc-teximage.c    | 2 +-
>  tests/texturing/s3tc-texsubimage.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/texturing/s3tc-errors.c b/tests/texturing/s3tc-errors.c
> index 81fee3e21..ccedf8a22 100644
> --- a/tests/texturing/s3tc-errors.c
> +++ b/tests/texturing/s3tc-errors.c
> @@ -36,7 +36,7 @@
>
>  PIGLIT_GL_TEST_CONFIG_BEGIN
>
> -    config.supports_gl_compat_version = 10;
> +    config.supports_gl_compat_version = 11;
>
Ages ago Ilia reported on IRC that some tests (was it 10 or 11, don't
recall) failing on the classic swrast and/or nouveau_vieux.
That waffle issue should be fixed, although if he can give this patch
a try (with platform glx) that'll be appreciated ...

Gut feeling suggests that the fix may not have been enough.

-Emil
On Fri, 2018-10-26 at 16:21 +0100, Emil Velikov wrote:
> On Fri, 26 Oct 2018 at 14:03, Erik Faye-Lund
> <erik.faye-lund@collabora.com> wrote:
> > 
> > The EXT_texture_compression_s3tc spec lists that it requires
> > OpenGL 1.1, not 1.0 like some of these list.
> > 
> > In reality, this probably doesn't make a huge difference, as
> > OpenGL 1.0 hardware/drivers are pretty much extinct, but let's
> > just get it right to avoid confusion.
> > 
> > Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
> > ---
> >  tests/texturing/s3tc-errors.c      | 2 +-
> >  tests/texturing/s3tc-teximage.c    | 2 +-
> >  tests/texturing/s3tc-texsubimage.c | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/texturing/s3tc-errors.c b/tests/texturing/s3tc-
> > errors.c
> > index 81fee3e21..ccedf8a22 100644
> > --- a/tests/texturing/s3tc-errors.c
> > +++ b/tests/texturing/s3tc-errors.c
> > @@ -36,7 +36,7 @@
> > 
> >  PIGLIT_GL_TEST_CONFIG_BEGIN
> > 
> > -    config.supports_gl_compat_version = 10;
> > +    config.supports_gl_compat_version = 11;
> > 
> 
> Ages ago Ilia reported on IRC that some tests (was it 10 or 11, don't
> recall) failing on the classic swrast and/or nouveau_vieux.
> That waffle issue should be fixed, although if he can give this patch
> a try (with platform glx) that'll be appreciated ...
> 
> Gut feeling suggests that the fix may not have been enough.

I don't think I understand the issue... Some of these tests use 10 and
some use 11 here, and 11 is the correct value (according to the spec).
I'm just making it consistent...

This is just a filter under what contexts these tests will run, no? I
haven't seen an OpenGL 1.0-only implementation, well, ever I think?
Isn't this just theoretical anyway? I don't understand how this could
have any practical effect...
On Fri, 26 Oct 2018 at 16:39, Erik Faye-Lund
<erik.faye-lund@collabora.com> wrote:
>
> On Fri, 2018-10-26 at 16:21 +0100, Emil Velikov wrote:
> > On Fri, 26 Oct 2018 at 14:03, Erik Faye-Lund
> > <erik.faye-lund@collabora.com> wrote:
> > >
> > > The EXT_texture_compression_s3tc spec lists that it requires
> > > OpenGL 1.1, not 1.0 like some of these list.
> > >
> > > In reality, this probably doesn't make a huge difference, as
> > > OpenGL 1.0 hardware/drivers are pretty much extinct, but let's
> > > just get it right to avoid confusion.
> > >
> > > Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
> > > ---
> > >  tests/texturing/s3tc-errors.c      | 2 +-
> > >  tests/texturing/s3tc-teximage.c    | 2 +-
> > >  tests/texturing/s3tc-texsubimage.c | 2 +-
> > >  3 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tests/texturing/s3tc-errors.c b/tests/texturing/s3tc-
> > > errors.c
> > > index 81fee3e21..ccedf8a22 100644
> > > --- a/tests/texturing/s3tc-errors.c
> > > +++ b/tests/texturing/s3tc-errors.c
> > > @@ -36,7 +36,7 @@
> > >
> > >  PIGLIT_GL_TEST_CONFIG_BEGIN
> > >
> > > -    config.supports_gl_compat_version = 10;
> > > +    config.supports_gl_compat_version = 11;
> > >
> >
> > Ages ago Ilia reported on IRC that some tests (was it 10 or 11, don't
> > recall) failing on the classic swrast and/or nouveau_vieux.
> > That waffle issue should be fixed, although if he can give this patch
> > a try (with platform glx) that'll be appreciated ...
> >
> > Gut feeling suggests that the fix may not have been enough.
>
> I don't think I understand the issue... Some of these tests use 10 and
> some use 11 here, and 11 is the correct value (according to the spec).
> I'm just making it consistent...
>
> This is just a filter under what contexts these tests will run, no? I
> haven't seen an OpenGL 1.0-only implementation, well, ever I think?
> Isn't this just theoretical anyway? I don't understand how this could
> have any practical effect...
>
It's been ~3 years so, my memory and amount of details is flaky.

A while ago Ilia and Brian Paul reported that some tests annotated as
10 (or was it 11)  were failing on nouveau_vieux and classic swrast
respectively.
That was because piglit was passing the explicit version to waffle,
which in turn was using the GLX_ARB_create_context since it's the only
way to request a specific version.

That should be addressed with the following waffle commit [1],
although I'm concerned about the 10 check in the patch.
Sadly I did not document why in the commit log (nor was it asked
during review), perhaps it has something to do with the massive
comment in waffle's glx_context.c [2].

That said, if your patch causes problems I could double-check and fix waffle.
Since I've annoyed Ilia enough times, I wanted to spare another
instance - hence the suggestion.

HTH
Emil

[1] https://github.com/waffle-gl/waffle/commit/e7f0314c7c87e031148d7dee0e6f28d490a97a2a
[2] https://github.com/waffle-gl/waffle/blob/e7f0314c7c87e031148d7dee0e6f28d490a97a2a/src/waffle/glx/glx_context.c#L86
On Fri, 2018-10-26 at 17:36 +0100, Emil Velikov wrote:
> On Fri, 26 Oct 2018 at 16:39, Erik Faye-Lund
> <erik.faye-lund@collabora.com> wrote:
> > 
> > On Fri, 2018-10-26 at 16:21 +0100, Emil Velikov wrote:
> > > On Fri, 26 Oct 2018 at 14:03, Erik Faye-Lund
> > > <erik.faye-lund@collabora.com> wrote:
> > > > 
> > > > The EXT_texture_compression_s3tc spec lists that it requires
> > > > OpenGL 1.1, not 1.0 like some of these list.
> > > > 
> > > > In reality, this probably doesn't make a huge difference, as
> > > > OpenGL 1.0 hardware/drivers are pretty much extinct, but let's
> > > > just get it right to avoid confusion.
> > > > 
> > > > Signed-off-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
> > > > ---
> > > >  tests/texturing/s3tc-errors.c      | 2 +-
> > > >  tests/texturing/s3tc-teximage.c    | 2 +-
> > > >  tests/texturing/s3tc-texsubimage.c | 2 +-
> > > >  3 files changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/tests/texturing/s3tc-errors.c
> > > > b/tests/texturing/s3tc-
> > > > errors.c
> > > > index 81fee3e21..ccedf8a22 100644
> > > > --- a/tests/texturing/s3tc-errors.c
> > > > +++ b/tests/texturing/s3tc-errors.c
> > > > @@ -36,7 +36,7 @@
> > > > 
> > > >  PIGLIT_GL_TEST_CONFIG_BEGIN
> > > > 
> > > > -    config.supports_gl_compat_version = 10;
> > > > +    config.supports_gl_compat_version = 11;
> > > > 
> > > 
> > > Ages ago Ilia reported on IRC that some tests (was it 10 or 11,
> > > don't
> > > recall) failing on the classic swrast and/or nouveau_vieux.
> > > That waffle issue should be fixed, although if he can give this
> > > patch
> > > a try (with platform glx) that'll be appreciated ...
> > > 
> > > Gut feeling suggests that the fix may not have been enough.
> > 
> > I don't think I understand the issue... Some of these tests use 10
> > and
> > some use 11 here, and 11 is the correct value (according to the
> > spec).
> > I'm just making it consistent...
> > 
> > This is just a filter under what contexts these tests will run, no?
> > I
> > haven't seen an OpenGL 1.0-only implementation, well, ever I think?
> > Isn't this just theoretical anyway? I don't understand how this
> > could
> > have any practical effect...
> > 
> 
> It's been ~3 years so, my memory and amount of details is flaky.
> 
> A while ago Ilia and Brian Paul reported that some tests annotated as
> 10 (or was it 11)  were failing on nouveau_vieux and classic swrast
> respectively.
> That was because piglit was passing the explicit version to waffle,
> which in turn was using the GLX_ARB_create_context since it's the
> only
> way to request a specific version.
> 
> That should be addressed with the following waffle commit [1],
> although I'm concerned about the 10 check in the patch.
> Sadly I did not document why in the commit log (nor was it asked
> during review), perhaps it has something to do with the massive
> comment in waffle's glx_context.c [2].
> 
> That said, if your patch causes problems I could double-check and fix
> waffle.
> Since I've annoyed Ilia enough times, I wanted to spare another
> instance - hence the suggestion.

Right. I gave it a go on classic swrast, and these tests are skipped
there due to lacking GL_EXT_texture_compression_s3tc. I don't have a
Nouveau system to test on.