[Mesa-dev] [PATCH 2/4] st/mesa: check return value of begin/end_query

[Nicolai Hähnle]

On 20.04.2016 11:13, Samuel Pitoiset wrote:
> On 04/20/2016 05:43 PM, Nicolai Hähnle wrote:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> They can only indicate out of memory conditions, since the other error
>> conditions are caught earlier.
>> ---
>>   src/mesa/state_tracker/st_cb_queryobj.c | 55
>> ++++++++++++++++++++-------------
>>   1 file changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/mesa/state_tracker/st_cb_queryobj.c
>> b/src/mesa/state_tracker/st_cb_queryobj.c
>> index cdb9efc..784ffde 100644
>> --- a/src/mesa/state_tracker/st_cb_queryobj.c
>> +++ b/src/mesa/state_tracker/st_cb_queryobj.c
>> @@ -61,13 +61,9 @@ st_NewQueryObject(struct gl_context *ctx, GLuint id)
>>   }
>>
>>
>> -
>>   static void
>> -st_DeleteQuery(struct gl_context *ctx, struct gl_query_object *q)
>> +free_queries(struct pipe_context *pipe, struct st_query_object *stq)
>>   {
>> -   struct pipe_context *pipe = st_context(ctx)->pipe;
>> -   struct st_query_object *stq = st_query_object(q);
>> -
>>      if (stq->pq) {
>>         pipe->destroy_query(pipe, stq->pq);
>>         stq->pq = NULL;
>> @@ -77,6 +73,16 @@ st_DeleteQuery(struct gl_context *ctx, struct
>> gl_query_object *q)
>>         pipe->destroy_query(pipe, stq->pq_begin);
>>         stq->pq_begin = NULL;
>>      }
>> +}
>> +
>> +
>> +static void
>> +st_DeleteQuery(struct gl_context *ctx, struct gl_query_object *q)
>> +{
>> +   struct pipe_context *pipe = st_context(ctx)->pipe;
>> +   struct st_query_object *stq = st_query_object(q);
>> +
>> +   free_queries(pipe, stq);
>>
>>      free(stq);
>>   }
>> @@ -89,6 +95,7 @@ st_BeginQuery(struct gl_context *ctx, struct
>> gl_query_object *q)
>>      struct pipe_context *pipe = st->pipe;
>>      struct st_query_object *stq = st_query_object(q);
>>      unsigned type;
>> +   bool ret = false;
>>
>>      st_flush_bitmap_cache(st_context(ctx));
>>
>> @@ -133,14 +140,7 @@ st_BeginQuery(struct gl_context *ctx, struct
>> gl_query_object *q)
>>
>>      if (stq->type != type) {
>>         /* free old query of different type */
>> -      if (stq->pq) {
>> -         pipe->destroy_query(pipe, stq->pq);
>> -         stq->pq = NULL;
>> -      }
>> -      if (stq->pq_begin) {
>> -         pipe->destroy_query(pipe, stq->pq_begin);
>> -         stq->pq_begin = NULL;
>> -      }
>> +      free_queries(pipe, stq);
>>         stq->type = PIPE_QUERY_TYPES; /* an invalid value */
>>      }
>>
>> @@ -151,20 +151,25 @@ st_BeginQuery(struct gl_context *ctx, struct
>> gl_query_object *q)
>>            stq->pq_begin = pipe->create_query(pipe, type, 0);
>>            stq->type = type;
>>         }
>> -      pipe->end_query(pipe, stq->pq_begin);
>> +      if (stq->pq_begin)
>> +         ret = pipe->end_query(pipe, stq->pq_begin);
>>      } else {
>>         if (!stq->pq) {
>>            stq->pq = pipe->create_query(pipe, type, q->Stream);
>>            stq->type = type;
>>         }
>> -      if (stq->pq) {
>> -         pipe->begin_query(pipe, stq->pq);
>> -      }
>> -      else {
>> -         _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBeginQuery");
>> -         return;
>> -      }
>> +      if (stq->pq)
>> +         ret = pipe->begin_query(pipe, stq->pq);
>>      }
>> +
>> +   if (!ret) {
>> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBeginQuery");
>> +
>> +      free_queries(pipe, stq);
>
> Why do you need to free queries now? Was there a memleak before? And why
> not in glEndQuery too?

There wasn't a memleak. The idea is that deleting objects that might be 
in "weird" internal states earlier is less likely to cause problems 
later on, but if you feel strongly about it I can remove it.


>> +      q->Active = GL_FALSE;
>> +      return;
>> +   }
>> +
>>      assert(stq->type == type);
>>   }
>>
>> @@ -174,6 +179,7 @@ st_EndQuery(struct gl_context *ctx, struct
>> gl_query_object *q)
>>   {
>>      struct pipe_context *pipe = st_context(ctx)->pipe;
>>      struct st_query_object *stq = st_query_object(q);
>> +   bool ret = false;
>>
>>      st_flush_bitmap_cache(st_context(ctx));
>>
>> @@ -185,7 +191,12 @@ st_EndQuery(struct gl_context *ctx, struct
>> gl_query_object *q)
>>      }
>>
>>      if (stq->pq)
>> -      pipe->end_query(pipe, stq->pq);
>> +      ret = pipe->end_query(pipe, stq->pq);
>> +
>> +   if (!ret) {
>> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBeginQuery");
>
> This should be "glEndQuery", right?

Good point, I've changed it locally.

> I think it would be better to move
> the error message inside the previous branch, what do you think?

No, this is in purpose, to catch a failure of pipe->create_query above.

Nicolai

>
>> +      return;
>> +   }
>>   }
>>
>>
>>