[Piglit] [PATCH] DSA: fix error value for *TextureSubImage* when target doesn't match

[Fredrik Höglund]

On Thursday 23 April 2015, Arthur Huillet wrote:
> Hi,
> 
> On Thu, 23 Apr 2015 14:26:32 +0300
> Martin Peres <martin.peres at linux.intel.com> wrote:
> 
> > >>> Does the patch look OK to you?
> > >> Yeah, patch looks perfectly fine. But I'd like to hear from Laura (or
> > >> Martin perhaps, who reviewed a lot of these) about whether this was
> > >> done on purpose or not, pending a spec fix.
> > 
> > I am not aware of any bug report filed by Laura on this. Given that she 
> > wrote this code in October last year, I am sure she would have received 
> > an answer from Khronos if she had filed a bug. This would suggest it is 
> > a bug in the test.
> > 
> > Let's wait for her comment to make sure of it.
> > 
> > Anyway, thanks for the patch Arthur! I know it can be frustrating to 
> > have to check every failing test while wondering whether this is a test 
> > or a driver bug. We should encourage developers writing piglit tests to 
> > try them on multiple drivers that already support the extension they are 
> > testing. This is however impossible to mandate.
> 
> Asking Piglit contributors to test their tests (...) on more than one implementation
> is certainly a good idea, but it's not always practical. I think it is easier to ask,
> for the tests for which it is appropriate (like the ones I fixed), to quote the part
> of the specification that they are testing. This also has the advantage of making 
> the reviewers' life easier - I don't think many people know by heart what error 
> is supposed to be returned by a particular error condition, so having a piece of 
> spec to check is helpful.
> I'll go a little further and suggest that in order to review a "GL_INVALID_*" 
> test properly, you *need* to look at the spec. So better quote it directly.
>  
> Here are more examples from DSA:
> 
> "bind-texture-unit.c:63:
> 
>     glGenTextures(1, &name);
>     glBindTextureUnit(0, name);
>     pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> 
> But here's the spec:
>     "An INVALID_OPERATION error is generated by BindTextureUnit if tex-
> ture is not zero or the name of an existing texture object."
> 
> So I believe INVALID_OPERATION is the correct thing to return (will send a patch
> when I can).
> 
> Then, bind-texture-unit.c:70:
> 
>     /* Texture unit doesn't exist */
> 
>     glDeleteTextures(1, &name);
>     glCreateTextures(GL_TEXTURE_2D, 1, &name);
>     glGetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, &nunits);
>     glBindTextureUnit(nunits, name); /* Too High */
>     pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
> 
> I haven't been able to find the relevant language to justify
> GL_INVALID_OPERATION here. (NVIDIA returns INVALID_VALUE)
> I won't send a patch since I can't back up my claim with spec."

I posted an analysis of these failures here a couple of weeks ago:

http://lists.freedesktop.org/archives/piglit/2015-April/015660.html

> > Do you try your tests against Intel's or AMD's driver for Windows? If 
> > so, why not start working on them in the open and push them to piglit 
> > since it now supports Windows?
> 
> I'm a Linux engineer and if I can avoid working on Windows, I will. :)
> 
>