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

[Ian Romanick]

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.

> 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
>