[Mesa-dev] [PATCH] gallium/hud: prevent NULL pointer dereference with pipe_query functions

[Samuel Pitoiset]

On 06/25/2015 02:36 PM, Marek Olšák wrote:
> What's the point of drawing a HUD pane if a query cannot be created?

With my series which adds support for global performance counters on 
NV50, query_create() may fail if we want to monitor *two* different 
query types with the HUD.
This limitation is due to how the HUD uses the pipe_query interface and 
this doesn't fit well with the underlying interface exposed by Nouveau.

In other words, with two different query types the scenario is as follows:
CREATE Q1, BEGIN Q1, CREATE Q2, BEGIN Q2, END Q1, RESULT Q1, BEGIN Q1, 
END Q2, RESULT Q2, BEGIN Q2, END Q1, and so on.

But, with nv50/nvc0 drivers I need to schedule hardware counters at 
query creation and this is going to be pretty hard without a really 
weird workaround.
Hence, only one query type can be monitored simultaneously, and 
query_create() fails.

A better scenario for nouveau drivers will be:
CREATE Q1, CREATE Q2, BEGIN Q1, BEGIN Q2, END Q1, END Q2, RESULT Q1, 
RESULT Q2, BEGIN Q1, and so on.
This could allow to introduce, for example, begin_all_queries() and 
end_all_queries() to be able to create/begin/end all queries in one shot 
*only*.

My plan is to change this behaviour but it will require lot of changes 
in the HUD mainly because queries are collected by pane.

> Can we detect this during initialization?

I'm not sure if we can detect this at initialization and if this is 
going to be easy to do.
But, how can we handle the case where a driver will only fail one time 
to create a query? Do we need to remove the pane? Not sure.
This is going to be hard to say, especially because nouveau drivers 
could fail if no hardware counters are available.

>
> Marek
>
> On Wed, Jun 24, 2015 at 9:26 PM, Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
>> The HUD doesn't check if query_create() fails and it calls other
>> pipe_query functions with NULL pointer instead of a valid query object.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>   src/gallium/auxiliary/hud/hud_driver_query.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/hud/hud_driver_query.c b/src/gallium/auxiliary/hud/hud_driver_query.c
>> index 603aba7..ee71678 100644
>> --- a/src/gallium/auxiliary/hud/hud_driver_query.c
>> +++ b/src/gallium/auxiliary/hud/hud_driver_query.c
>> @@ -62,7 +62,8 @@ query_new_value(struct hud_graph *gr)
>>      uint64_t now = os_time_get();
>>
>>      if (info->last_time) {
>> -      pipe->end_query(pipe, info->query[info->head]);
>> +      if (info->query[info->head])
>> +         pipe->end_query(pipe, info->query[info->head]);
>>
>>         /* read query results */
>>         while (1) {
>> @@ -70,7 +71,7 @@ query_new_value(struct hud_graph *gr)
>>            union pipe_query_result result;
>>            uint64_t *res64 = (uint64_t *)&result;
>>
>> -         if (pipe->get_query_result(pipe, query, FALSE, &result)) {
>> +         if (query && pipe->get_query_result(pipe, query, FALSE, &result)) {
>>               info->results_cumulative += res64[info->result_index];
>>               info->num_results++;
>>
>> @@ -88,7 +89,8 @@ query_new_value(struct hud_graph *gr)
>>                          "gallium_hud: all queries are busy after %i frames, "
>>                          "can't add another query\n",
>>                          NUM_QUERIES);
>> -               pipe->destroy_query(pipe, info->query[info->head]);
>> +               if (info->query[info->head])
>> +                  pipe->destroy_query(pipe, info->query[info->head]);
>>                  info->query[info->head] =
>>                        pipe->create_query(pipe, info->query_type, 0);
>>               }
>> @@ -113,15 +115,15 @@ query_new_value(struct hud_graph *gr)
>>            info->results_cumulative = 0;
>>            info->num_results = 0;
>>         }
>> -
>> -      pipe->begin_query(pipe, info->query[info->head]);
>>      }
>>      else {
>>         /* initialize */
>>         info->last_time = now;
>>         info->query[info->head] = pipe->create_query(pipe, info->query_type, 0);
>> -      pipe->begin_query(pipe, info->query[info->head]);
>>      }
>> +
>> +   if (info->query[info->head])
>> +      pipe->begin_query(pipe, info->query[info->head]);
>>   }
>>
>>   static void
>> --
>> 2.4.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev