Re: [Piglit] [PATCH 03/24] framework: add --glsl option
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
Re: [Piglit] [PATCH 03/24] framework: add --glsl option
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. So how about that compromise? We keep the -glsl option on shader_runner, but drop the piglit framework support to run several tests with that option? BR signature.asc Description: OpenPGP digital signature ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 03/24] framework: add --glsl option
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. Dylan signature.asc Description: signature ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 03/24] framework: add --glsl option
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 signature.asc Description: OpenPGP digital signature ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 03/24] framework: add --glsl option
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. Dylan signature.asc Description: signature ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 03/24] framework: add --glsl option
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 signature.asc Description: OpenPGP digital signature ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 03/24] framework: add --glsl option
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? Dylan signature.asc Description: signature ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
