[Mesa-dev] [PATCH 5/5] i965/query: Cache whether the batch references the query BO.

[Kenneth Graunke]

On Monday, December 15, 2014 01:28:52 PM Ian Romanick wrote:
> On 12/14/2014 04:39 PM, Ben Widawsky wrote:
> > On Fri, Dec 12, 2014 at 11:15:42PM -0800, Kenneth Graunke wrote:
> >> Chris Wilson noted that repeated calls to CheckQuery() would call
> >> drm_intel_bo_references(brw->batch.bo, query->bo) on each invocation,
> >> which is expensive.  Once we've flushed, we know that future batches
> >> won't reference query->bo, so there's no point in asking more than once.
> >>
> >> This patch adds a brw_query_object::flushed flag, which is a
> >> conservative estimate of whether the batch has been flushed.
> >>
> >> On the first call to CheckQuery() or WaitQuery(), we check if the
> >> batch references query->bo.  If not, it must have been flushed for
> >> some reason (such as being full).  We record that it was flushed.
> >> If it does reference query->bo, we explicitly flush, and record that
> >> we did so.
> >>
> >> Any subsequent checks will simply see that query->flushed is set,
> >> and skip the drm_intel_bo_references() call.
> >>
> >> Inspired by a patch from Chris Wilson.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86969
> >> 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/brw_context.h   |  3 +++
> >>  src/mesa/drivers/dri/i965/gen6_queryobj.c | 27 +++++++++++++++++++++++----
> >>  2 files changed, 26 insertions(+), 4 deletions(-)
> >>
> >> ickle's patch in bug #86969 eliminated all calls to drm_intel_bo_references
> >> in the query object code.  One consequence is that the first call to
> >> CheckQuery or WaitQuery would always flush the batch, even if it wasn't
> >> necessary (say, the batch had been flushed).
> >>
> >> Mine is a bit different - it still uses bo_references on the first call to
> >> CheckQuery/WaitQuery - but then records that the batch has been flushed, so
> >> we never have to call it again.
> >>
> >> Either approach should greatly reduce the overhead from repeatedly calling
> >> drm_intel_bo_references.
> >>
> >> Eero, would you be able to test this and see if it solves the issue you
> >> were seeing?  If so, could you provide some simple statistics showing the
> >> benefit?  Thanks!
> >>
> >> This is available as the 'query-references' branch in my tree (~kwg/mesa).
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> >> index 1b8f0bb..a63c483 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_context.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> >> @@ -831,6 +831,9 @@ struct brw_query_object {
> >>  
> >>     /** Last index in bo with query data for this object. */
> >>     int last_index;
> >> +
> >> +   /** True if we know the batch has been flushed since we ended the query. */
> >> +   bool flushed;
> >>  };
> >>  
> >>  struct intel_sync_object {
> >> diff --git a/src/mesa/drivers/dri/i965/gen6_queryobj.c b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> >> index 0d51390..de71bb5 100644
> >> --- a/src/mesa/drivers/dri/i965/gen6_queryobj.c
> >> +++ b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> >> @@ -281,6 +281,27 @@ gen6_end_query(struct gl_context *ctx, struct gl_query_object *q)
> >>     default:
> >>        unreachable("Unrecognized query target in brw_end_query()");
> >>     }
> >> +
> >> +   /* The current batch contains the commands to handle EndQuery(),
> >> +    * but they won't actually execute until it is flushed.
> >> +    */
> >> +   query->flushed = false;
> >> +}
> > 
> > I think it makes more sense to set the flushed state at BeginQuery, and to
> > remove this hunk. I think that's feasible. Am I correct that this is the
> > generalized regex for the callchain?
> > BeginQuery->(flush_batch_if_needed)*->EndQuery->(flush_batch_if_needed)*->EndQuery
> 
> 
> No.  It's BeginQuery->(flush_batch_if_needed)*->EndQuery.  The
> application cannot ask for the query results until after the EndQuery.
> I think the way Ken has it is correct.

No.  It's glBeginQuery -> glEndQuery -> WaitQuery() or CheckQuery(), which may
flush the batch if needed.

BeginQuery writes a counter snapshot into query->bo, and EndQuery writes a
second snapshot.  You can ask for the result any time after EndQuery.  To
obtain the results, we need both snapshots to actually be written to the
buffer.  Neither BeginQuery nor EndQuery flush, so one or both of the register
store commands might be sitting in the batch, unexecuted.

So, when the application asks for the results (WaitQuery() or CheckQuery()),
we may need to flush so those commands actually get executed.  However, if
we've flushed already, for some other reason, we don't need to flush again.

There's no point in flushing before EndQuery.

> > If this is correct, BeginBatch is the real marker of when we need to frob the
> > flushed state, since EndQuery is either a transient point, or the final call in
> > the Query object's life through the GL sequence.
> > 
> > Additionally, it looks like if we implement QueryCounter (consistent naming
> > FTW), EndQuery would only ever be called the last time the query object is
> > referenced, and that simplifies this even more.
> > 
> >> +
> >> +/**
> >> + * Flush the batch if it still references the query object BO.
> >> + */
> >> +static void
> >> +flush_batch_if_needed(struct brw_context *brw, struct brw_query_object *query)
> >> +{
> >> +   /* If the batch doesn't reference the BO, it must have been flushed
> >> +    * (for example, due to being full).  Record that it's been flushed.
> >> +    */
> >> +   query->flushed = query->flushed ||
> >> +      !drm_intel_bo_references(brw->batch.bo, query->bo);
> >> +
> >> +   if (!query->flushed)
> >> +      intel_batchbuffer_flush(brw);
> >>  }
> >>  
> >>  /**
> > 
> > Do you want to add an always_flush_batch check here?

Not particularly.  always_flush_batch means flushing the batch between each
primitive we draw.  We can do flushes for other reasons.

> >> @@ -298,8 +319,7 @@ static void gen6_wait_query(struct gl_context *ctx, struct gl_query_object *q)
> >>      * still contributing to it, flush it now to finish that work so the
> >>      * result will become available (eventually).
> >>      */
> >> -   if (drm_intel_bo_references(brw->batch.bo, query->bo))
> >> -      intel_batchbuffer_flush(brw);
> >> +   flush_batch_if_needed(brw, query);
> >>  
> >>     gen6_queryobj_get_results(ctx, query);
> >>  }
> >> @@ -328,8 +348,7 @@ static void gen6_check_query(struct gl_context *ctx, struct gl_query_object *q)
> >>      *      not ready yet on the first time it is queried.  This ensures that
> >>      *      the async query will return true in finite time.
> >>      */
> >> -   if (drm_intel_bo_references(brw->batch.bo, query->bo))
> >> -      intel_batchbuffer_flush(brw);
> >> +   flush_batch_if_needed(brw, query);
> >>  
> >>     if (!drm_intel_bo_busy(query->bo)) {
> >>        gen6_queryobj_get_results(ctx, query);
> > 
> > So while I had a verbose nit (which may not even be correct), I don't see
> > anything wrong here, so it's:
> > Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> > 
> > I do hope to see some perf improvements.
-------------- 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/20141215/c7ac6a32/attachment.sig>