On 23/04/15 15:12, Arthur Huillet wrote:
Hi,
On Thu, 23 Apr 2015 14:26:32 +0300
Martin Peres <martin.pe...@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 think we already ask piglit developers to quote the spec at the
beginning of their tests. I certainly tried to do that on my tests and
only test the behaviour described in this test. If it has been done,
then I guess the review was a problem here. It would seem like multiple
months passed between the moment the patches were written and the moment
they got commited. I fear that no-one wanted to do a good review so it
just got pushed.
Sometimes, it is good for readability to re-quote the spec in-line or
move part of the spec documentation where it is tested (like in this
test [1]), but this is not always working or possible, like in this test
[2].
In any case, we indeed need to be careful about these errors.
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).
Please do :)
Seems like the test has been written to check for no change in behaviour
with between the DSA and non-DSA paths instead of a close look at the
documentation.
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."
We should not test for unspecified behaviour. That also deserves a patch
to get rid of the check altogether.
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. :)
Ah ah, indeed!
I assumed that the tests were written by the windows engineers and
suggested that they could piglit too. If you also write tests from
Linux, then you have even less excuses not to make them public in piglit :p
[1]
http://cgit.freedesktop.org/piglit/commit/?id=8c1d2e8bac5d3526091ca911dd5d4ed0f859d392
[2]
http://cgit.freedesktop.org/piglit/commit/?id=fa7c2ac879962e3922e7e694955c6879156b7d45
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
