[Mesa-dev] [SQUASH] i965: Zero out query buffer done bit

[Jordan Justen]

On 2016-04-27 12:28:15, Kenneth Graunke wrote:
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_context.h   |  2 ++
>  src/mesa/drivers/dri/i965/brw_queryobj.c  |  4 ++--
>  src/mesa/drivers/dri/i965/gen6_queryobj.c | 30 ++++++++++++++++++++++++++----
>  3 files changed, 30 insertions(+), 6 deletions(-)
> 
> Jordan,
> 
> I'm pretty sure you need to zero out the "Done?" bit in query->bo.  The BOs
> we allocate aren't guaranteed to be zero...so we may end up copying garbage
> to the MI_PREDICATE registers, rather than 0/1 as we wanted.

I think in gen6_begin_query, when we call:

   /* For ARB_query_buffer_object: The result is not available */
   set_query_availability(brw, query, false);

it should insert a CS write immediate to set it to 0. (Only for
'pipelined' queries.) Will that work?

-Jordan

> 
> Here's some code that Eric wrote a long time ago to do that; I've updated
> it a couple times now.  Feel free to squash it into your patch.
> 
> With all of my feedback addressed, you get a Reviewed-by :)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index b75eabd..3805b56 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1429,6 +1429,8 @@ void brw_query_counter(struct gl_context *ctx, struct gl_query_object *q);
>  bool brw_is_query_pipelined(struct brw_query_object *query);
>  
>  /** gen6_queryobj.c */
> +void brw_allocate_query_bo(struct brw_context *brw,
> +                           struct brw_query_object *query);
>  void gen6_init_queryobj_functions(struct dd_function_table *functions);
>  void brw_write_timestamp(struct brw_context *brw, drm_intel_bo *bo, int idx);
>  void brw_write_depth_count(struct brw_context *brw, drm_intel_bo *bo, int idx);
> diff --git a/src/mesa/drivers/dri/i965/brw_queryobj.c b/src/mesa/drivers/dri/i965/brw_queryobj.c
> index 81ee5ea..b7c2726 100644
> --- a/src/mesa/drivers/dri/i965/brw_queryobj.c
> +++ b/src/mesa/drivers/dri/i965/brw_queryobj.c
> @@ -470,8 +470,8 @@ brw_query_counter(struct gl_context *ctx, struct gl_query_object *q)
>  
>     assert(q->Target == GL_TIMESTAMP);
>  
> -   drm_intel_bo_unreference(query->bo);
> -   query->bo = drm_intel_bo_alloc(brw->bufmgr, "timestamp query", 4096, 4096);
> +   brw_allocate_query_bo(brw, query);
> +
>     brw_write_timestamp(brw, query->bo, 0);
>  
>     query->flushed = false;
> diff --git a/src/mesa/drivers/dri/i965/gen6_queryobj.c b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> index f95c9fc..04f58f6 100644
> --- a/src/mesa/drivers/dri/i965/gen6_queryobj.c
> +++ b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> @@ -40,6 +40,8 @@
>  #include "intel_buffer_objects.h"
>  #include "intel_reg.h"
>  
> +#define QUERY_DONE_OFFSET (2 * sizeof(uint64_t))
> +
>  static inline void
>  set_query_availability(struct brw_context *brw, struct brw_query_object *query,
>                         bool available)
> @@ -51,7 +53,7 @@ set_query_availability(struct brw_context *brw, struct brw_query_object *query,
>         brw_is_query_pipelined(query)) {
>        brw_emit_pipe_control_write(brw,
>                                    PIPE_CONTROL_WRITE_IMMEDIATE,
> -                                  query->bo, 2 * sizeof(uint64_t),
> +                                  query->bo, QUERY_DONE_OFFSET,
>                                    available, 0);
>     }
>  }
> @@ -140,6 +142,28 @@ emit_pipeline_stat(struct brw_context *brw, drm_intel_bo *bo,
>     brw_store_register_mem64(brw, bo, reg, idx * sizeof(uint64_t));
>  }
>  
> +void
> +brw_allocate_query_bo(struct brw_context *brw, struct brw_query_object *query)
> +{
> +   /* Since we're starting a new query, we need to throw away previous
> +    * uncollected results if there are any.
> +    */
> +   drm_intel_bo_unreference(query->bo);
> +
> +   query->bo = drm_intel_bo_alloc(brw->bufmgr, "query results", 4096, 4096);
> +
> +   if (ctx->Extensions.ARB_query_buffer_object) {
> +      /* Clear the "done" field.
> +       *
> +       * We can safely use _unsynchronized here (and possibly avoid bothering
> +       * the kernel for this mapping at all on a cached buffer on an LLC
> +       * system), because drm_intel_bo_alloc() guarantees you an idle BO.
> +       */
> +      drm_intel_gem_bo_map_unsynchronized(query->bo);
> +      *(uint32_t *)(query->bo->virtual + QUERY_DONE_OFFSET) = 0;
> +      drm_intel_bo_unmap(query->bo);
> +   }
> +}
>  
>  /**
>   * Wait on the query object's BO and calculate the final result.
> @@ -256,9 +280,7 @@ gen6_begin_query(struct gl_context *ctx, struct gl_query_object *q)
>     struct brw_context *brw = brw_context(ctx);
>     struct brw_query_object *query = (struct brw_query_object *)q;
>  
> -   /* Since we're starting a new query, we need to throw away old results. */
> -   drm_intel_bo_unreference(query->bo);
> -   query->bo = drm_intel_bo_alloc(brw->bufmgr, "query results", 4096, 4096);
> +   brw_allocate_query_bo(brw, query);
>  
>     /* For ARB_query_buffer_object: The result is not available */
>     set_query_availability(brw, query, false);
> -- 
> 2.8.0
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev