[Mesa-dev] [RFC] i965: Don't consider uniform value locations in program uploads

[Eric Anholt]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On Wednesday, June 03, 2015 09:21:11 PM Topi Pohjolainen wrote:
>> Shader programs are cached per stage (FS, VS, GS) using the
>> corresponding shader source identifier and compile time choices
>> as key. However, one not only stores the program binary but
>> a pair consisting of program binary and program data. The latter
>> represents the store of constants (such as uniforms) used by
>> the program.
>> 
>> However, when programs are searched in the cache for reloading
>> only the program key representing the binary is considered
>> (see for example, brw_upload_wm_prog() and brw_search_cache()).
>> Hence, when programs are re-loaded from cache the first program
>> binary, program data pair is extracted without considering if
>> the program data matches the currently in use uniform storage
>> as well.
>> 
>> My reasoning Why this actually works is because the key
>> contains the identifier of the corresponding gl_program that
>> represents the source code for the shader program. Hence,
>> two programs having identical source code still have unique
>> keys.
>> And therefore brw_try_upload_using_copy() never encounters
>> a case where a matching binary is found but the program data
>> doesn't match.
>> 
>> My ultimate goal is to stop storing pointers to the individual
>> components of a uniform but to store only a pointer to the
>> "struct gl_uniform_storage" instead, and allow
>> gen6_upload_push_constants() to iterate over individual
>> components and array elements. This is needed to be able to
>> convert 32-bits floats to fp16 - otherwise there is only
>> pointer to 32-bits without knowing its type (int, float, etc)
>> let alone its target precision.
>> 
>> No regression in jenkins. However, we talked about this with
>> Ken and this doesn't really tell much as piglit doesn't really
>> re-use shader sources during one execution.
>> 
>> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>> CC: Kenneth Graunke <kenneth at whitecape.org>
>> CC: Tapani P\344lli <tapani.palli at intel.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_program.c | 6 ------
>>  1 file changed, 6 deletions(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_program.c b/src/mesa/drivers/dri/i965/brw_program.c
>> index e5c0d3c..7f5fde8 100644
>> --- a/src/mesa/drivers/dri/i965/brw_program.c
>> +++ b/src/mesa/drivers/dri/i965/brw_program.c
>> @@ -576,12 +576,6 @@ brw_stage_prog_data_compare(const struct brw_stage_prog_data *a,
>>     if (memcmp(a, b, offsetof(struct brw_stage_prog_data, param)))
>>        return false;
>>  
>> -   if (memcmp(a->param, b->param, a->nr_params * sizeof(void *)))
>> -      return false;
>> -
>> -   if (memcmp(a->pull_param, b->pull_param, a->nr_pull_params * sizeof(void *)))
>> -      return false;
>> -
>>     return true;
>>  }
>>  
>> 
>
> (Eric, feel free to add thoughts if you care to.  If not, no worries...)
>
> Okay, I've spent a while doing some git archaeology and trying to piece
> together this puzzle...this all goes back to 2011 and even 2010, so it's
> pushing my limits of recollection...
>
> Eric introduced brw_try_upload_using_copy() in 2011 (18d4a44bd).  His
> commit message indicates that it actually did something back then.
>
> I thought of one reason why it might have worked: in the bad old days,
> we used to call ProgramStringNotify() every time sampler uniforms
> changed.  Which increments key->program_string_id, meaning that every
> time sampler uniforms changed, the key would never match again.  But the
> shader assembly would be identical, and the uniform storage pointers
> should even have been the same.  Which should have hit Eric's code,
> preventing us from uploading an extra duplicate copy.
>
> In late 2012 (174d44a9c4d3), I fixed that, so we stopped doing that.
> I suspect that at this point, brw_try_upload_using_copy() basically
> stopped being useful.  I'm having a real hard time thinking of another
> case where the key wouldn't match, but both the shader assembly and
> prog_data - including the param pointers - would match.
>
> Looking at it now, I don't see any point at all in the aux_compare
> functions.  I've got no idea why brw_try_upload_using_copy() would
> bother checking prog_data.  All it does is avoid uploading an extra
> copy of the shader assembly into cache->bo.  Whether it succeeds or
> fails, we still create a new brw_cache_item entry which contains
> both the key and prog_data, which goes in brw->cache.items[].

The point was that, in addition to not duplicating the assembly, you
also only had one prog_data, and because of that you got to avoid FS
statechanges between identical programs.  It may be that once all the
bugs have been ironed out, we basically never ended up with sharing -- I
vaguely recall bugs in key comparison related to the prog_data.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150629/0a7e7b7c/attachment.sig>