<p dir="ltr"><br>
On Jun 30, 2015 1:05 AM, "Iago Toral" <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> wrote:<br>
><br>
> Hi Jason,<br>
><br>
> On Mon, 2015-06-29 at 16:22 -0700, Jason Ekstrand wrote:<br>
> > On Fri, Jun 26, 2015 at 1:06 AM, Eduardo Lima Mitev <<a href="mailto:elima@igalia.com">elima@igalia.com</a>> wrote:<br>
> > > From: Iago Toral Quiroga <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>><br>
> > ><br>
> > > This is based on similar code existing in vec4_visitor. It builds the<br>
> > > uniform register file iterating through each uniform variable. It<br>
> > > also stores the index of each register at the corresponding offset<br>
> > > in a map. This map will later be used by load_uniform intrinsic<br>
> > > instructions to build the correct UNIFORM source register.<br>
> > ><br>
> > > Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=89580">https://bugs.freedesktop.org/show_bug.cgi?id=89580</a><br>
> > > ---<br>
> > >  src/mesa/drivers/dri/i965/brw_vec4.h       |   2 +<br>
> > >  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 115 ++++++++++++++++++++++++++++-<br>
> > >  2 files changed, 114 insertions(+), 3 deletions(-)<br>
> > ><br>
> > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h<br>
> > > index 673df4e..6535f19 100644<br>
> > > --- a/src/mesa/drivers/dri/i965/brw_vec4.h<br>
> > > +++ b/src/mesa/drivers/dri/i965/brw_vec4.h<br>
> > > @@ -414,6 +414,8 @@ public:<br>
> > >     src_reg *nir_inputs;<br>
> > >     int *nir_outputs;<br>
> > >     brw_reg_type *nir_output_types;<br>
> > > +   unsigned *nir_uniform_offset;<br>
> > > +   unsigned *nir_uniform_driver_location;<br>
> > ><br>
> > >  protected:<br>
> > >     void emit_vertex();<br>
> > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp<br>
> > > index 2d457a6..40ec66f 100644<br>
> > > --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp<br>
> > > +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp<br>
> > > @@ -106,19 +106,128 @@ vec4_visitor::nir_setup_outputs(nir_shader *shader)<br>
> > >  void<br>
> > >  vec4_visitor::nir_setup_uniforms(nir_shader *shader)<br>
> > >  {<br>
> > > -   /* @TODO: Not yet implemented */<br>
> > > +   uniforms = 0;<br>
> > > +<br>
> > > +   nir_uniform_offset =<br>
> > > +      rzalloc_array(mem_ctx, unsigned, this->uniform_array_size);<br>
> > > +   memset(nir_uniform_offset, 0, this->uniform_array_size * sizeof(unsigned));<br>
> ><br>
> > rzalloc memsets the whole thing to 0 for you, this memset is redundant.<br>
> ><br>
> > > +<br>
> > > +   nir_uniform_driver_location =<br>
> > > +      rzalloc_array(mem_ctx, unsigned, this->uniform_array_size);<br>
> > > +   memset(nir_uniform_driver_location, 0,<br>
> > > +          this->uniform_array_size * sizeof(unsigned));<br>
> ><br>
> > Same here.<br>
><br>
> Oh, right.<br>
><br>
> > > +<br>
> > > +   if (shader_prog) {<br>
> > > +      foreach_list_typed(nir_variable, var, node, &shader->uniforms) {<br>
> > > +         /* UBO's, atomics and samplers don't take up space in the<br>
> > > +            uniform file */<br>
> > > +         if (var->interface_type != NULL || var->type->contains_atomic() ||<br>
> > > +             type_size(var->type) == 0) {<br>
> ><br>
> > I'm curious as to why you have this extra type_size() == 0 condition.<br>
> > We don't have that in the FS NIR code.  What caused you to add it?<br>
><br>
> Take this piglit test for example:<br>
> bin/textureSize vs isampler1D -auto -fbo<br>
><br>
> here, 'tex' is a uniform of size 0 since type_size() returns 0 for all<br>
> sampler types. If we do not ignore these, we will try to store uniform<br>
> information for them in the various structures we have to track uniform<br>
> data, like uniform_size[] and others. The size allocated for these<br>
> arrays is computed by in the vec4_visitor constructor based on<br>
> stage_prog_data->nr_params (uniform_array_size) and that does not seem<br>
> to make room for zero-sized uniforms. Without that check we would<br>
> process more uniforms than uniform_array_size and overflow the arrays we<br>
> allocate to track uniform information. I understand that<br>
> stage_prog_data->nr_params does not track uniforms that don't use<br>
> register space, so skipping uniforms with no size seems to make sense<br>
> here.<br>
><br>
> Notice that this is done in the current vec4_visitor too, when we visit<br>
> the variable in vec4_visitor::visit(ir_variable *ir), for ir_var_uniform<br>
> there is this code:<br>
><br>
> if (ir->is_in_uniform_block() || type_size(ir->type) == 0)<br>
>     return;</p>
<p dir="ltr">For some reason, I was thinking textures were in their own list but now that I look again, they are in the uniforms list. I guess this makes sense.</p>
<p dir="ltr">> > > +            continue;<br>
> > > +         }<br>
> > > +<br>
> > > +         assert(uniforms < uniform_array_size);<br>
> > > +         this->uniform_size[uniforms] = type_size(var->type);<br>
> > > +<br>
> > > +         if (strncmp(var->name, "gl_", 3) == 0)<br>
> > > +            nir_setup_builtin_uniform(var);<br>
> > > +         else<br>
> > > +            nir_setup_uniform(var);<br>
> > > +      }<br>
> > > +   } else {<br>
> > > +      /* ARB_vertex_program is not supported yet */<br>
> > > +      assert("Not implemented");<br>
> > > +   }<br>
> > >  }<br>
> > ><br>
> > >  void<br>
> > >  vec4_visitor::nir_setup_uniform(nir_variable *var)<br>
> > >  {<br>
> > > -   /* @TODO: Not yet implemented */<br>
> > > +   int namelen = strlen(var->name);<br>
> > > +<br>
> > > +   /* The data for our (non-builtin) uniforms is stored in a series of<br>
> > > +    * gl_uniform_driver_storage structs for each subcomponent that<br>
> > > +    * glGetUniformLocation() could name.  We know it's been set up in the same<br>
> > > +    * order we'd walk the type, so walk the list of storage and find anything<br>
> > > +    * with our name, or the prefix of a component that starts with our name.<br>
> > > +    */<br>
> > > +    unsigned offset = 0;<br>
> > > +    for (unsigned u = 0; u < shader_prog->NumUniformStorage; u++) {<br>
> > > +       struct gl_uniform_storage *storage = &shader_prog->UniformStorage[u];<br>
> > > +<br>
> > > +       if (storage->builtin)<br>
> > > +          continue;<br>
> > > +<br>
> > > +       if (strncmp(var->name, storage->name, namelen) != 0 ||<br>
> > > +           (storage->name[namelen] != 0 &&<br>
> > > +            storage->name[namelen] != '.' &&<br>
> > > +            storage->name[namelen] != '[')) {<br>
> > > +          continue;<br>
> > > +       }<br>
> > > +<br>
> > > +       gl_constant_value *components = storage->storage;<br>
> > > +       unsigned vector_count = (MAX2(storage->array_elements, 1) *<br>
> > > +                                storage->type->matrix_columns);<br>
> ><br>
> > In the FS backend, we simply use storage->type->component_slots().<br>
> > Why can't we do that here?  It seems to be performing the same<br>
> > calculation.<br>
><br>
> This does not seem to do what we want in the vec4 backend. For example,<br>
> consider the following piglit test:<br>
><br>
> tests/spec/gl-3.1/attributeless-vertexid.shader_test<br>
><br>
> In that test we have something like this:<br>
><br>
>    vec2[](<br>
>         vec2(-1, 1),<br>
>         vec2(-1,-1),<br>
>         vec2( 1,-1),<br>
>         vec2( 1, 1)<br>
>         );<br>
><br>
> And for that I see:<br>
><br>
> storage->type->component_slots() = 2<br>
><br>
> This is because storage->type is glsl_type::_vec2_type. This is not<br>
> convenient for us in the vec4 backend because it is giving a scalar<br>
> count and we want to compute the count in units of vec4.</p>
<p dir="ltr">Sorry I missed the fact that you were only multiplying by matrix_columns and not rows.  This is probably fine.<br>
--Jason</p>
<p dir="ltr">> > > +<br>
> > > +       for (unsigned s = 0; s < vector_count; s++) {<br>
> > > +          assert(uniforms < uniform_array_size);<br>
> > > +          uniform_vector_size[uniforms] = storage->type->vector_elements;<br>
> > > +<br>
> > > +          int i;<br>
> > > +          for (i = 0; i < uniform_vector_size[uniforms]; i++) {<br>
> > > +             stage_prog_data->param[uniforms * 4 + i] = components;<br>
> > > +             components++;<br>
> > > +          }<br>
> > > +          for (; i < 4; i++) {<br>
> > > +             static gl_constant_value zero = { 0.0 };<br>
> ><br>
> > This should probably be const.<br>
><br>
> Yes, I will fix this.<br>
><br>
> > > +             stage_prog_data->param[uniforms * 4 + i] = &zero;<br>
> > > +          }<br>
> > > +<br>
> > > +          int uniform_offset = var->data.driver_location + offset;<br>
> > > +          nir_uniform_offset[uniform_offset] = uniforms;<br>
> > > +          offset++;<br>
> > > +<br>
> > > +          nir_uniform_driver_location[uniforms] = var->data.driver_location;<br>
> > > +          uniforms++;<br>
> > > +       }<br>
> > > +    }<br>
> > >  }<br>
> > ><br>
> > >  void<br>
> > >  vec4_visitor::nir_setup_builtin_uniform(nir_variable *var)<br>
> > >  {<br>
> > > -   /* @TODO: Not yet implemented */<br>
> > > +   const nir_state_slot *const slots = var->state_slots;<br>
> > > +   assert(var->state_slots != NULL);<br>
> > > +<br>
> > > +   unsigned offset = 0;<br>
> > > +   for (unsigned int i = 0; i < var->num_state_slots; i++) {<br>
> > > +      /* This state reference has already been setup by ir_to_mesa,<br>
> > > +       * but we'll get the same index back here.  We can reference<br>
> > > +       * ParameterValues directly, since unlike brw_fs.cpp, we never<br>
> > > +       * add new state references during compile.<br>
> > > +       */<br>
> > > +      int index = _mesa_add_state_reference(this->prog->Parameters,<br>
> > > +                                           (gl_state_index *)slots[i].tokens);<br>
> > > +      gl_constant_value *values =<br>
> > > +         &this->prog->Parameters->ParameterValues[index][0];<br>
> > > +<br>
> > > +      assert(this->uniforms < uniform_array_size);<br>
> > > +<br>
> > > +      for (unsigned j = 0; j < 4; j++)<br>
> > > +         stage_prog_data->param[this->uniforms * 4 + j] =<br>
> > > +            &values[GET_SWZ(slots[i].swizzle, j)];<br>
> > > +<br>
> > > +      this->uniform_vector_size[this->uniforms] =<br>
> > > +         (var->type->is_scalar() || var->type->is_vector() ||<br>
> > > +          var->type->is_matrix() ? var->type->vector_elements : 4);<br>
> > > +<br>
> > > +      int uniform_offset = var->data.driver_location + offset;<br>
> > > +      nir_uniform_offset[uniform_offset] = uniforms;<br>
> > > +      offset++;<br>
> > > +<br>
> > > +      nir_uniform_driver_location[uniforms] = var->data.driver_location;<br>
> > > +      this->uniforms++;<br>
> > > +   }<br>
> > >  }<br>
> > ><br>
> > >  void<br>
> > > --<br>
> > > 2.1.4<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><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><br>
><br>
><br>
</p>