[Mesa-dev] [PATCH] mesa/arb_gpu_shader_fp64: add support for glGetUniformdv

[Timothy Arceri]

Couple of comments below, otherwise Reviewed-by: Timothy Arceri
<t_arceri at yahoo.com.au>

On Mon, 2015-07-27 at 13:14 +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> This was missed when I did fp64, I've sent a piglit test to cover
> the case as well.
> 
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  src/mesa/main/uniform_query.cpp | 15 +++++++++++----
>  src/mesa/main/uniforms.c        |  9 ---------
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/src/mesa/main/uniform_query.cpp 
> b/src/mesa/main/uniform_query.cpp
> index b5a94e9..5fb903a 100644
> --- a/src/mesa/main/uniform_query.cpp
> +++ b/src/mesa/main/uniform_query.cpp
> @@ -319,24 +319,31 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint 
> program, GLint location,
>  
>        return;
>     }
> +   if ((uni->type->base_type == GLSL_TYPE_DOUBLE &&
> +        returnType != GLSL_TYPE_DOUBLE) ||
> +       (uni->type->base_type != GLSL_TYPE_DOUBLE &&
> +        returnType == GLSL_TYPE_DOUBLE)) {
> +	 _mesa_error( ctx, GL_INVALID_OPERATION,
> +	             "glGetnUniform*vARB(incompatible uniform types)");

We could be nice to the dev and add uniform vs return type to the error
message.

> +	return;
> +   }
>  
>     {
>        unsigned elements = (uni->type->is_sampler())
>  	 ? 1 : uni->type->components();
> +      const int dmul = uni->type->base_type == GLSL_TYPE_DOUBLE ? 2 : 1;
>  
>        /* Calculate the source base address *BEFORE* modifying elements to
>         * account for the size of the user's buffer.
>         */
>        const union gl_constant_value *const src =
> -	 &uni->storage[offset * elements];
> +	 &uni->storage[offset * elements * dmul];
>  
> -      assert(returnType == GLSL_TYPE_FLOAT || returnType == GLSL_TYPE_INT 
> ||
> -             returnType == GLSL_TYPE_UINT);

I guess you could have just added GLSL_TYPE_DOUBLE to the assert but I dont
feel to strongly about it.

>        /* The three (currently) supported types all have the same size,
>         * which is of course the same as their union. That'll change
>         * with glGetUniformdv()...
>         */

The above comment needs to be updated or maybe just removed.

> -      unsigned bytes = sizeof(src[0]) * elements;
> +      unsigned bytes = sizeof(src[0]) * elements * dmul;
>        if (bufSize < 0 || bytes > (unsigned) bufSize) {
>  	 _mesa_error( ctx, GL_INVALID_OPERATION,
>  	             "glGetnUniform*vARB(out of bounds: bufSize is %d,"
> diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
> index 6ba746e..13bec88 100644
> --- a/src/mesa/main/uniforms.c
> +++ b/src/mesa/main/uniforms.c
> @@ -888,16 +888,7 @@ _mesa_GetnUniformdvARB(GLuint program, GLint location,
>  {
>     GET_CURRENT_CONTEXT(ctx);
>  
> -   (void) program;
> -   (void) location;
> -   (void) bufSize;
> -   (void) params;
> -
> -   /*
>     _mesa_get_uniform(ctx, program, location, bufSize, GLSL_TYPE_DOUBLE, 
> params);
> -   */
> -   _mesa_error(ctx, GL_INVALID_OPERATION, "glGetUniformdvARB"
> -               "(GL_ARB_gpu_shader_fp64 not implemented)");
>  }
>  
>  void GLAPIENTRY