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

[Stéphane Marchesin]

On Mon, Aug 27, 2012 at 2:48 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 08/27/2012 10:49 AM, Eric Anholt wrote:
>> Saves 26.5MB of wasted memory allocation in the l4d2 demo.
>> ---
>>  src/mesa/drivers/dri/i965/brw_context.h     |   15 ++++++++---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp        |    2 --
>>  src/mesa/drivers/dri/i965/brw_state_cache.c |    7 +++++
>>  src/mesa/drivers/dri/i965/brw_wm.c          |   38 +++++++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_wm.h          |    1 +
>>  5 files changed, 58 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
>> index 15ef0a3..c1cd500 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> @@ -313,11 +313,11 @@ struct brw_wm_prog_data {
>>      */
>>     uint32_t barycentric_interp_modes;
>>
>> -   /* Pointer to tracked values (only valid once
>> +   /* Pointers to tracked values (only valid once
>>      * _mesa_load_state_parameters has been called at runtime).
>>      */
>> -   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;
>>  };
>>
>>  /**
>> @@ -619,6 +619,7 @@ struct brw_cache_item {
>>  };
>>
>>
>> +typedef void (*cache_aux_free_func)(const void *aux);
>>
>>  struct brw_cache {
>>     struct brw_context *brw;
>> @@ -629,6 +630,14 @@ struct brw_cache {
>>
>>     uint32_t next_offset;
>>     bool bo_used_by_gpu;
>> +
>> +   /**
>> +    * Functions used in determining whether the prog_data for a new cache item
>> +    * matches an existing cache item.  This is sometimes done by memcmp, but
>> +    * for some cache ids that would mean statically sizing arrays instead of
>> +    * using nice pointers.
>> +    */
>
> This comment makes no sense to me.  Your newly introduced function is a
> helper to free arrays.  I don't see any changes to how key comparisons
> are done (memcmp or something else).  I'm kind of scared that this
> comment implies that memcmp doesn't work but I don't see a change to not
> use memcmp.

Yes I think the code is wrong in that respect.

The shader cache key is basically made of strcat(shader, uniforms),
and we match it using a memcmp.

With this change I think there is a way to have two pairs (shader,
uniforms) which have the same memory representation, for example the
same 20 bytes spread among shader/uniforms as follows: shader1 is 8
bytes and uniforms1 are 12 bytes/ shader2 is 12 bytes and uniforms2
are 8 bytes.

This will have the same hash/size and also the same memcmp, so you'll
end up using shader1 instead of shader2.

The only reason it worked before is that the aux size was always constant.

Stéphane