<p dir="ltr"><br>
---- Timothy Arceri wrote ----</p>
<p dir="ltr">> On Sun, 2015-04-26 at 15:23 +0200, Alejandro Piñeiro wrote:<br>
> On 26/04/15 00:08, Timothy Arceri wrote:<br>
> > > On Sat, 2015-04-25 at 18:46 +0200, Alejandro Piñeiro wrote:<br>
> > >> There was a typo on commit c0cd5b, doing it when explicit_binding<br>
> > >> was false. This prevented to use any binding point different to 0.<br>
> > >> ---<br>
> > >><br>
> > >> Taking into account the explanation on the header about the<br>
> > >> variable binding (ast.h:553)<br>
> > >><br>
> > >>    /**<br>
> > >>     * Binding specified via GL_ARB_shading_language_420pack's<br>
> "binding" keyword.<br>
> > >>     *<br>
> > >>     * \note<br>
> > >>     * This field is only valid if \c explicit_binding is set.<br>
> > >>     */<br>
> > >>    int binding;<br>
> > >><br>
> > >> The binding is correct (and should be updated) if explicit_binding<br>
> is true.<br>
> > >> But the current behaviour was updating it if it was false. <br>
> > >><br>
> > >> This was not detected by piglit because all the calls to<br>
> > >> glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER are using 0.<br>
> > >><br>
> > >> I tested this patch by running all piglit on my system, and I<br>
> didn't<br>
> > >> detect regression. I also runned make check without issues.<br>
> > >><br>
> > >> <a href="https://bugs.freedesktop.org/show_bug.cgi?id=90175">https://bugs.freedesktop.org/show_bug.cgi?id=90175</a><br>
> > > You should probably convert your test program to a piglit test also<br>
> so<br>
> > > this bug can be detected if it happens again in the future.<br>
> > <br>
> > There are several piglit tests at spec/arb_shader_atomic_counters<br>
> > testing that you get what you expect while using atomic counters.<br>
> > Probably it would be enough to just modify some of the already<br>
> existing<br>
> > tests, using a non-zero binding point (for example at semantics.c).<br>
> > <br>
> > But I don't have too much experience tweaking/creating piglit tests.<br>
> > What option would be preferred for this case? A new test or just<br>
> modify<br>
> > a little one of the already available?<br>
> ><br>
> <br>
> Take a look at buffer-binding.c it would probably make sense to add your<br>
> subtest to this test.<br>
> <br>
> Your test would probably look something like this:<br>
> <br>
> static bool<br>
> run_test_explicit_binding(test params here)<br>
> {<br>
> <br>
>  /* test code */<br>
> <br>
>  if (explict_binding_set_incorrectly)<br>
>  return false;<br>
> <br>
>  return true;<br>
> }<br>
> <br>
> And you would add something like this to piglit_init()*/<br>
> <br>
> if (piglit_is_extension_supported("GL_ARB_shading_language_420pack"))<br></p>
<p dir="ltr">My bad you don't need the above if.<br></p>
<p dir="ltr">> <br>
>         atomic_counters_subtest(&status, GL_NONE,<br>
>                                 "Atomic buffer explicit binding"<br>
>                                 run_test_explicit_binding, <br>
> }<br>
> <br>
> You would also need to update the comment at the top of the file.<br>
> <br>
> <br>
> _______________________________________________<br>
> mesa-dev mailing list<br>
> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a></p>