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

[Kenneth Graunke]

On Saturday, December 13, 2014 08:21:35 PM Ben Widawsky wrote:
> 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.

You're right, but I think it's fine.

The lifetime is:
1. BeginQuery():
   - Creates a new BO. (bo != NULL)
   - q->Ready = False
   - q->Active = True
   When Active, asking about the result raises an error; mesa/main/queryobj.c
   won't ever call the CheckQuery/WaitQuery driver hooks.

2. EndQuery() writes the final snapshot (bo != NULL still), and marks the query
   as inactive (q->Active = False).  Ready is still False.

3. Now you can CheckQuery() or WaitQuery().  If the result is ready, they map
   query->bo, obtain the results, and free the buffer (bo = NULL).  Then it
   is marked Ready.

So...bo == NULL means that some earlier CheckQuery/WaitQuery call already
fetched the results, freed the buffer, and set the Ready flag.  Setting Ready
again would be redundant, so it doesn't matter.

Technically, bo == NULL can also happen if you ask for the results but haven't
ever called BeginQuery().  This is handled by setting Ready = True when creating
new query objects.

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

Hmm.  Not seeing that, but yeah, it's probably killed by the next patch.

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

Thanks Ben!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141214/71655c40/attachment.sig>