[Mesa-dev] [PATCH 1/5] i965/query: Set Ready flag in gen6_queryobj_get_results().

[Ben Widawsky]

On Fri, Dec 12, 2014 at 11:15:38PM -0800, Kenneth Graunke wrote:
> q->Ready means that the results are in, and core Mesa is free to return
> them to the application.  gen6_queryobj_get_results() is a natural place
> to set that flag; doing so means callers don't have to.
> 
> The older non-hardware-context aware code couldn't do this, because we
> had to call brw_queryobj_get_results() to gather intermediate results
> when we ran out of space for snapshots in the query buffer.  We only
> gather complete results in the Gen6+ code, however.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Eero Tamminen <eero.t.tamminen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/gen6_queryobj.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen6_queryobj.c b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> index 130236e..3013513 100644
> --- a/src/mesa/drivers/dri/i965/gen6_queryobj.c
> +++ b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> @@ -195,6 +195,8 @@ gen6_queryobj_get_results(struct gl_context *ctx,
>      */
>     drm_intel_bo_unreference(query->bo);
>     query->bo = NULL;
> +
> +   query->Base.Ready = true;
>  }
>  
>  /**
> @@ -305,7 +307,6 @@ static void gen6_wait_query(struct gl_context *ctx, struct gl_query_object *q)
>     struct brw_query_object *query = (struct brw_query_object *)q;
>  
>     gen6_queryobj_get_results(ctx, query);
> -   query->Base.Ready = true;
>  }
>  
>  /**
> @@ -331,7 +332,6 @@ static void gen6_check_query(struct gl_context *ctx, struct gl_query_object *q)
>  
>     if (query->bo == NULL || !drm_intel_bo_busy(query->bo)) {
>        gen6_queryobj_get_results(ctx, query);
> -      query->Base.Ready = true;
>     }
>  }
The behavior changes here if query->bo == NULL (after the patch Ready is never
set). However, looking through the code it seems query->bo won't ever be NULL.
AFAICT therefore, this is fine, but maybe you can take a look and make sure as
well.

Also, I notice an extraneous flush in this path, but I think you address it in a
later patch.

Assuming that's verified (and perhaps commit message updated to explain), this
is
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>