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

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> 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.
>
Yeah, exactly, that's the reason for the memory corruption, the FS
back-end assumes that nr_params is in components so it ends up
allocating less memory than we need.

> 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>
>
Thanks!

>> 
>> 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: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150316/28ba5c5f/attachment.sig>