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

[Pierre Moreau]

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.

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

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