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 <kenn...@whitecape.org>
>> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
>> Cc: Eero Tamminen <eero.t.tammi...@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 <b...@bwidawsk.net>
>
> I do hope to see some perf improvements.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
