Re: [Piglit] [PATCH 03/24] framework: add --glsl option

2018-06-27 Thread Dylan Baker
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

2018-06-27 Thread Alejandro Piñeiro
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

2018-06-26 Thread Dylan Baker
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

2018-06-26 Thread Alejandro Piñeiro
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

2018-06-25 Thread Dylan Baker
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

2018-06-23 Thread Alejandro Piñeiro
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

2018-06-22 Thread Dylan Baker
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