On 25 September 2015 12:54:38 am AEST, "Samuel Iglesias Gonsálvez" <sigles...@igalia.com> wrote: > > >On 24/09/15 02:04, Timothy Arceri wrote: >> On Wed, 2015-09-23 at 17:02 +0200, Antía Puentes wrote: >>> Hi! Timothy, >>> thanks for your review. >>> >>> Seeing your patch in: >>> >http://lists.freedesktop.org/archives/mesa-dev/2015-September/095070. >>> html >>> >>> The condition in my patch: >>> (var->type->interface_packing != GLSL_INTERFACE_PACKING_PACKED) >>> should also be changed to: >>> (var->get_interface_type()->interface_packing != >>> GLSL_INTERFACE_PACKING_PACKED) >>> >>> Right? >> >> You don't really need to do that because var is already the variable >> not the array and we know its the interface not an interface member >> because of the conditions above your change. >> > >'var->type' refers to the array of interface block here but the >interface packing information is hold inside the interface block type. > >Because of that, we need to use >var->get_interface_type()->interface_packing to get the valid >information for the condition, so we don't do the wrong thing. > >I even ran a GDB session locally with one of my programs to be sure >before replying :-) > >If you agree, we are going to do that change before pushing the patch >to >master.
After a second look I agree, this does need to change. Thanks for double checking. > >Sam > >[0] >http://lists.freedesktop.org/archives/mesa-dev/2015-September/094009.html > >> >>> >>> On mié, 2015-09-23 at 23:44 +1000, Timothy Arceri wrote: >>>> On Wed, 2015-09-23 at 12:25 +0200, Samuel Iglesias Gonsálvez wrote: >>>>> >>>>> On 21/09/15 13:11, Samuel Iglesias Gonsálvez wrote: >>>>>> On 21/09/15 09:41, Tapani Pälli wrote: >>>>>>> Seems like a nice fix, takes ES3 CTS failures from 116 to 64 >>>>>>> on >>>>>>> Haswell >>>>>>> (most failing tests are with ubos). >>>>>>> >>>>>>> Tested-by: Tapani Pälli <tapani.pa...@intel.com> >>>>>>> >>>>>> >>>>>> OK thanks! >>>>>> >>>>>>> This is individual patch not related to just SSBOs, maybe >>>>>>> this >>>>>>> could be >>>>>>> pushed separately from the rest? >>>>>>> >>>>>> >>>>>> Yes, this patch can be pushed separately from the rest of >>>>>> patches >>>>>> of the >>>>>> series: we just need an R-b, as usual. >>>>>> >>>>>> We are going to discuss the proper solution with Timothy [0], >>>>>> as he >>>>>> found that we are not covering one case. >>>>>> >>>>> >>>>> Timothy has sent a patch fixing the packed case [0] and he >>>>> developed >>>>> a >>>>> piglit test for it [1]. >>>>> >>>>> We are going to wait for an R-b of Antía's patch before pushing >>>>> it. >>>> >>>> I sent a reply to this same email saying almost the same thing but >>>> it >>>> seems to have gone missing. >>>> >>>> Anyway I also sent my r-b, this patch looks good to me. >>>> >>>> Reviewed-by: Timothy Arceri <t_arc...@yahoo.com.au> >>>> >>>> >>>>> >>>>> Sam >>>>> >>>>> [0] >>>>> http://lists.freedesktop.org/archives/mesa-dev/2015-September/095 >>>>> 070. >>>>> html >>>>> [1] >>>>> http://lists.freedesktop.org/archives/piglit/2015-September/01724 >>>>> 7.html >>>>> >>>>>> Sam >>>>>> >>>>>> [0] https://bugs.freedesktop.org/show_bug.cgi?id=83508 >>>>>> >>>>>> >>>>>>> // Tapani >>>>>>> >>>>>>> On 09/10/2015 04:36 PM, Iago Toral Quiroga wrote: >>>>>>>> From: Antia Puentes <apuen...@igalia.com> >>>>>>>> >>>>>>>> Commit 1ca25ab (glsl: Do not eliminate 'shared' or 'std140' >>>>>>>> blocks >>>>>>>> or block members) considered as active 'shared' and >>>>>>>> 'std140' >>>>>>>> uniform >>>>>>>> blocks and uniform block arrays, but did not include the >>>>>>>> block >>>>>>>> array >>>>>>>> elements. Because of that, it was possible to have an >>>>>>>> active >>>>>>>> uniform >>>>>>>> block array without any elements marked as used, making the >>>>>>>> assertion >>>>>>>> ((b->num_array_elements > 0) == b->type->is_array()) >>>>>>>> in link_uniform_blocks() fail. >>>>>>>> >>>>>>>> Fixes the following 5 dEQP tests: >>>>>>>> >>>>>>>> * dEQP >>>>>>>> -GLES3.functional.ubo.random.nested_structs_instance_arrays >>>>>>>> .18 >>>>>>>> * dEQP >>>>>>>> -GLES3.functional.ubo.random.nested_structs_instance_arrays >>>>>>>> .24 >>>>>>>> * >>>>>>>> dEQP >>>>>>>> -GLES3.functional.ubo.random.nested_structs_arrays_instance >>>>>>>> _arr >>>>>>>> ays.19 >>>>>>>> * dEQP >>>>>>>> -GLES3.functional.ubo.random.all_per_block_buffers.49 >>>>>>>> * dEQP-GLES3.functional.ubo.random.all_shared_buffer.36 >>>>>>>> >>>>>>>> Fixes bugzilla: >>>>>>>> https://bugs.freedesktop.org/show_bug.cgi?id=83508 >>>>>>>> --- >>>>>>>> src/glsl/link_uniform_block_active_visitor.cpp | 23 >>>>>>>> +++++++++++++++++++++++ >>>>>>>> 1 file changed, 23 insertions(+) >>>>>>>> >>>>>>>> diff --git a/src/glsl/link_uniform_block_active_visitor.cpp >>>>>>>> b/src/glsl/link_uniform_block_active_visitor.cpp >>>>>>>> index 5102947..fbe79de 100644 >>>>>>>> --- a/src/glsl/link_uniform_block_active_visitor.cpp >>>>>>>> +++ b/src/glsl/link_uniform_block_active_visitor.cpp >>>>>>>> @@ -106,6 +106,22 @@ >>>>>>>> link_uniform_block_active_visitor::visit(ir_variable *var) >>>>>>>> assert(b->num_array_elements == 0); >>>>>>>> assert(b->array_elements == NULL); >>>>>>>> assert(b->type != NULL); >>>>>>>> + assert(!b->type->is_array() || b->has_instance_name); >>>>>>>> + >>>>>>>> + /* For uniform block arrays declared with a shared or >>>>>>>> std140 layout >>>>>>>> + * qualifier, mark all its instances as used. >>>>>>>> + */ >>>>>>>> + if (b->type->is_array() && b->type->length > 0) { >>>>>>>> + b->num_array_elements = b->type->length; >>>>>>>> + b->array_elements = reralloc(this->mem_ctx, >>>>>>>> + b->array_elements, >>>>>>>> + unsigned, >>>>>>>> + b->num_array_elements); >>>>>>>> + >>>>>>>> + for (unsigned i = 0; i < b->num_array_elements; i++) >>>>>>>> { >>>>>>>> + b->array_elements[i] = i; >>>>>>>> + } >>>>>>>> + } >>>>>>>> >>>>>>>> return visit_continue; >>>>>>>> } >>>>>>>> @@ -147,6 +163,13 @@ >>>>>>>> link_uniform_block_active_visitor::visit_enter(ir_dereferen >>>>>>>> ce_a >>>>>>>> rray *ir) >>>>>>>> assert((b->num_array_elements == 0) == (b >>>>>>>> ->array_elements >>>>>>>> == NULL)); >>>>>>>> assert(b->type != NULL); >>>>>>>> >>>>>>>> + /* If the block array was declared with a shared or >>>>>>>> + * std140 layout qualifier, all its instances have been >>>>>>>> already >>>>>>>> marked >>>>>>>> + * as used in >>>>>>>> link_uniform_block_active_visitor::visit(ir_variable >>>>>>>> *). >>>>>>>> + */ >>>>>>>> + if (var->type->interface_packing != >>>>>>>> GLSL_INTERFACE_PACKING_PACKED) >>>>>>>> + return visit_continue_with_parent; >>>>>>>> + >>>>>>>> ir_constant *c = ir->array_index->as_constant(); >>>>>>>> >>>>>>>> if (c) { >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> 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 >>>>>> >>>>> _______________________________________________ >>>>> 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 >>> >>> >> -- Sent from my Android device with K-9 Mail. Please excuse my brevity. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
