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

[Pohjolainen, Topi]

On Wed, Jun 03, 2015 at 09:21:11PM +0300, 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.

In fact, thinking some more I think this is possible when the
same, say fragment shader, is used with two different vertex
shaders. This results into there being matching binaries but
program data pointing to different storage. Looking at
brw_upload_cache() I still can't see how failing
brw_try_upload_using_copy() makes a difference. We only upload
the program binary again (even though that is the part that
actually matches). And then proceed the same way regardless
of the result of brw_try_upload_using_copy(). The program data
gets augmented with the key.

But the point remains that when a program is reloaded through
the brw_search_cache() only the key (and not the program data)
is considered returning the first matching pair.

I probably need to write a piglit test for this.

> 
> 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;
>  }
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev