[Mesa-dev] [PATCH 1/2] i965: Allocate vec4_visitor's uniform_size and uniform_vector_size arrays dynamically.

[Kenneth Graunke]

On 11/27/2013 05:28 AM, Petri Latvala wrote:
> v2: Don't add function parameters, pass the required size in
> prog_data->nr_params.
> 
> Signed-off-by: Petri Latvala <petri.latvala at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_vec4.h           | 5 +++--
>  src/mesa/drivers/dri/i965/brw_vec4_gs.c        | 5 +++++
>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 7 +++++++
>  src/mesa/drivers/dri/i965/brw_vs.c             | 8 ++++++++
>  4 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
> index 5cec9f9..5f5f5cd 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -325,8 +325,9 @@ public:
>      */
>     dst_reg output_reg[BRW_VARYING_SLOT_COUNT];
>     const char *output_reg_annotation[BRW_VARYING_SLOT_COUNT];
> -   int uniform_size[MAX_UNIFORMS];
> -   int uniform_vector_size[MAX_UNIFORMS];
> +   int *uniform_size;
> +   int *uniform_vector_size;
> +   int uniform_param_count; /*< Size of uniform_[vector_]size arrays */

I'm not crazy about this variable name.  Between the params arrays,
uniform_* arrays, nr_params count, and uniforms count...we already have
a lot of distinct things that sound alike.

How about:

int uniform_array_size; /**< Size of uniform_[vector_]size arrays. */

That seems clearer to me.  Especially seeing that the value here is
really the size of the array, which is an overestimate/upper bound on
the number of uniforms, not the actual number of elements in the
uniforms or params arrays.

>     int uniforms;
>  
>     src_reg shader_start_time;
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs.c b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
> index 018b0b6..7cf9bac 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
> @@ -64,6 +64,11 @@ do_gs_prog(struct brw_context *brw,
>  
>     c.prog_data.base.param = rzalloc_array(NULL, const float *, param_count);
>     c.prog_data.base.pull_param = rzalloc_array(NULL, const float *, param_count);
> +   /* Setting nr_params here NOT to the size of the param and pull_param
> +    * arrays, but to the number of uniform components vec4_visitor
> +    * needs. vec4_visitor::setup_uniforms() will set it back to a proper value.
> +    */
> +   c.prog_data.base.nr_params = param_count / 4 + gs->num_samplers;

Hmm.  You're counting the number of vec4s, but...don't samplers take up
a single entry, since they're just integers?  This seems odd to me.

You might also consider doing ALIGN(param_count, 4) / 4 so that you
round up rather than truncating on the division.

I also would really like to keep nr_params in consistent units, i.e.
always uniform float-size components or always vec4s.

>     if (gp->program.OutputType == GL_POINTS) {
>        /* When the output type is points, the geometry shader may output data
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index a13eafb..b9226dc 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -3253,6 +3253,10 @@ vec4_visitor::vec4_visitor(struct brw_context *brw,
>       fail_msg(NULL),
>       first_non_payload_grf(0),
>       need_all_constants_in_pull_buffer(false),
> +     /* Initialize uniform_param_count to at least 1 because gen6 VS requires at
> +      * least one. See setup_uniforms() in brw_vec4.cpp.
> +      */

I think you mean "Gen4-5 requires at least one push constant", not "gen6
VS."  At least, that's what setup_uniforms() is doing.

> +     uniform_param_count(prog_data->nr_params ? prog_data->nr_params : 1),

I think this would be clearer as:

   MAX2(prog_data->nr_params, 1)

>       debug_flag(debug_flag),
>       no_spills(no_spills)
>  {
> @@ -3290,6 +3294,9 @@ vec4_visitor::vec4_visitor(struct brw_context *brw,
>     this->max_grf = brw->gen >= 7 ? GEN7_MRF_HACK_START : BRW_MAX_GRF;
>  
>     this->uniforms = 0;
> +
> +   this->uniform_size = rzalloc_array(mem_ctx, int, this->uniform_param_count);
> +   this->uniform_vector_size = rzalloc_array(mem_ctx, int, this->uniform_param_count);
>  }
>  
>  vec4_visitor::~vec4_visitor()
> diff --git a/src/mesa/drivers/dri/i965/brw_vs.c b/src/mesa/drivers/dri/i965/brw_vs.c
> index b5c8b63..8d0933d 100644
> --- a/src/mesa/drivers/dri/i965/brw_vs.c
> +++ b/src/mesa/drivers/dri/i965/brw_vs.c
> @@ -242,6 +242,14 @@ do_vs_prog(struct brw_context *brw,
>  
>     prog_data.base.param = rzalloc_array(NULL, const float *, param_count);
>     prog_data.base.pull_param = rzalloc_array(NULL, const float *, param_count);
> +   /* Setting nr_params here NOT to the size of the param and pull_param
> +    * arrays, but to the number of uniform components vec4_visitor
> +    * needs. vec4_visitor::setup_uniforms() will set it back to a proper value.
> +    */
> +   prog_data.base.nr_params = param_count / 4;
> +   if (vs) {
> +      prog_data.base.nr_params += vs->num_samplers;
> +   }
>  
>     GLbitfield64 outputs_written = vp->program.Base.OutputsWritten;
>     prog_data.inputs_read = vp->program.Base.InputsRead;
>