[Mesa-dev] [PATCH 09/11] i965/nir: Pull common ARB program uniform handling into a common function
Jason Ekstrand
jason at jlekstrand.net
Fri Oct 2 10:47:42 PDT 2015
On Fri, Oct 2, 2015 at 3:25 AM, Iago Toral <itoral at igalia.com> wrote:
> On Wed, 2015-09-30 at 18:41 -0700, Jason Ekstrand wrote:
>> The way we deal with ARB program uniforms is basically the same in both the
>> vec4 and the fs backend. This commit takes the best parts of both
>> implementations and pulls the common code into a shared helper function.
>> ---
>> src/mesa/drivers/dri/i965/Makefile.sources | 1 +
>> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 10 +----
>> src/mesa/drivers/dri/i965/brw_nir.h | 9 ++++
>> src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 61 ++++++++++++++++++++++++++
>> src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 29 ++----------
>> 5 files changed, 76 insertions(+), 34 deletions(-)
>> create mode 100644 src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
>>
>> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources b/src/mesa/drivers/dri/i965/Makefile.sources
>> index 540e3df..eb8196d 100644
>> --- a/src/mesa/drivers/dri/i965/Makefile.sources
>> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
>> @@ -86,6 +86,7 @@ i965_FILES = \
>> brw_nir.h \
>> brw_nir.c \
>> brw_nir_analyze_boolean_resolves.c \
>> + brw_nir_uniforms.cpp \
>> brw_object_purgeable.c \
>> brw_packed_float.c \
>> brw_performance_monitor.c \
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> index d33e452..3ba6a67 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> @@ -196,14 +196,8 @@ fs_visitor::nir_setup_uniforms(nir_shader *shader)
>> param_size[var->data.driver_location] = type_size_scalar(var->type);
>> }
>> } else {
>> - /* prog_to_nir only creates a single giant uniform variable so we can
>> - * just set param up directly. */
>> - for (unsigned p = 0; p < prog->Parameters->NumParameters; p++) {
>> - for (unsigned int i = 0; i < 4; i++) {
>> - stage_prog_data->param[4 * p + i] =
>> - &prog->Parameters->ParameterValues[p][i];
>> - }
>> - }
>> + brw_nir_setup_arb_uniforms(shader, prog, stage_prog_data);
>> +
>> if(prog->Parameters->NumParameters > 0)
>> param_size[0] = prog->Parameters->NumParameters * 4;
>> }
>> diff --git a/src/mesa/drivers/dri/i965/brw_nir.h b/src/mesa/drivers/dri/i965/brw_nir.h
>> index ad71293..b4a6dc0 100644
>> --- a/src/mesa/drivers/dri/i965/brw_nir.h
>> +++ b/src/mesa/drivers/dri/i965/brw_nir.h
>> @@ -85,6 +85,15 @@ enum brw_reg_type brw_type_for_nir_type(nir_alu_type type);
>>
>> enum glsl_base_type brw_glsl_base_type_for_nir_type(nir_alu_type type);
>>
>> +void brw_nir_setup_glsl_uniforms(nir_shader *shader,
>> + struct gl_shader_program *shader_prog,
>> + const struct gl_program *prog,
>> + struct brw_stage_prog_data *stage_prog_data,
>> + bool is_scalar);
>
> brw_nir_setup_glsl_uniforms should be declared in the next patch.
Done
>> +void brw_nir_setup_arb_uniforms(nir_shader *shader, struct gl_program *prog,
>> + struct brw_stage_prog_data *stage_prog_data);
>> +
>> #ifdef __cplusplus
>> }
>> #endif
>> diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
>> new file mode 100644
>> index 0000000..ede4e91
>> --- /dev/null
>> +++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
>> @@ -0,0 +1,61 @@
>> +/*
>> + * Copyright © 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +
>> +#include "brw_shader.h"
>> +#include "brw_nir.h"
>> +
>> +void
>> +brw_nir_setup_arb_uniforms(nir_shader *shader, struct gl_program *prog,
>> + struct brw_stage_prog_data *stage_prog_data)
>> +{
>> + struct gl_program_parameter_list *plist = prog->Parameters;
>> +
>> +#ifndef NDEBUG
>> + if (!shader->uniforms.is_empty()) {
>> + /* For ARB programs, only a single "parameters" variable is generated to
>> + * support uniform data.
>> + */
>> + assert(shader->uniforms.length() == 1);
>> + nir_variable *var = (nir_variable *) shader->uniforms.get_head();
>> + assert(strcmp(var->name, "parameters") == 0);
>> + assert(var->type->array_size() == (int)plist->NumParameters);
>> + }
>> +#endif
>> +
>> + for (unsigned p = 0; p < plist->NumParameters; p++) {
>> + /* Parameters should be either vec4 uniforms or single component
>> + * constants; matrices and other larger types should have been broken
>> + * down earlier.
>> + */
>> + assert(plist->Parameters[p].Size <= 4);
>> +
>> + unsigned i;
>> + for (i = 0; i < plist->Parameters[p].Size; i++) {
>> + stage_prog_data->param[4 * p + i] = &plist->ParameterValues[p][i];
>> + }
>> + for (; i < 4; i++) {
>> + static const gl_constant_value zero = { 0.0 };
>> + stage_prog_data->param[4 * p + i] = &zero;
>> + }
>> + }
>> +}
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> index ee94e58..99fd71f 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> @@ -153,33 +153,10 @@ vec4_visitor::nir_setup_uniforms(nir_shader *shader)
>> nir_setup_uniform(var);
>> }
>> } else {
>> - /* For ARB_vertex_program, only a single "parameters" variable is
>> - * generated to support uniform data.
>> - */
>> - nir_variable *var = (nir_variable *) shader->uniforms.get_head();
>> - assert(shader->uniforms.length() == 1 &&
>> - strcmp(var->name, "parameters") == 0);
>> -
>> - assert(var->data.driver_location == 0);
>> - uniform_size[0] = type_size_vec4(var->type);
>> -
>> - struct gl_program_parameter_list *plist = prog->Parameters;
>> - for (unsigned p = 0; p < plist->NumParameters; p++) {
>> - /* Parameters should be either vec4 uniforms or single component
>> - * constants; matrices and other larger types should have been broken
>> - * down earlier.
>> - */
>> - assert(plist->Parameters[p].Size <= 4);
>> + brw_nir_setup_arb_uniforms(shader, prog, stage_prog_data);
>>
>> - unsigned i;
>> - for (i = 0; i < plist->Parameters[p].Size; i++) {
>> - stage_prog_data->param[p * 4 + i] = &plist->ParameterValues[p][i];
>> - }
>> - for (; i < 4; i++) {
>> - static const gl_constant_value zero = { 0.0 };
>> - stage_prog_data->param[p * 4 + i] = &zero;
>> - }
>> - }
>> + if(prog->Parameters->NumParameters > 0)
>> + uniform_size[0] = prog->Parameters->NumParameters;
>
> Feel free to ignore this, but I wonder: why don't we just remove the
> condition? (here and in the FS backend). It does not seem like it helps
> anything, right?
Because if there are no parameters, the uniform_size array will be
created with a zero length and this would be an out-of-bounds write
> With the comment about brw_nir_setup_glsl_uniforms addressed, this is:
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
>
>> }
>> }
>>
>
>
More information about the mesa-dev
mailing list