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 <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.
>
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 <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
> >
>
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
