[Mesa-dev] [PATCH 4/4] nv50: do not create an invalid HW query type

[Samuel Pitoiset]

On 10/19/2015 12:43 PM, Pierre Moreau wrote:
> On 11:06 AM - Oct 19 2015, Samuel Pitoiset wrote:
>>
>> On 10/19/2015 11:01 AM, Pierre Moreau wrote:
>>> Hi Samuel,
>>>
>>> (some comments below)
>>>
>>> On 11:36 PM - Oct 18 2015, Samuel Pitoiset wrote:
>>>> While we are at it, store the rotate offset for occlusion queries to
>>>> nv50_hw_query like on nvc0.
>>>>
>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>>> ---
>>>>   src/gallium/drivers/nouveau/nv50/nv50_query_hw.c | 45 +++++++++++++++++-------
>>>>   src/gallium/drivers/nouveau/nv50/nv50_query_hw.h |  3 +-
>>>>   2 files changed, 35 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_query_hw.c b/src/gallium/drivers/nouveau/nv50/nv50_query_hw.c
>>>> index fcdd183..6260410 100644
>>>> --- a/src/gallium/drivers/nouveau/nv50/nv50_query_hw.c
>>>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_query_hw.c
>>>> @@ -126,9 +126,9 @@ nv50_hw_begin_query(struct nv50_context *nv50, struct nv50_query *q)
>>>>       * query might set the initial render condition to false even *after* we re-
>>>>       * initialized it to true.
>>>>       */
>>>> -   if (q->type == PIPE_QUERY_OCCLUSION_COUNTER) {
>>>> -      hq->offset += 32;
>>>> -      hq->data += 32 / sizeof(*hq->data);
>>>> +   if (hq->rotate) {
>>>> +      hq->offset += hq->rotate;
>>>> +      hq->data += hq->rotate / sizeof(*hq->data);
>>>>         if (hq->offset - hq->base_offset == NV50_HW_QUERY_ALLOC_SPACE)
>>>>            nv50_hw_query_allocate(nv50, q, NV50_HW_QUERY_ALLOC_SPACE);
>>>> @@ -330,6 +330,7 @@ nv50_hw_create_query(struct nv50_context *nv50, unsigned type, unsigned index)
>>>>   {
>>>>      struct nv50_hw_query *hq;
>>>>      struct nv50_query *q;
>>>> +   unsigned space;
>>>>      hq = CALLOC_STRUCT(nv50_hw_query);
>>>>      if (!hq)
>>>> @@ -339,22 +340,42 @@ nv50_hw_create_query(struct nv50_context *nv50, unsigned type, unsigned index)
>>>>      q->funcs = &hw_query_funcs;
>>>>      q->type = type;
>>>> -   if (!nv50_hw_query_allocate(nv50, q, NV50_HW_QUERY_ALLOC_SPACE)) {
>>>> +   switch (q->type) {
>>>> +   case PIPE_QUERY_OCCLUSION_COUNTER:
>>>> +      hq->rotate = 32;
>>> You should have `hq->rotate` default to 0 in other cases, as IIRC, you have no
>>> guaranty about the value of an uninitialised variable.
>> CALLOC_STRUCT will be initialize all fields to 0.
> Oh, that's nice! Didn't know about it.
>
>>>> +      space = NV50_HW_QUERY_ALLOC_SPACE;
>>>> +      break;
>>>> +   case PIPE_QUERY_PRIMITIVES_GENERATED:
>>>> +   case PIPE_QUERY_PRIMITIVES_EMITTED:
>>>> +   case PIPE_QUERY_SO_STATISTICS:
>>>> +   case PIPE_QUERY_PIPELINE_STATISTICS:
>>>> +      hq->is64bit = true;
>>> Same comment as for `hq->rotate`: have `hq->is64bit` default to `false`.
>>>
>>>> +      space = NV50_HW_QUERY_ALLOC_SPACE;
>>>> +      break;
>>>> +   case PIPE_QUERY_TIME_ELAPSED:
>>>> +   case PIPE_QUERY_TIMESTAMP:
>>>> +   case PIPE_QUERY_TIMESTAMP_DISJOINT:
>>>> +   case PIPE_QUERY_GPU_FINISHED:
>>>> +   case NVA0_HW_QUERY_STREAM_OUTPUT_BUFFER_OFFSET:
>>>> +      space = NV50_HW_QUERY_ALLOC_SPACE;
>>>> +      break;
>>>> +   default:
>>>> +      debug_printf("invalid query type: %u\n", type);
>>>> +      FREE(q);
>>>> +      return NULL;
>>>> +   }
>>>> +
>>>> +   if (!nv50_hw_query_allocate(nv50, q, space)) {
>>> `space` is always `NV50_HW_QUERY_ALLOC_SPACE`. Is there an advantage to
>>> introducing this `space` variable? Do you plan to later add other possible
>>> values to it?
>> I have a patch locally which reduces the size of that buffer for some
>> queries,
>> but this is not really related to this series. I'll submit it later (with
>> other patches).
> One could argue then that you should introduce `space` in those later patches.

space was already here, I just kept it :)

>
> Anyway,
> Reviewed-by: Pierre Moreau <pierre.morrow at free.fr>

Thanks!

>>> Pierre
>>>
>>>
>>>>         FREE(hq);
>>>>         return NULL;
>>>>      }
>>>> -   if (q->type == PIPE_QUERY_OCCLUSION_COUNTER) {
>>>> +   if (hq->rotate) {
>>>>         /* we advance before query_begin ! */
>>>> -      hq->offset -= 32;
>>>> -      hq->data -= 32 / sizeof(*hq->data);
>>>> +      hq->offset -= hq->rotate;
>>>> +      hq->data -= hq->rotate / sizeof(*hq->data);
>>>>      }
>>>> -   hq->is64bit = (type == PIPE_QUERY_PRIMITIVES_GENERATED ||
>>>> -                 type == PIPE_QUERY_PRIMITIVES_EMITTED ||
>>>> -                 type == PIPE_QUERY_SO_STATISTICS ||
>>>> -                 type == PIPE_QUERY_PIPELINE_STATISTICS);
>>>> -
>>>>      return q;
>>>>   }
>>>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_query_hw.h b/src/gallium/drivers/nouveau/nv50/nv50_query_hw.h
>>>> index ea2bf24..3a53e8a 100644
>>>> --- a/src/gallium/drivers/nouveau/nv50/nv50_query_hw.h
>>>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_query_hw.h
>>>> @@ -24,9 +24,10 @@ struct nv50_hw_query {
>>>>      uint32_t sequence;
>>>>      struct nouveau_bo *bo;
>>>>      uint32_t base_offset;
>>>> -   uint32_t offset; /* base + i * 32 */
>>>> +   uint32_t offset; /* base + i * rotate */
>>>>      uint8_t state;
>>>>      bool is64bit;
>>>> +   uint8_t rotate;
>>>>      int nesting; /* only used for occlusion queries */
>>>>      struct nouveau_mm_allocation *mm;
>>>>      struct nouveau_fence *fence;
>>>> -- 
>>>> 2.6.1
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev