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