[Mesa-dev] [PATCH 6/9] i965: Map the query results for the life of the bo

[Chris Wilson]

Quoting Kenneth Graunke (2017-06-14 23:50:12)
> On Friday, June 9, 2017 6:01:37 AM PDT Chris Wilson wrote:
> > If we map the bo upon creation, we can avoid the latency of mmapping it
> > when querying, and later use the asynchronous, persistent map of the
> > predicate to do a quick query.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > Cc: Matt Turner <mattst88 at gmail.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_bufmgr.c    | 15 +++++++++++++
> >  src/mesa/drivers/dri/i965/brw_bufmgr.h    |  2 ++
> >  src/mesa/drivers/dri/i965/brw_context.h   |  1 +
> >  src/mesa/drivers/dri/i965/gen6_queryobj.c | 37 ++++++++++++++++++++++---------
> >  4 files changed, 44 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > index 01590a0b0a..9028b538c6 100644
> > --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > @@ -775,6 +775,21 @@ brw_bo_map(struct brw_context *brw, struct brw_bo *bo, unsigned flags)
> >        return brw_bo_map_gtt(brw, bo, flags);
> >  }
> >  
> > +void
> > +brw_bo_map_sync(struct brw_context *brw, struct brw_bo *bo, unsigned flags)
> > +{
> > +   unsigned domain;
> > +
> > +   if (bo->tiling_mode != I915_TILING_NONE && !(flags & MAP_RAW))
> > +      domain = I915_GEM_DOMAIN_GTT;
> > +   else if (can_map_cpu(bo, flags))
> > +      domain = I915_GEM_DOMAIN_CPU;
> > +   else
> > +      domain = I915_GEM_DOMAIN_GTT;
> > +
> > +   set_domain(brw, __func__, bo, domain, flags & MAP_WRITE ? domain : 0);
> > +}
> > +
> >  int
> >  brw_bo_unmap(struct brw_bo *bo)
> >  {
> > diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.h b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> > index 3a397be695..214b75bf1a 100644
> > --- a/src/mesa/drivers/dri/i965/brw_bufmgr.h
> > +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.h
> > @@ -196,6 +196,8 @@ void brw_bo_unreference(struct brw_bo *bo);
> >   */
> >  MUST_CHECK void *brw_bo_map(struct brw_context *brw, struct brw_bo *bo, unsigned flags);
> >  
> > +void brw_bo_map_sync(struct brw_context *brw, struct brw_bo *bo, unsigned flags);
> > +
> >  /**
> >   * Reduces the refcount on the userspace mapping of the buffer
> >   * object.
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> > index c5acb83ad0..117b1ecdca 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > @@ -419,6 +419,7 @@ struct brw_query_object {
> >  
> >     /** Last query BO associated with this query. */
> >     struct brw_bo *bo;
> > +   uint64_t *results;
> >  
> >     /** Last index in bo with query data for this object. */
> >     int last_index;
> > diff --git a/src/mesa/drivers/dri/i965/gen6_queryobj.c b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > index f913f986ae..18af608166 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_queryobj.c
> > @@ -221,7 +221,9 @@ gen6_queryobj_get_results(struct gl_context *ctx,
> >     if (query->bo == NULL)
> >        return;
> >  
> > -   uint64_t *results = brw_bo_map(brw, query->bo, MAP_READ);
> > +   brw_bo_map_sync(brw, query->bo, MAP_READ | MAP_COHERENT);
> > +   uint64_t *results = query->results;
> > +
> >     switch (query->Base.Target) {
> >     case GL_TIME_ELAPSED:
> >        /* The query BO contains the starting and ending timestamps.
> > @@ -296,7 +298,6 @@ gen6_queryobj_get_results(struct gl_context *ctx,
> >     default:
> >        unreachable("Unrecognized query target in brw_queryobj_get_results()");
> >     }
> > -   brw_bo_unmap(query->bo);
> >  
> >     /* Now that we've processed the data stored in the query's buffer object,
> >      * we can release it.
> > @@ -307,6 +308,23 @@ gen6_queryobj_get_results(struct gl_context *ctx,
> >     query->Base.Ready = true;
> >  }
> >  
> > +static int gen6_alloc_query(struct brw_context *brw,
> > +                            struct brw_query_object *query)
> > +{
> > +   /* Since we're starting a new query, we need to throw away old results. */
> > +   if (query->bo)
> > +      brw_bo_unreference(query->bo);
> > +
> > +   query->bo = brw_bo_alloc(brw->bufmgr, "query results", 4096, 4096);
> > +   query->results = brw_bo_map(brw, query->bo,
> > +                               MAP_READ | MAP_COHERENT | MAP_ASYNC);
> 
> I don't understand why you're using MAP_ASYNC here.  We're allocating a new
> BO here, and not using the BO_ALLOC_FOR_RENDER flag, so it will be idle.
> (brw_bufmgr.c:297 should ensure we never get a busy BO - if the cached BOs
> are busy, it will just allocate us a new one.)
> 
> So, MAP_ASYNC shouldn't avoid a stall.  It does, however, skip the
> SET_DOMAIN call, which means that it may not have the right domain
> for our new coherent mapping.  Hence, you need to whack it later with
> your new brw_bo_map_sync() helper.

That's bug in the brw_bo_map if we request COHERENT it should ensure
that the mapping is indeed coherent. I used ASYNC here for primarily
documentation -- it was copied from where the ASYNC was required on the
mapping and I kept it because it describes the intent that we will be
accessing this buffer without synchronisation.
> 
> I think you can drop MAP_ASYNC, and drop brw_bo_map_sync() entirely,
> with no ill-effects.  Or am I wrong?

The map_sync is for a different reason, as the goal is to read the
result before the batch is completed. Only when we need to force the
wait do we then have to stall using the brw_bo_map_sync(), and since we
don't strictly know the access method we defer to brw_bufmgr to figure
out how to wait on the buffer. For example, a good shortcut there will be

if (flags & COHERENT) {
	brw_bo_wait(bo, flags & MAP_WRITE);
	return;
}

to avoid the pesky set-domain.
-Chris