Quoting Alejandro Piñeiro (2018-06-27 03:43:35) > On 26/06/18 17:35, Dylan Baker wrote: > > Quoting Alejandro Piñeiro (2018-06-26 03:44:53) > >> On 25/06/18 17:59, Dylan Baker wrote: > >>> Quoting Alejandro Piñeiro (2018-06-23 04:26:33) > >>>> On 22/06/18 19:14, Dylan Baker wrote: > >>>>> Quoting Alejandro Piñeiro (2018-06-20 05:40:38) > >>>>>> So when executing shader tests, they will be executed with -glsl. This > >>>>>> is the force GLSL mode, that is only relevant if the shader test > >>>>>> includes SPIR-V shaders. > >>>>> I'm not sure I understand what you're doing. It looks like you're > >>>>> planning to > >>>>> build tests that can be run in either SPIRV or GLSL mode, that they can > >>>>> be > >>>>> forced into GLSL mode using a switch, or use SPIRV mode if available. > >>>>> Is that > >>>>> correct? > >>>> Yes. Perhaps the commit message is not really clear, as all the details > >>>> are included on the previous commit "shader_runner/spirv: support > >>>> loading SPIR-V shaders". On that commit we explain that we add a -glsl > >>>> option to shader_runner, that is mostly a debugging option. What this > >>>> commits adds is adding the -glsl option when running the tests in a > >>>> batch. With this series, the -glsl option would only make sense with the > >>>> ARB_gl_spirv tests we are adding. > >>>> > >>>> Do you think that I should update the commit message to be more clear? > >>>> > >>>> BR > >>>> > >>> My concern is that you could end up with two tests with the same name that > >>> aren't the same (I think), since the --glsl option won't change the name > >>> of the > >>> test. > >> Right now, the idea behind the --glsl option is not getting two tests > >> from the same executable. The tests included with this series are > >> expected to run on SPIR-V mode. So for example, we would not add a new > >> entry on opengl.py/shader.py to run those tests twice. As mentioned it > >> is mostly a debug utility/sanity check, that was useful when we were > >> working on getting them passed, so we understand that will be still > >> useful on the future, for any other driver interested on support the > >> extension, or even for i965 again, if there is any future regression. > >> > >> Perhaps you are thinking that we plan to use that option, or similar, to > >> reuse tests from other extensions. What I called "borrowed tests" on the > >> RFC I sent some months ago. For that case, our plan would be generate > >> new tests to be placed under generated_tests. So in the case of reusing > >> tests from other extensions, we would still have two different base > >> tests (one for GLSL, other for SPIRV). > >> > >> BR > > That sounds reasonable. My concern was that someone would see tests failing > > because they lacked spirv tools, and run with --glsl, then give that to a > > second > > person who did have spriv-tools, and see different results. > > Well, fwiw, that option is just useful, not totally needed. So if you > are really worried about people not using it properly, we can just drop > this patch. After all, usually when you are debugging, you do it with a > individual test, so it is enough if shader_runner has that option.
I'm fine with it as is, if I asked for a change what I think I'd ask for is that the -glsl option would change the name of the test so that if you compare them they don't line up. But I don't think that's going to be a real issue, and you can leave it as-is. Dylan
signature.asc
Description: signature
_______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit