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.pitoi...@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.mor...@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@lists.freedesktop.org > >>http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev