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

[Petri Latvala]

On 12/20/2013 09:26 PM, Kenneth Graunke wrote:
> 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.

Sounds good. Changing the name.


>>      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.

No, that value is the number of float-size components. param_count, 
calculated above that code, multiplies the number of components by 4 to 
have room elsewhere for scalar params that eat up a vec4. I'm just 
dividing the number back.

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

Ok, changing that. Here and in vs.

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

I would really like that too, but unfortunately the inconsistency was 
already present.

The best option would be to have another variable for uniform memory 
use, but I didn't want to make too invasive changes. nr_param gets used 
here for uniform float-size counts, and later on for something else. 
This results in some overuse of memory when uniforms are smaller than 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.

Argh, yes. Fixing that comment.

>> +     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)

Yes, changing.


-- 
Petri Latvala