[Mesa-dev] [PATCH 14/17] i965: Grow the batch/state buffers if we need space and can't flush.

[Kenneth Graunke]

On Wednesday, September 6, 2017 3:08:44 AM PDT Chris Wilson wrote:
> Quoting Kenneth Graunke (2017-09-06 01:09:47)
> > Previously, we would just assert fail and die in this case.  The only
> > safeguard is the "estimated max prim size" checks when starting a draw
> > (or compute dispatch or BLORP operation)...which are woefully broken.
> > 
> > Growing is fairly straightforward:
> > 
> > 1. Allocate a new larger BO.
> > 2. memcpy the existing contents over to the new buffer
> > 3. Set the new BO to the same GTT offset as the old BO.  When emitting
> >    relocations, we write the presumed GTT offset of the target BO.  If
> >    we changed it, we'd have to update all the existing values (by
> >    walking the relocation list and looking at offsets), which is more
> >    expensive.  With the old BO freed, ideally the kernel could simply
> >    place the new BO at that offset anyway.
> > 4. Update the validation list to contain the new BO.
> > 5. Update the relocation list to have the GEM handle for the new BO
> >    (which we can skip if using I915_EXEC_HANDLE_LUT).
> > ---
> >  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 104 ++++++++++++++++++++++++--
> >  1 file changed, 99 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > index 909f56f9792..118f75c4d71 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -43,6 +43,9 @@
> >  #define BATCH_SZ (8192*sizeof(uint32_t))
> >  #define STATE_SZ (8192*sizeof(uint32_t))
> >  
> > +/* Don't exceed this - batchbuffers need to fit in the ring! */
> 
> I don't understand this comment. I probably just have the wrong pov, you
> say ring and I then think of the legacy/lrc ringbuffer in the kernel.

My understanding was that the legacy Gen4-7.5 ringbuffer mode allocated a
ringbuffer that was...128kB large?  So if you exceeded that size, the
batch would not fit in the ring at all, and execbuf would fail.

65536 may be the wrong size here.

> > +#define MAX_BATCH_SIZE 65536
> > +
> >  static void
> >  intel_batchbuffer_reset(struct intel_batchbuffer *batch,
> >                          struct brw_bufmgr *bufmgr);
> > @@ -228,6 +231,78 @@ intel_batchbuffer_free(struct intel_batchbuffer *batch)
> >        _mesa_hash_table_destroy(batch->state_batch_sizes, NULL);
> >  }
> >  
> > +static void
> > +replace_bo_in_reloc_list(struct brw_reloc_list *rlist,
> > +                         uint32_t old_handle, uint32_t new_handle)
> > +{
> > +   for (int i = 0; i < rlist->reloc_count; i++) {
> > +      if (rlist->relocs[i].target_handle == old_handle)
> > +         rlist->relocs[i].target_handle = new_handle;
> > +   }
> > +}
> > +
> > +static void
> > +grow_buffer(struct brw_context *brw,
> > +            struct brw_bo **bo_ptr,
> > +            uint32_t **map_ptr,
> > +            unsigned existing_bytes,
> > +            unsigned new_size)
> > +{
> > +   struct intel_batchbuffer *batch = &brw->batch;
> > +   struct brw_bufmgr *bufmgr = brw->bufmgr;
> > +
> > +   uint32_t *old_map = *map_ptr;
> > +   struct brw_bo *old_bo = *bo_ptr;
> > +
> > +   struct brw_bo *new_bo = brw_bo_alloc(bufmgr, old_bo->name, new_size, 4096);
> > +   uint32_t *new_map = brw_bo_map(brw, new_bo, MAP_READ | MAP_WRITE);
> > +
> > +   perf_debug("Growing %s - ran out of space\n", old_bo->name);
> > +
> > +   /* Copy existing data to the new larger buffer */
> > +   memcpy(new_map, old_map, existing_bytes);
> 
> Needs the sse41 treatment.

Good catch, thanks!

> > +
> > +   /* Try to put the new BO at the same GTT offset as the old BO (which
> > +    * we're throwing away, so it doesn't need to be there).
> > +    *
> > +    * This guarantees that our relocations continue to work: values we've
> > +    * already written into the buffer, values we're going to write into the
> > +    * buffer, and the validation/relocation lists all will match.
> > +    */
> > +   new_bo->gtt_offset = old_bo->gtt_offset;
> > +   new_bo->index = old_bo->index;
> > +
> > +   /* Batch/state buffers are per-context, and if we've run out of space,
> > +    * we must have actually used them before, so...they will be in the list.
> > +    */
> > +   assert(old_bo->index < batch->exec_count);
> > +   assert(batch->exec_bos[old_bo->index] == old_bo);
> > +
> > +   /* Update the validation list to use the new BO. */
> > +   batch->exec_bos[old_bo->index] = new_bo;
> > +   batch->validation_list[old_bo->index].handle = new_bo->gem_handle;
> 
> Nice touch.
> 
> > +   brw_bo_reference(new_bo);
> > +   brw_bo_unreference(old_bo);
> 
> > +
> > +   if (!batch->use_batch_first) {
> > +      /* We're not using I915_EXEC_HANDLE_LUT, which means we need to go
> > +       * update the relocation list entries to point at the new BO as well.
> > +       * (With newer kernels, the "handle" is an offset into the validation
> > +       * list, which remains unchanged, so we can skip this.)
> > +       */
> > +      replace_bo_in_reloc_list(&batch->batch_relocs,
> > +                               old_bo->gem_handle, new_bo->gem_handle);
> > +      replace_bo_in_reloc_list(&batch->state_relocs,
> > +                               old_bo->gem_handle, new_bo->gem_handle);
> > +   }
> > +
> > +   /* Drop the *bo_ptr reference.  This should free the old BO. */
> > +   brw_bo_unreference(old_bo);
> 
> Ok, just took a double take even with the comment in place.
> 
> > +   *bo_ptr = new_bo;
> > +   *map_ptr = new_map;
> > +}

Yeah...the double unreference is kind of spooky.  The alternative is to
make the batch and state buffers not referenced by the validation list,
which does save a few atomics, but adds a != batch_bo && != state_bo
check in the validation list loops...so...seemed better to just do this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170906/aa29fbac/attachment.sig>