[Mesa-dev] [PATCH] i965: Use memcpy instead of an assignment when storing uniforms

[Eric Anholt]

Roland Scheidegger <sroland at vmware.com> writes:

> Am 08.08.2014 22:56, schrieb Neil Roberts:
>> The i965 driver uses a float pointer to point to the value of a uniform and
>> also as the destination within the batch buffer. However the same locations
>> can also be used to store values for integer uniforms. Previously the value
>> was being copied into the batch buffer with a regular assignment. This breaks
>> if the compiler does this by loading and saving through an x87 register
>> because the fst instruction tries to fix up invalid float values. That can
>> corrupt some specific integer values. This patch changes it to use a memcpy
>> instead so that it won't use a floating-point register.
>> 
>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugs.freedesktop.org/show_bug.cgi?id%3D81150&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=5ZyizVpb8bGGN4qANo378HB6GFoFoBABoDXdPAyIszE%3D%0A&s=107a04b60233e66abfb9e83a62b90d128d60d6c61eb41b1a08ec7ab537283903
>> ---
>>  src/mesa/drivers/dri/i965/gen6_vs_state.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/gen6_vs_state.c b/src/mesa/drivers/dri/i965/gen6_vs_state.c
>> index 905e123..cdbc083 100644
>> --- a/src/mesa/drivers/dri/i965/gen6_vs_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen6_vs_state.c
>> @@ -81,7 +81,13 @@ gen6_upload_push_constants(struct brw_context *brw,
>>         * wouldn't be set for them.
>>        */
>>        for (i = 0; i < prog_data->nr_params; i++) {
>> -         param[i] = *prog_data->param[i];
>> +         /* A memcpy is used here instead of a regular assignment because
>> +          * otherwise the value may end up being copied through a
>> +          * floating-point register. This will break if the x87 registers are
>> +          * used and we are storing an integer value here because the fst
>> +          * instruction tries to fix up invalid values and that would corrupt
>> +          * an integer value */
>> +         memcpy(param + i, prog_data->param[i], sizeof param[i]);
>>        }

Or just change the pointer type to uint32_t, right?

> Wow, crazy.
> Maybe it would make sense to just use a void pointer and do a single
> memcpy instead of looping through all params?
> I wonder why this isn't hit elsewhere, I'm pretty sure there's other
> places (not necessarily in i965 driver) which assign potential int data
> through floats. Seems to me the compiler is pretty crazy to use fst
> though to begin with...

Well, the uniforms aren't in a single linear allocation.  You need to
gather them from their separate storage locations.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140808/e518e86f/attachment.sig>