Re: [Mesa-dev] [PATCH] glsl: properly setting var-data.binding if explicit_binding is true
Timothy Arceri wrote
On Sun, 2015-04-26 at 15:23 +0200, Alejandro Piñeiro wrote:
On 26/04/15 00:08, Timothy Arceri wrote:
On Sat, 2015-04-25 at 18:46 +0200, Alejandro Piñeiro wrote:
There was a typo on commit c0cd5b, doing it when explicit_binding
was false. This prevented to use any binding point different to 0.
---
Taking into account the explanation on the header about the
variable binding (ast.h:553)
/**
* Binding specified via GL_ARB_shading_language_420pack's
binding keyword.
*
* \note
* This field is only valid if \c explicit_binding is set.
*/
int binding;
The binding is correct (and should be updated) if explicit_binding
is true.
But the current behaviour was updating it if it was false.
This was not detected by piglit because all the calls to
glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER are using 0.
I tested this patch by running all piglit on my system, and I
didn't
detect regression. I also runned make check without issues.
https://bugs.freedesktop.org/show_bug.cgi?id=90175
You should probably convert your test program to a piglit test also
so
this bug can be detected if it happens again in the future.
There are several piglit tests at spec/arb_shader_atomic_counters
testing that you get what you expect while using atomic counters.
Probably it would be enough to just modify some of the already
existing
tests, using a non-zero binding point (for example at semantics.c).
But I don't have too much experience tweaking/creating piglit tests.
What option would be preferred for this case? A new test or just
modify
a little one of the already available?
Take a look at buffer-binding.c it would probably make sense to add your
subtest to this test.
Your test would probably look something like this:
static bool
run_test_explicit_binding(test params here)
{
/* test code */
if (explict_binding_set_incorrectly)
return false;
return true;
}
And you would add something like this to piglit_init()*/
if (piglit_is_extension_supported(GL_ARB_shading_language_420pack))
My bad you don't need the above if.
atomic_counters_subtest(status, GL_NONE,
Atomic buffer explicit binding
run_test_explicit_binding,
}
You would also need to update the comment at the top of the file.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: properly setting var-data.binding if explicit_binding is true
On Sun, 2015-04-26 at 15:23 +0200, Alejandro Piñeiro wrote:
On 26/04/15 00:08, Timothy Arceri wrote:
On Sat, 2015-04-25 at 18:46 +0200, Alejandro Piñeiro wrote:
There was a typo on commit c0cd5b, doing it when explicit_binding
was false. This prevented to use any binding point different to 0.
---
Taking into account the explanation on the header about the
variable binding (ast.h:553)
/**
* Binding specified via GL_ARB_shading_language_420pack's
binding keyword.
*
* \note
* This field is only valid if \c explicit_binding is set.
*/
int binding;
The binding is correct (and should be updated) if explicit_binding
is true.
But the current behaviour was updating it if it was false.
This was not detected by piglit because all the calls to
glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER are using 0.
I tested this patch by running all piglit on my system, and I
didn't
detect regression. I also runned make check without issues.
https://bugs.freedesktop.org/show_bug.cgi?id=90175
You should probably convert your test program to a piglit test also
so
this bug can be detected if it happens again in the future.
There are several piglit tests at spec/arb_shader_atomic_counters
testing that you get what you expect while using atomic counters.
Probably it would be enough to just modify some of the already
existing
tests, using a non-zero binding point (for example at semantics.c).
But I don't have too much experience tweaking/creating piglit tests.
What option would be preferred for this case? A new test or just
modify
a little one of the already available?
Take a look at buffer-binding.c it would probably make sense to add your
subtest to this test.
Your test would probably look something like this:
static bool
run_test_explicit_binding(test params here)
{
/* test code */
if (explict_binding_set_incorrectly)
return false;
return true;
}
And you would add something like this to piglit_init()*/
if (piglit_is_extension_supported(GL_ARB_shading_language_420pack))
atomic_counters_subtest(status, GL_NONE,
Atomic buffer explicit binding
run_test_explicit_binding,
}
You would also need to update the comment at the top of the file.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: properly setting var-data.binding if explicit_binding is true
Hi Alejandro On 26 April 2015 at 14:23, Alejandro Piñeiro apinhe...@igalia.com wrote: On 26/04/15 00:08, Timothy Arceri wrote: On Sat, 2015-04-25 at 18:46 +0200, Alejandro Piñeiro wrote: There was a typo on commit c0cd5b, doing it when explicit_binding was false. This prevented to use any binding point different to 0. Considering that the offending commit landed in 10.4-devel, please add the following line in the commit message. This way we won't forget picking this up for the stable branches. Cc: 10.4 10.5 mesa-sta...@lists.freedesktop.org --- Taking into account the explanation on the header about the variable binding (ast.h:553) /** * Binding specified via GL_ARB_shading_language_420pack's binding keyword. * * \note * This field is only valid if \c explicit_binding is set. */ int binding; The binding is correct (and should be updated) if explicit_binding is true. But the current behaviour was updating it if it was false. This was not detected by piglit because all the calls to glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER are using 0. I tested this patch by running all piglit on my system, and I didn't detect regression. I also runned make check without issues. https://bugs.freedesktop.org/show_bug.cgi?id=90175 Please move the bug entry to the commit message and prefix with Bugzilla: You should probably convert your test program to a piglit test also so this bug can be detected if it happens again in the future. There are several piglit tests at spec/arb_shader_atomic_counters testing that you get what you expect while using atomic counters. Probably it would be enough to just modify some of the already existing tests, using a non-zero binding point (for example at semantics.c). But I don't have too much experience tweaking/creating piglit tests. What option would be preferred for this case? A new test or just modify a little one of the already available? Don't think that changing the behaviour of existing tests is a good idea. As Timothy mentioned, you can add a subtest which will cut down the copy/pasta a bit. Cheers, Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: properly setting var-data.binding if explicit_binding is true
On 26/04/15 00:08, Timothy Arceri wrote: On Sat, 2015-04-25 at 18:46 +0200, Alejandro Piñeiro wrote: There was a typo on commit c0cd5b, doing it when explicit_binding was false. This prevented to use any binding point different to 0. --- Taking into account the explanation on the header about the variable binding (ast.h:553) /** * Binding specified via GL_ARB_shading_language_420pack's binding keyword. * * \note * This field is only valid if \c explicit_binding is set. */ int binding; The binding is correct (and should be updated) if explicit_binding is true. But the current behaviour was updating it if it was false. This was not detected by piglit because all the calls to glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER are using 0. I tested this patch by running all piglit on my system, and I didn't detect regression. I also runned make check without issues. https://bugs.freedesktop.org/show_bug.cgi?id=90175 You should probably convert your test program to a piglit test also so this bug can be detected if it happens again in the future. There are several piglit tests at spec/arb_shader_atomic_counters testing that you get what you expect while using atomic counters. Probably it would be enough to just modify some of the already existing tests, using a non-zero binding point (for example at semantics.c). But I don't have too much experience tweaking/creating piglit tests. What option would be preferred for this case? A new test or just modify a little one of the already available? Best regards -- Alejandro Piñeiro (apinhe...@igalia.com) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: properly setting var-data.binding if explicit_binding is true
On Sat, 2015-04-25 at 18:46 +0200, Alejandro Piñeiro wrote: There was a typo on commit c0cd5b, doing it when explicit_binding was false. This prevented to use any binding point different to 0. --- Taking into account the explanation on the header about the variable binding (ast.h:553) /** * Binding specified via GL_ARB_shading_language_420pack's binding keyword. * * \note * This field is only valid if \c explicit_binding is set. */ int binding; The binding is correct (and should be updated) if explicit_binding is true. But the current behaviour was updating it if it was false. This was not detected by piglit because all the calls to glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER are using 0. I tested this patch by running all piglit on my system, and I didn't detect regression. I also runned make check without issues. https://bugs.freedesktop.org/show_bug.cgi?id=90175 You should probably convert your test program to a piglit test also so this bug can be detected if it happens again in the future. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl: properly setting var-data.binding if explicit_binding is true
There was a typo on commit c0cd5b, doing it when explicit_binding was false. This prevented to use any binding point different to 0. --- Taking into account the explanation on the header about the variable binding (ast.h:553) /** * Binding specified via GL_ARB_shading_language_420pack's binding keyword. * * \note * This field is only valid if \c explicit_binding is set. */ int binding; The binding is correct (and should be updated) if explicit_binding is true. But the current behaviour was updating it if it was false. This was not detected by piglit because all the calls to glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER are using 0. I tested this patch by running all piglit on my system, and I didn't detect regression. I also runned make check without issues. https://bugs.freedesktop.org/show_bug.cgi?id=90175 src/glsl/link_atomics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp index 603873a..9528e42 100644 --- a/src/glsl/link_atomics.cpp +++ b/src/glsl/link_atomics.cpp @@ -201,7 +201,7 @@ link_assign_atomic_counter_resources(struct gl_context *ctx, gl_uniform_storage *const storage = prog-UniformStorage[id]; mab.Uniforms[j] = id; - if (!var-data.explicit_binding) + if (var-data.explicit_binding) var-data.binding = i; storage-atomic_buffer_index = i; -- 2.1.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
