[Mesa-dev] [PATCH 4/4] i965: Convert reloc.target_handle into an index for I915_EXEC_HANDLE_LUT

[Chris Wilson]

Quoting Daniel Vetter (2017-07-07 11:31:46)
> On Mon, Jun 19, 2017 at 11:06:50AM +0100, Chris Wilson wrote:
> > Passing the index of the target buffer via the reloc.target_handle is
> > marginally more efficient for the kernel (it can avoid some allocations,
> > and can use a direct lookup rather than a hash or search). It is also
> > useful for ourselves as we can use the index into our exec_bos for other
> > tasks.
> > 
> > v2: Only enable HANDLE_LUT if we can use BATCH_FIRST and thereby avoid
> > a post-processing loop to fixup the relocations.
> > 
> > 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>
> > Cc: Jason Ekstrand <jason.ekstrand at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_context.h       |  1 +
> >  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 82 ++++++++++++++++++++-------
> >  2 files changed, 61 insertions(+), 22 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> > index 2fb2cab918..93ddd0825a 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > @@ -450,6 +450,7 @@ struct intel_batchbuffer {
> >  
> >     uint32_t state_batch_offset;
> >     enum brw_gpu_ring ring;
> > +   bool has_batch_first;
> >     bool needs_sol_reset;
> >     bool state_base_address_emitted;
> >  
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > index 15aaf01e52..5fa849c5a5 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -40,6 +40,10 @@
> >  
> >  #define FILE_DEBUG_FLAG DEBUG_BUFMGR
> >  
> > +#define DBG_NO_BATCH_FIRST 0
> > +#define I915_PARAM_HAS_EXEC_BATCH_FIRST 48
> > +#define I915_EXEC_BATCH_FIRST (1 << 18)
> 
> Needs an #ifndef I think, to avoid troubles when updating libdrm. Or just
> properly update mesa's copy of i915_drm.h, that would be much better.

Because src/include/drm/i915_drm.h didn't exist at time. In the current
version of the patch, this pair are no longer required as they are
already defined.

> >  static void
> >  intel_batchbuffer_reset(struct intel_batchbuffer *batch,
> >                          struct brw_bufmgr *bufmgr,
> > @@ -57,13 +61,33 @@ uint_key_hash(const void *key)
> >     return (uintptr_t) key;
> >  }
> >  
> > +static int gem_param(int fd, int name)
> > +{
> > +   drm_i915_getparam_t gp;
> > +   int v = -1; /* No param uses (yet) the sign bit, reserve it for errors */
> > +
> > +   memset(&gp, 0, sizeof(gp));
> > +   gp.param = name;
> > +   gp.value = &v;
> > +   if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> > +      return -1;
> > +
> > +   return v;
> > +}
> 
> Afaict this exists as intel_get_param already, why can't we use that?

Because it didn't exist when I wrote the patch. (I'm platforming for
kern_info to be queried once alongside dev_info.)

> > @@ -793,8 +829,9 @@ brw_emit_reloc(struct intel_batchbuffer *batch, uint32_t batch_offset,
> >     assert(batch_offset <= BATCH_SZ - sizeof(uint32_t));
> >     assert(_mesa_bitcount(write_domain) <= 1);
> >  
> > +   unsigned int index;
> >     if (target != batch->bo) {
> > -      unsigned int index = add_exec_bo(batch, target);
> > +      index = add_exec_bo(batch, target);
> >        struct drm_i915_gem_exec_object2 *exec = &batch->exec_objects[index];
> >  
> >        if (write_domain) {
> > @@ -811,6 +848,7 @@ brw_emit_reloc(struct intel_batchbuffer *batch, uint32_t batch_offset,
> >        offset64 = exec->offset;
> >     } else {
> >        offset64 = target->offset64;
> > +      index = target->index;
> 
> index = 0; Yes the bathc isn't ever shared, but I think it's better to
> make this obviously safe.

It's safer using the tracking than adding the presumption, surely?
That way this patch (in the original ordering) was just as happy with
using the batch in the last_slot and doing the fixups.
-Chris