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

[Ben Widawsky]

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)

> > 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?
> > 
> >> @@ -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.
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> 

-- 
Ben Widawsky, Intel Open Source Technology Center