[Mesa-dev] [PATCH] Remove error when calling glGenQueries/glDeleteQueries while a query is active

[Brian Paul]

On 10/17/2013 02:35 PM, Carl Worth wrote:
> There is nothing in the OpenGL specification which prevents the user from
> calling glGenQueries to generate a new query object while another object is
> active. Neither is there anything in the Mesa implementation which prevents
> this. So remove the INVALID_OPERATION errors in this case.
>
> Similarly, it is explicitly allowed by the OpenGL specification to delete an
> active query, so remove the assertion for that case and be sure to call the
> driver's EndQuery hook.
>
> CC: <mesa-stable at lists.freedesktop.org>
> ---
>
> Brian Paul <brianp at vmware.com> writes:
>> On 10/17/2013 12:14 PM, Carl Worth wrote:
>> But from http://www.opengl.org/registry/specs/ARB/occlusion_query.txt:
>>
>> """
>>       Calling either GenQueriesARB or DeleteQueriesARB while any query of
>>       any target is active causes an INVALID_OPERATION error to be
>>       generated.
>> """
>> (it's about half-way down in the file)  It's also mentioned in the
>> "Errors" section.
>
> Thanks, Brian. That certainly does justify where the original code came
> from.
>
>> Maybe that was rescinded since that spec was done.  If so, I'm fine with
>> removing the code.
>
> I can't find any similar error requirement in the OpenGL 4.4 (Core)
> specification. (And with the increased number of different query types,
> it doesn't seem that the error requirement makes sense.) I did find the
> following sentence in the specification (section 4.2):
>
> 	If an active query object is deleted its name immediately
> 	becomes unused, but the underlying object is not deleted until
> 	it is no longer active.
>
> This sentence presumes the possibility of deleting an active query, so I
> think it is reasonable to remove the error.

Yeah, sounds like it.

Would you be able to try your new piglit test on NVIDIA or AMD to see 
what they do, just to be sure?


>> However, I wouldn't be surprised if our drivers crashed and burned if an
>> active query is deleted.  gl_query_object isn't referenced counted.
>
> Thanks for the catch. My revised patch below calls the driver's EndQuery
> hook, which will hopefully avoid this problem. This does inactivate the
> query immediately at delete time, which might seem inconsistent with the
> sentence I quoted from the specification above. But I think this is fine
> since once the object is deleted the user has no visibility into whether
> the object is active or not.
>
> -Carl
>
>   src/mesa/main/queryobj.c | 19 ++++---------------
>   1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/src/mesa/main/queryobj.c b/src/mesa/main/queryobj.c
> index a180133..ebdb71c 100644
> --- a/src/mesa/main/queryobj.c
> +++ b/src/mesa/main/queryobj.c
> @@ -202,13 +202,6 @@ _mesa_GenQueries(GLsizei n, GLuint *ids)
>         return;
>      }
>
> -   /* No query objects can be active at this time! */
> -   if (ctx->Query.CurrentOcclusionObject ||
> -       ctx->Query.CurrentTimerObject) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "glGenQueriesARB");
> -      return;
> -   }
> -
>      first = _mesa_HashFindFreeKeyBlock(ctx->Query.QueryObjects, n);
>      if (first) {
>         GLsizei i;
> @@ -241,18 +234,14 @@ _mesa_DeleteQueries(GLsizei n, const GLuint *ids)
>         return;
>      }
>
> -   /* No query objects can be active at this time! */
> -   if (ctx->Query.CurrentOcclusionObject ||
> -       ctx->Query.CurrentTimerObject) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "glDeleteQueriesARB");
> -      return;
> -   }
> -
>      for (i = 0; i < n; i++) {
>         if (ids[i] > 0) {
>            struct gl_query_object *q = _mesa_lookup_query_object(ctx, ids[i]);
>            if (q) {
> -            ASSERT(!q->Active); /* should be caught earlier */
> +            if (q->Active) {
> +               q->Active = GL_FALSE;
> +               ctx->Driver.EndQuery(ctx, q);
> +            }
>               _mesa_HashRemove(ctx->Query.QueryObjects, ids[i]);
>               ctx->Driver.DeleteQuery(ctx, q);
>            }
>

I'd like to test this with a few gallium drivers, but I'm swamped today. 
  I may not get to this until next week...

-Brian