Re: [Mesa-dev] [PATCH 1/4] nv50: add a header file for nv50_query

Mon, 19 Oct 2015 01:59:41 -0700 

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.pitoi...@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@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






















					Reply via email to