[Mesa-dev] [PATCH 21/22] i965: Setup glsl uniforms by index rather than name matching
Timothy Arceri
tarceri at itsqueeze.com
Thu May 3 23:33:20 UTC 2018
On 03/05/18 22:50, Neil Roberts wrote:
> Thanks for the feedback.
>
> I’ve implemented your suggestion as the top two patches on a branch
> here:
>
> https://github.com/Igalia/mesa/tree/nroberts/count-uniform-storage
>
> It also fixes a slight bug that it wasn’t passing down the
> uniform_storage_locs argument into the recursion.
>
> However I think my preference would be for keeping this as a separate
> function because it seems a little confusing to make the function count two
> different things depending on a bool argument. Perhaps for the sake of
> maintainability it would be good to move it to glsl_types though.
Well we do a similar thing for count_attribute_slots() so I don't think
its to big of a deal.
>
> If there is a strong preference to merge it into one function then of
> course I don’t mind.
As long as we document the difference between the two functions having
two would probably be ok also, although my preference is to just have
the one. Up to you others might disagree with me.
>
> Regards,
> - Neil
>
> Timothy Arceri <tarceri at itsqueeze.com> writes:
>
>> On 18/04/18 00:36, Alejandro Piñeiro wrote:
>>> From: Neil Roberts <nroberts at igalia.com>
>>>
>>> Previously when setting up a uniform it would try to walk the uniform
>>> storage slots and find one that matches the name of the given
>>> variable. However, each variable already has a location which is an
>>> index into the UniformStorage array so we can just directly jump to
>>> the right slot. Some of the variables take up more than one slot so we
>>> still need to calculate how many it uses.
>>>
>>> The main reason to do this is to support ARB_gl_spirv because in that
>>> case the uniforms don’t have names so the previous approach won’t
>>> work.
>>
>> This is a nice improvement away :)
>>
>> However it might be nicer to just update the existing
>> glsl_type::uniform_locations() helper
>> rather than create the custom count_uniform_storage_slots() helper.
>>
>> e.g
>>
>> unsigned
>> glsl_type::uniform_locations(bool uniform_storage_locs) const
>> {
>> unsigned size = 0;
>>
>> switch (this->base_type) {
>> case GLSL_TYPE_UINT:
>> case GLSL_TYPE_INT:
>> case GLSL_TYPE_FLOAT:
>> case GLSL_TYPE_FLOAT16:
>> case GLSL_TYPE_DOUBLE:
>> case GLSL_TYPE_UINT16:
>> case GLSL_TYPE_UINT8:
>> case GLSL_TYPE_INT16:
>> case GLSL_TYPE_INT8:
>> case GLSL_TYPE_UINT64:
>> case GLSL_TYPE_INT64:
>> case GLSL_TYPE_BOOL:
>> case GLSL_TYPE_SAMPLER:
>> case GLSL_TYPE_IMAGE:
>> case GLSL_TYPE_SUBROUTINE:
>> return 1;
>>
>> case GLSL_TYPE_STRUCT:
>> case GLSL_TYPE_INTERFACE:
>> for (unsigned i = 0; i < this->length; i++)
>> size += this->fields.structure[i].type->uniform_locations();
>> return size;
>> case GLSL_TYPE_ARRAY:
>> if (!uniform_storage_locs ||
>> this->fields.array->is_array() ||
>> this->fields.array->is_interface() ||
>> this->fields.array->is_record()) {
>> /* For uniform locations passed to the user via the API we
>> * count all array elements.
>> */
>> return this->length * this->fields.array->uniform_locations();
>> } else {
>> /* For uniform storage the innermost array only uses a
>> * single slot.
>> */
>> return 1;
>> }
>> default:
>> return 0;
>> }
>> }
>>
>> If you agree this patch is:
>>
>> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
>>
>>> ---
>>> src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 50 +++++++++++++++++++-------
>>> 1 file changed, 37 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
>>> index 62b2951432a..cb5e07f74ba 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
>>> @@ -118,35 +118,59 @@ brw_setup_image_uniform_values(gl_shader_stage stage,
>>> }
>>> }
>>>
>>> +static unsigned
>>> +count_uniform_storage_slots(const struct glsl_type *type)
>>> +{
>>> + /* gl_uniform_storage can cope with one level of array, so if the
>>> + * type is a composite type or an array where each element occupies
>>> + * more than one slot than we need to recursively process it.
>>> + */
>>> + if (glsl_type_is_struct(type)) {
>>> + unsigned location_count = 0;
>>> +
>>> + for (unsigned i = 0; i < glsl_get_length(type); i++) {
>>> + const struct glsl_type *field_type = glsl_get_struct_field(type, i);
>>> +
>>> + location_count += count_uniform_storage_slots(field_type);
>>> + }
>>> +
>>> + return location_count;
>>> + }
>>> +
>>> + if (glsl_type_is_array(type)) {
>>> + const struct glsl_type *element_type = glsl_get_array_element(type);
>>> +
>>> + if (glsl_type_is_array(element_type) ||
>>> + glsl_type_is_struct(element_type)) {
>>> + unsigned element_count = count_uniform_storage_slots(element_type);
>>> + return element_count * glsl_get_length(type);
>>> + }
>>> + }
>>> +
>>> + return 1;
>>> +}
>>> +
>>> static void
>>> brw_nir_setup_glsl_uniform(gl_shader_stage stage, nir_variable *var,
>>> const struct gl_program *prog,
>>> struct brw_stage_prog_data *stage_prog_data,
>>> bool is_scalar)
>>> {
>>> - int namelen = strlen(var->name);
>>> -
>>> /* The data for our (non-builtin) uniforms is stored in a series of
>>> * gl_uniform_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.
>>> + * order we'd walk the type, so walk the list of storage that matches the
>>> + * range of slots covered by this variable.
>>> */
>>> unsigned uniform_index = var->data.driver_location / 4;
>>> - for (unsigned u = 0; u < prog->sh.data->NumUniformStorage; u++) {
>>> + unsigned num_slots = count_uniform_storage_slots(var->type);
>>> + for (unsigned u = 0; u < num_slots; u++) {
>>> struct gl_uniform_storage *storage =
>>> - &prog->sh.data->UniformStorage[u];
>>> + &prog->sh.data->UniformStorage[var->data.location + u];
>>>
>>> if (storage->builtin || storage->type->is_sampler())
>>> continue;
>>>
>>> - if (strncmp(var->name, storage->name, namelen) != 0 ||
>>> - (storage->name[namelen] != 0 &&
>>> - storage->name[namelen] != '.' &&
>>> - storage->name[namelen] != '[')) {
>>> - continue;
>>> - }
>>> -
>>> if (storage->type->is_image()) {
>>> brw_setup_image_uniform_values(stage, stage_prog_data,
>>> uniform_index, storage);
>>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list