[Mesa-dev] [PATCH 1/4] nv50: add a header file for nv50_query
Samuel Pitoiset
samuel.pitoiset at gmail.com
Mon Oct 19 01:54:09 PDT 2015
On 10/19/2015 10:43 AM, Pierre Moreau wrote:
> Hi Samuel,
>
> (some comments further down)
>
> On 11:30 PM - Oct 18 2015, Samuel Pitoiset wrote:
>> Like for nvc0, this will allow to split different types of queries and
>> to prepare the way for both global performance counters and MP counters.
>>
>> While we are at it, make use of nv50_query struct instead of pipe_query.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>> src/gallium/drivers/nouveau/Makefile.sources | 1 +
>> src/gallium/drivers/nouveau/nv50/nv50_context.h | 12 +------
>> src/gallium/drivers/nouveau/nv50/nv50_query.c | 29 ++--------------
>> src/gallium/drivers/nouveau/nv50/nv50_query.h | 40 ++++++++++++++++++++++
>> .../drivers/nouveau/nv50/nv50_shader_state.c | 4 +--
>> src/gallium/drivers/nouveau/nv50/nv50_vbo.c | 3 +-
>> 6 files changed, 49 insertions(+), 40 deletions(-)
>> create mode 100644 src/gallium/drivers/nouveau/nv50/nv50_query.h
>>
>> diff --git a/src/gallium/drivers/nouveau/Makefile.sources b/src/gallium/drivers/nouveau/Makefile.sources
>> index c18e9f5..06d9d97 100644
>> --- a/src/gallium/drivers/nouveau/Makefile.sources
>> +++ b/src/gallium/drivers/nouveau/Makefile.sources
>> @@ -73,6 +73,7 @@ NV50_C_SOURCES := \
>> nv50/nv50_program.h \
>> nv50/nv50_push.c \
>> nv50/nv50_query.c \
>> + nv50/nv50_query.h \
>> nv50/nv50_resource.c \
>> nv50/nv50_resource.h \
>> nv50/nv50_screen.c \
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_context.h b/src/gallium/drivers/nouveau/nv50/nv50_context.h
>> index 69c1212..fb74a97 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_context.h
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_context.h
>> @@ -16,6 +16,7 @@
>> #include "nv50/nv50_program.h"
>> #include "nv50/nv50_resource.h"
>> #include "nv50/nv50_transfer.h"
>> +#include "nv50/nv50_query.h"
>>
>> #include "nouveau_context.h"
>> #include "nouveau_debug.h"
>> @@ -195,17 +196,6 @@ void nv50_default_kick_notify(struct nouveau_pushbuf *);
>> /* nv50_draw.c */
>> extern struct draw_stage *nv50_draw_render_stage(struct nv50_context *);
>>
>> -/* nv50_query.c */
>> -void nv50_init_query_functions(struct nv50_context *);
>> -void nv50_query_pushbuf_submit(struct nouveau_pushbuf *, uint16_t method,
>> - struct pipe_query *, unsigned result_offset);
>> -void nv84_query_fifo_wait(struct nouveau_pushbuf *, struct pipe_query *);
>> -void nva0_so_target_save_offset(struct pipe_context *,
>> - struct pipe_stream_output_target *,
>> - unsigned index, bool seralize);
>> -
>> -#define NVA0_QUERY_STREAM_OUTPUT_BUFFER_OFFSET (PIPE_QUERY_TYPES + 0)
>> -
>> /* nv50_shader_state.c */
>> void nv50_vertprog_validate(struct nv50_context *);
>> void nv50_gmtyprog_validate(struct nv50_context *);
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_query.c b/src/gallium/drivers/nouveau/nv50/nv50_query.c
>> index 5368ee7..7718d69 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_query.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_query.c
>> @@ -25,6 +25,7 @@
>> #define NV50_PUSH_EXPLICIT_SPACE_CHECKING
>>
>> #include "nv50/nv50_context.h"
>> +#include "nv50/nv50_query.h"
>> #include "nv_object.xml.h"
>>
>> #define NV50_QUERY_STATE_READY 0
>> @@ -39,29 +40,8 @@
>> * queries anyway.
>> */
>>
>> -struct nv50_query {
>> - uint32_t *data;
>> - uint16_t type;
>> - uint16_t index;
>> - uint32_t sequence;
>> - struct nouveau_bo *bo;
>> - uint32_t base;
>> - uint32_t offset; /* base + i * 32 */
>> - uint8_t state;
>> - bool is64bit;
>> - int nesting; /* only used for occlusion queries */
>> - struct nouveau_mm_allocation *mm;
>> - struct nouveau_fence *fence;
>> -};
>> -
>> #define NV50_QUERY_ALLOC_SPACE 256
>>
>> -static inline struct nv50_query *
>> -nv50_query(struct pipe_query *pipe)
>> -{
>> - return (struct nv50_query *)pipe;
>> -}
>> -
>> static bool
>> nv50_query_allocate(struct nv50_context *nv50, struct nv50_query *q, int size)
>> {
>> @@ -363,9 +343,8 @@ nv50_query_result(struct pipe_context *pipe, struct pipe_query *pq,
>> }
>>
>> void
>> -nv84_query_fifo_wait(struct nouveau_pushbuf *push, struct pipe_query *pq)
>> +nv84_query_fifo_wait(struct nouveau_pushbuf *push, struct nv50_query *q)
>> {
>> - struct nv50_query *q = nv50_query(pq);
>> unsigned offset = q->offset;
>>
>> PUSH_SPACE(push, 5);
>> @@ -453,10 +432,8 @@ nv50_render_condition(struct pipe_context *pipe,
>>
>> void
>> nv50_query_pushbuf_submit(struct nouveau_pushbuf *push, uint16_t method,
>> - struct pipe_query *pq, unsigned result_offset)
>> + struct nv50_query *q, unsigned result_offset)
>> {
>> - struct nv50_query *q = nv50_query(pq);
>> -
>> nv50_query_update(q);
>> if (q->state != NV50_QUERY_STATE_READY)
>> nouveau_bo_wait(q->bo, NOUVEAU_BO_RD, push->client);
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_query.h b/src/gallium/drivers/nouveau/nv50/nv50_query.h
>> new file mode 100644
>> index 0000000..722af0c
>> --- /dev/null
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_query.h
>> @@ -0,0 +1,40 @@
> Shouldn't there be a copyright notice (or whatever that text is called) here?
Apparently not for header files.
>
>> +#ifndef __NV50_QUERY_H__
>> +#define __NV50_QUERY_H__
>> +
>> +#include "pipe/p_context.h"
>> +
>> +#include "nouveau_context.h"
>> +#include "nouveau_mm.h"
>> +
>> +#define NVA0_QUERY_STREAM_OUTPUT_BUFFER_OFFSET (PIPE_QUERY_TYPES + 0)
>> +
>> +struct nv50_query {
>> + uint32_t *data;
>> + uint16_t type;
>> + uint16_t index;
>> + uint32_t sequence;
>> + struct nouveau_bo *bo;
>> + uint32_t base;
>> + uint32_t offset; /* base + i * 32 */
>> + uint8_t state;
>> + bool is64bit;
>> + int nesting; /* only used for occlusion queries */
>> + struct nouveau_mm_allocation *mm;
>> + struct nouveau_fence *fence;
>> +};
>> +
>> +static inline struct nv50_query *
>> +nv50_query(struct pipe_query *pipe)
>> +{
>> + return (struct nv50_query *)pipe;
>> +}
>> +
>> +void nv50_init_query_functions(struct nv50_context *);
>> +void nv50_query_pushbuf_submit(struct nouveau_pushbuf *, uint16_t,
>> + struct nv50_query *, unsigned result_offset);
>> +void nv84_query_fifo_wait(struct nouveau_pushbuf *, struct nv50_query *);
>> +void nva0_so_target_save_offset(struct pipe_context *,
>> + struct pipe_stream_output_target *,
>> + unsigned, bool);
>> +
>> +#endif
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_shader_state.c b/src/gallium/drivers/nouveau/nv50/nv50_shader_state.c
>> index 941555f..958d044 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_shader_state.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_shader_state.c
>> @@ -629,7 +629,7 @@ nv50_stream_output_validate(struct nv50_context *nv50)
>> const unsigned n = nv50->screen->base.class_3d >= NVA0_3D_CLASS ? 4 : 3;
>>
>> if (n == 4 && !targ->clean)
>> - nv84_query_fifo_wait(push, targ->pq);
>> + nv84_query_fifo_wait(push, nv50_query(targ->pq));
> This is not related to your patch, but there should be an `assert(targ->pq)`
> here, as `nv84_query_fifo_wait` dereferences it without checking (or add a check
> in `nv84_query_fifo_wait`. Or maybe `targ->pq` can't be NULL, in that case the
> `assert` a few lines below is useless and could be remove.
We actually never check if targ->pq is not NULL (even in
nv84_query_fifo_wait()),
and this patch just follows the same behaviour.
Anyway, if targ->pq is NULL the driver will miserably crashes so I don'
think it's really
useful to check if it's not NULL (especially in this patch). This could
be improved later
if someone want to do it. :)
By the way, it's *exactly* the same code for the nvc0 driver.
>
>> BEGIN_NV04(push, NV50_3D(STRMOUT_ADDRESS_HIGH(i)), n);
>> PUSH_DATAh(push, buf->address + targ->pipe.buffer_offset);
>> PUSH_DATA (push, buf->address + targ->pipe.buffer_offset);
>> @@ -639,7 +639,7 @@ nv50_stream_output_validate(struct nv50_context *nv50)
>> if (!targ->clean) {
>> assert(targ->pq);
>> nv50_query_pushbuf_submit(push, NVA0_3D_STRMOUT_OFFSET(i),
>> - targ->pq, 0x4);
>> + nv50_query(targ->pq), 0x4);
> You're casting `targ->pq` only twice, so not sure if it would make sense to
> define a `nv50_query` pointer at the start of the function (along with the
> assert, if neeeded), and cast `targ->pq` only once.
>
>> } else {
>> BEGIN_NV04(push, NVA0_3D(STRMOUT_OFFSET(i)), 1);
>> PUSH_DATA(push, 0);
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_vbo.c b/src/gallium/drivers/nouveau/nv50/nv50_vbo.c
>> index f5f4708..dbc6632 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_vbo.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_vbo.c
>> @@ -745,7 +745,8 @@ nva0_draw_stream_output(struct nv50_context *nv50,
>> PUSH_DATA (push, 0);
>> BEGIN_NV04(push, NVA0_3D(DRAW_TFB_STRIDE), 1);
>> PUSH_DATA (push, so->stride);
>> - nv50_query_pushbuf_submit(push, NVA0_3D_DRAW_TFB_BYTES, so->pq, 0x4);
>> + nv50_query_pushbuf_submit(push, NVA0_3D_DRAW_TFB_BYTES,
>> + nv50_query(so->pq), 0x4);
> Again, if an assert is needed for the comments above, you might need to add one
> here too.
>
>
> Pierre
Thanks Pierre.
>
>
>
>> BEGIN_NV04(push, NV50_3D(VERTEX_END_GL), 1);
>> PUSH_DATA (push, 0);
>>
>> --
>> 2.6.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list