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

[Samuel Pitoiset]

On 04/20/2016 09:37 PM, Nicolai Hähnle wrote:
> 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.

No, it's fine by me. I was just wondering. :-)

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

Okay, thanks!

Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>

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