[Mesa-dev] [PATCH 2/2] Differentiate between DeleteQueries and DeleteQueriesARB

[Iago Toral]

On Tue, 2016-04-05 at 14:27 +0300, kevin.rogovin at intel.com wrote:
> From: Kevin Rogovin <kevin.rogovin at intel.com>
> 
> The extension, GL_ARB_occlusion_queries mandates that an
> INVALID_OPERATION should be thrown if DeleteQueriesARB is
> called while a query is active. In contrast, the GL spec
> has no such requirement for DeleteQueries. This patch
> unaliases the two functions and has that the ARB variant
> performs that extra check.
> 
> Signed-off-by: Kevin Rogovin <kevin.rogovin at intel.com>
> ---
>  src/mapi/glapi/gen/gl_API.xml |  2 +-
>  src/mesa/main/context.c       |  1 +
>  src/mesa/main/queryobj.c      | 31 ++++++++++++++++++++++++++-----
>  src/mesa/main/queryobj.h      |  2 ++
>  4 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml
> index 5918e63..452584c 100644
> --- a/src/mapi/glapi/gen/gl_API.xml
> +++ b/src/mapi/glapi/gen/gl_API.xml
> @@ -7569,7 +7569,7 @@
>          <param name="ids" type="GLuint *"/>
>      </function>
>  
> -    <function name="DeleteQueriesARB" alias="DeleteQueries">
> +    <function name="DeleteQueriesARB">
>          <param name="n" type="GLsizei"/>
>          <param name="ids" type="const GLuint *"/>
>      </function>
> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> index dbba136..1b250c0 100644
> --- a/src/mesa/main/context.c
> +++ b/src/mesa/main/context.c
> @@ -1090,6 +1090,7 @@ create_beginend_table(const struct gl_context *ctx)
>     COPY_DISPATCH(IsTexture);
>     COPY_DISPATCH(IsTransformFeedback);
>     COPY_DISPATCH(DeleteQueries);
> +   COPY_DISPATCH(DeleteQueriesARB);
>     COPY_DISPATCH(AreTexturesResident);
>     COPY_DISPATCH(FenceSync);
>     COPY_DISPATCH(ClientWaitSync);
> diff --git a/src/mesa/main/queryobj.c b/src/mesa/main/queryobj.c
> index 43f1b0c..d8575c2 100644
> --- a/src/mesa/main/queryobj.c
> +++ b/src/mesa/main/queryobj.c
> @@ -321,19 +321,29 @@ _mesa_CreateQueries(GLenum target, GLsizei n, GLuint *ids)
>     create_queries(ctx, target, n, ids, true);
>  }
>  
> -
> -void GLAPIENTRY
> -_mesa_DeleteQueries(GLsizei n, const GLuint *ids)
> +static void
> +delete_queries(GLsizei n, const GLuint *ids, const char *func, bool is_arb)
>  {
>     GLint i;
>     GET_CURRENT_CONTEXT(ctx);
>     FLUSH_VERTICES(ctx, 0);
>  
>     if (MESA_VERBOSE & VERBOSE_API)
> -      _mesa_debug(ctx, "glDeleteQueries(%d)\n", n);
> +      _mesa_debug(ctx, "%s(%d)\n", func, n);
>  
>     if (n < 0) {
> -      _mesa_error(ctx, GL_INVALID_VALUE, "glDeleteQueriesARB(n < 0)");
> +      _mesa_error(ctx, GL_INVALID_VALUE, "%s(n < 0)", func);
> +      return;
> +   }
> +
> +   /* From GL_ARB_occlusion_query:
> +      "The error INVALID_OPERATION is generated if GenQueriesARB or
> +       DeleteQueriesARB is called when a query of any target is active."
> +      That extension only support the target SAMPLES_PASSED_ARB, so we

... only supports ...

Same typo in the other patch.

> +      only check for CurrentOcclusionObject
> +    */
> +   if(is_arb && ctx->Query.CurrentOcclusionObject != NULL) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s\n", func);
>        return;
>     }
>  
> @@ -358,6 +368,17 @@ _mesa_DeleteQueries(GLsizei n, const GLuint *ids)
>     }
>  }
>  
> +void GLAPIENTRY
> +_mesa_DeleteQueries(GLsizei n, const GLuint *ids)
> +{
> +   delete_queries(n, ids, "glDeleteQueries", false);
> +}
> +
> +void GLAPIENTRY
> +_mesa_DeleteQueriesARB(GLsizei n, const GLuint *ids)
> +{
> +   delete_queries(n, ids, "glDeleteQueriesARB", true);
> +}

Maybe you could only add the boolean parameter and decide the function
name inside delete_queries() based on that, it is a bit redundant
otherwise.

Also, you solve this slightly different for GenQueries, where we add the
ARB-specific check inside the ARB function, why not just use the same
approach for both to keep consistency? create_queries() already decides
the funcion name inside based on a bool parameter too.

Either way, the series is:
Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

>  GLboolean GLAPIENTRY
>  _mesa_IsQuery(GLuint id)
> diff --git a/src/mesa/main/queryobj.h b/src/mesa/main/queryobj.h
> index 245d104..524767e 100644
> --- a/src/mesa/main/queryobj.h
> +++ b/src/mesa/main/queryobj.h
> @@ -56,6 +56,8 @@ void GLAPIENTRY
>  _mesa_CreateQueries(GLenum target, GLsizei n, GLuint *ids);
>  void GLAPIENTRY
>  _mesa_DeleteQueries(GLsizei n, const GLuint *ids);
> +void GLAPIENTRY
> +_mesa_DeleteQueriesARB(GLsizei n, const GLuint *ids);
>  GLboolean GLAPIENTRY
>  _mesa_IsQuery(GLuint id);
>  void GLAPIENTRY