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

[Roland Scheidegger]

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]);
>        }
>  
>        if (0) {
> 

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

Roland