[Mesa-dev] [PATCH 3/3] i965: Make the param pointer arrays for the VS dynamically sized.

[Kenneth Graunke]

On 08/27/2012 11:40 AM, Ian Romanick wrote:
> On 08/27/2012 10:49 AM, Eric Anholt wrote:
>> Saves 96MB of wasted memory in the l4d2 demo.
>> ---
>>   src/mesa/drivers/dri/i965/brw_context.h     |    4 ++--
>>   src/mesa/drivers/dri/i965/brw_state_cache.c |    2 ++
>>   src/mesa/drivers/dri/i965/brw_vs.c          |   32
>> +++++++++++++++++++++++++++
>>   src/mesa/drivers/dri/i965/brw_vs.h          |    1 +
>>   4 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>> b/src/mesa/drivers/dri/i965/brw_context.h
>> index c1cd500..b76afc0 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> @@ -443,8 +443,8 @@ struct brw_vs_prog_data {
>>       */
>>      GLuint urb_entry_size;
>>
>> -   const float *param[MAX_UNIFORMS * 4]; /* should be: BRW_MAX_CURBE */
>> -   const float *pull_param[MAX_UNIFORMS * 4];
>> +   const float **param;
>> +   const float **pull_param;
>>
>>      bool uses_new_param_layout;
>>      bool uses_vertexid;
>> diff --git a/src/mesa/drivers/dri/i965/brw_state_cache.c
>> b/src/mesa/drivers/dri/i965/brw_state_cache.c
>> index 092baf3..f69a94a 100644
>> --- a/src/mesa/drivers/dri/i965/brw_state_cache.c
>> +++ b/src/mesa/drivers/dri/i965/brw_state_cache.c
>> @@ -48,6 +48,7 @@
>>   #include "intel_batchbuffer.h"
>>   #include "brw_state.h"
>>   #include "brw_wm.h"
>> +#include "brw_vs.h"
>>
>>   #define FILE_DEBUG_FLAG DEBUG_STATE
>>
>> @@ -335,6 +336,7 @@ brw_init_caches(struct brw_context *brw)
>>                     "program cache",
>>                     4096, 64);
>>
>> +   cache->aux_free[BRW_VS_PROG] = brw_vs_prog_data_free;
>>      cache->aux_free[BRW_WM_PROG] = brw_wm_prog_data_free;
>>   }
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vs.c
>> b/src/mesa/drivers/dri/i965/brw_vs.c
>> index 2ad4134..e789dd2 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vs.c
>> +++ b/src/mesa/drivers/dri/i965/brw_vs.c
>> @@ -201,6 +201,10 @@ do_vs_prog(struct brw_context *brw,
>>      void *mem_ctx;
>>      int aux_size;
>>      int i;
>> +   struct gl_shader *vs = NULL;
>> +
>> +   if (prog)
>> +      vs = prog->_LinkedShaders[MESA_SHADER_VERTEX];
>>
>>      memset(&c, 0, sizeof(c));
>>      memcpy(&c.key, key, sizeof(*key));
>> @@ -210,6 +214,25 @@ do_vs_prog(struct brw_context *brw,
>>      brw_init_compile(brw, &c.func, mem_ctx);
>>      c.vp = vp;
>>
>> +   /* Allocate the references to the uniforms that will end up in the
>> +    * prog_data associated with the compiled program, and which will
>> be freed
>> +    * by the state cache.
>> +    */
>> +   int param_count;
>> +   if (vs) {
>> +      /* We add padding around uniform values below vec4 size, with
>> the worst
>> +       * case being a float value that gets blown up to a vec4, so be
>> +       * conservative here.
>> +       */
>> +      param_count = vs->num_uniform_components * 4;
>> +
>> +      /* We also upload clip plane data as uniforms */
>> +      param_count += MAX_CLIP_PLANES * 4;
>> +   } else
>> +      param_count = vp->program.Base.Parameters->NumParameters * 4;
>> +   c.prog_data.param = rzalloc_array(NULL, const float *, param_count);
>> +   c.prog_data.pull_param = rzalloc_array(NULL, const float *,
>> param_count);
>> +
> 
> Maybe add a blank line here?  I was first going to complain that the
> indentation was broken before I noticed that the c.prog_data.*
> assignments weren't part of the else block.

Curly braces on one branch but not the other is one of my pet-peeves.  I
would prefer it if you just put braces on both.

Otherwise, this patch looks fine (assuming my paranoia on patch 2 about
memcmp isn't an issue).

Patches 1 and 3 are:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

>>      c.prog_data.outputs_written = vp->program.Base.OutputsWritten;
>>      c.prog_data.inputs_read = vp->program.Base.InputsRead;
>>
>> @@ -411,3 +434,12 @@ brw_vs_precompile(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>
>>      return success;
>>   }
>> +
>> +void
>> +brw_vs_prog_data_free(const void *in_prog_data)
>> +{
>> +   const struct brw_vs_prog_data *prog_data = in_prog_data;
>> +
>> +   ralloc_free((void *)prog_data->param);
>> +   ralloc_free((void *)prog_data->pull_param);
>> +}
>> diff --git a/src/mesa/drivers/dri/i965/brw_vs.h
>> b/src/mesa/drivers/dri/i965/brw_vs.h
>> index 6d3b6ce..9153996 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_vs.h
>> @@ -120,5 +120,6 @@ struct brw_vs_compile {
>>   bool brw_vs_emit(struct gl_shader_program *prog, struct
>> brw_vs_compile *c);
>>   void brw_old_vs_emit(struct brw_vs_compile *c);
>>   bool brw_vs_precompile(struct gl_context *ctx, struct
>> gl_shader_program *prog);
>> +void brw_vs_prog_data_free(const void *in_prog_data);
>>
>>   #endif
>>
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev