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

[Kenneth Graunke]

On Monday, December 15, 2014 01:38:47 PM Ben Widawsky wrote:
> On Mon, Dec 15, 2014 at 01:28:52PM -0800, 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.
> > 
> 
> It seems like until we implement QueryCounter that's not true.  (I mentioned
> this below)

You're right, I neglected to explain QueryCounter().  Most queries take two
counter snapshots (at BeginQuery and EndQuery time), and we subtract the two
to produce a proper value.

GL_TIMESTAMP is a unique counter - instead of a delta, you only get a single
value.  You don't use glBeginQuery and glEndQuery to access it.  Instead, you
use glQueryCounter(q, GL_TIMESTAMP).

So, the workflow is:

glQueryCounter -> CheckQuery or WaitQuery (which may flush the batch).

In other words, QueryCounter replaces BeginQuery/EndQuery.  The rest is the
same.
-------------- 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/e3d3cede/attachment-0001.sig>