[Mesa-dev] [PATCH] glsl: properly setting var->data.binding if explicit_binding is true

[Timothy Arceri]

---- 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150427/3d2c5ced/attachment-0001.html>