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

[Chris Wilson]

Quoting Kenneth Graunke (2017-09-06 16:20:00)
> 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.

We don't copy the batch into the ring, we just stick a
MI_BATCH_BUFFER_START in there (with a flush before/after and a
breadcrumb, along with any context switch, change of mm, etc).

64k is indeed a magic limit for the state batch though, but as there's
no limit on the batch buffer size (after converting it to a pure command
stream, just the prospect of a timeout). Well... There is an implicit
assumption that you don't exceed 256KiB for a batch (as a limit for a gen2
workaround and a different gen7 workaround).

Elsewhere, I have used 256KiB batches (and then shrunk to fit) simply
because of UINT16_MAX dwords. (Which is kind of why the kernel assumes
that the upper reasonable maximum size is 256KiB for its w/a.)

> > > +   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.

It's not terrible, it just looked fishy! But I didn't like any of my
suggestions for the comment either.
-Chris