[Mesa-dev] [PATCH] i965: Set nr_params to the number of uniform components in the VS/GS path.

[Kenneth Graunke]

On Thursday, January 22, 2015 06:32:21 PM Francisco Jerez wrote:
> Both do_vs_prog and do_gs_prog initialize brw_stage_prog_data::nr_params to
> the number of uniform *vectors* required by the shader rather than the number
> of uniform components, contradicting the comment.  This is inconsistent with
> what the state upload code and scalar path expect but it happens to work until
> Gen8 because vec4_visitor interprets it as a number of vectors on construction
> and later on overwrites its original value with the number of uniform
> components referenced by the shader.
> 
> Also there's no need to add the number of samplers, they're not actually
> passed in as uniforms.
> 
> Fixes a memory corruption issue on BDW with SIMD8 VS.
> ---
>  src/mesa/drivers/dri/i965/brw_gs.c             |  6 +-----
>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp |  3 ++-
>  src/mesa/drivers/dri/i965/brw_vs.c             | 10 +---------
>  3 files changed, 4 insertions(+), 15 deletions(-)

Yikes, sorry...I thought this patch landed a long time ago.

This looks good to me, but I'm having a bit of trouble figuring out
what's actually changing.  It looks like fs_visitor::assign_constant_locations
sets nr_params to an appropriate value, like the vec4 backend does.

One difference I noticed was that fs_visitor::init() allocates the
param_size array to have nr_params elements, which would be too small;
your patch would fix that.  I could see that wreaking random havoc.

With s/CEILING/DIV_ROUND_UP/, this gets:

Cc: "10.5" <mesa-stable at lists.freedesktop.org>
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> 
> diff --git a/src/mesa/drivers/dri/i965/brw_gs.c b/src/mesa/drivers/dri/i965/brw_gs.c
> index c7ebe5f..ce3cba4 100644
> --- a/src/mesa/drivers/dri/i965/brw_gs.c
> +++ b/src/mesa/drivers/dri/i965/brw_gs.c
> @@ -69,11 +69,7 @@ do_gs_prog(struct brw_context *brw,
>        rzalloc_array(NULL, const gl_constant_value *, param_count);
>     c.prog_data.base.base.pull_param =
>        rzalloc_array(NULL, const gl_constant_value *, param_count);
> -   /* Setting nr_params here NOT to the size of the param and pull_param
> -    * arrays, but to the number of uniform components vec4_visitor
> -    * needs. vec4_visitor::setup_uniforms() will set it back to a proper value.
> -    */
> -   c.prog_data.base.base.nr_params = ALIGN(param_count, 4) / 4 + gs->num_samplers;
> +   c.prog_data.base.base.nr_params = param_count;
>  
>     if (brw->gen >= 7) {
>        if (gp->program.OutputType == GL_POINTS) {
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index 8b8b27f..f06ee53 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -3624,7 +3624,8 @@ vec4_visitor::vec4_visitor(struct brw_context *brw,
>      */
>     this->uniform_array_size = 1;
>     if (prog_data) {
> -      this->uniform_array_size = MAX2(stage_prog_data->nr_params, 1);
> +      this->uniform_array_size = MAX2(CEILING(stage_prog_data->nr_params, 4),
> +                                      1);
>     }
>  
>     this->uniform_size = rzalloc_array(mem_ctx, int, this->uniform_array_size);
> diff --git a/src/mesa/drivers/dri/i965/brw_vs.c b/src/mesa/drivers/dri/i965/brw_vs.c
> index 2d56b74..f360d4e 100644
> --- a/src/mesa/drivers/dri/i965/brw_vs.c
> +++ b/src/mesa/drivers/dri/i965/brw_vs.c
> @@ -241,15 +241,7 @@ do_vs_prog(struct brw_context *brw,
>        rzalloc_array(NULL, const gl_constant_value *, param_count);
>     stage_prog_data->pull_param =
>        rzalloc_array(NULL, const gl_constant_value *, param_count);
> -
> -   /* Setting nr_params here NOT to the size of the param and pull_param
> -    * arrays, but to the number of uniform components vec4_visitor
> -    * needs. vec4_visitor::setup_uniforms() will set it back to a proper value.
> -    */
> -   stage_prog_data->nr_params = ALIGN(param_count, 4) / 4;
> -   if (vs) {
> -      stage_prog_data->nr_params += vs->num_samplers;
> -   }
> +   stage_prog_data->nr_params = param_count;
>  
>     GLbitfield64 outputs_written = vp->program.Base.OutputsWritten;
>     prog_data.inputs_read = vp->program.Base.InputsRead;
> 
-------------- 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/20150316/6fdb9b67/attachment.sig>