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

[Kenneth Graunke]

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

So, I think we should just delete the aux compare functions
entirely.  AFAICT this stuff goes way back - maybe even to the original
driver import - so I suspect we just kept doing it, even when
unnecessary...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150625/5288bc24/attachment-0001.sig>