[Mesa-dev] [PATCH resend 1/2] i965: Avoid problems from referencing orphaned BOs after growing.

[Chris Wilson]

Quoting Kenneth Graunke (2018-01-05 18:56:32)
> Growing the batch/state buffer is a lot more dangerous than I thought.
> 
> A number of places emit multiple state buffer sections, and then write
> data to the returned pointer, or save a pointer to brw->batch.state.bo
> and then use it in relocations.  If each call can grow, this can result
> in stale map references or stale BO pointers.  Furthermore, fences refer
> to the old batch BO, and that reference needs to continue working.
> 
> To avoid these woes, we avoid ever swapping the brw->batch.*.bo pointer,
> instead exchanging the brw_bo structures in place.  That way, stale BO
> references are fine - the GEM handle changes, but the brw_bo pointer
> doesn't.  We also defer the memcpy until a quiescent point, so callers
> can write to the returned pointer - which may be in either BO - and
> we'll sort it out and combine the two properly in the end.
> 
> Fixes: 2dfc119f22f257082ab0 "i965: Grow the batch/state buffers if we need space and can't flush."
> ---
>  src/mesa/drivers/dri/i965/brw_context.h       |   2 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 139 ++++++++++++++++++++------
>  2 files changed, 108 insertions(+), 33 deletions(-)
> 
> Resending these patches from November.  We've still got random 
> entry->handle == batch->batch.bo->gem_handle assertion failures.
> I think this patch should fix those issues.  Without it, you can
> end up with data in the wrong BO, or both statebuffers in the
> validation list, which will likely lead to asserts or hangs.
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index 0f0aad85348..2a96cdeceb1 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -474,6 +474,8 @@ struct brw_growing_bo {
>     struct brw_bo *bo;
>     uint32_t *map;
>     uint32_t *cpu_map;
> +   struct brw_bo *partial_bo;
> +   unsigned partial_bytes;
>  };
>  
>  struct intel_batchbuffer {
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 0e090f46250..c28216c0f11 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -174,6 +174,8 @@ intel_batchbuffer_reset(struct brw_context *brw)
>     batch->last_bo = batch->batch.bo;
>  
>     batch->batch.bo = brw_bo_alloc(bufmgr, "batchbuffer", BATCH_SZ, 4096);
> +   batch->batch.partial_bo = NULL;
> +   batch->batch.partial_bytes = 0;
>     if (!batch->batch.cpu_map) {
>        batch->batch.map =
>           brw_bo_map(brw, batch->batch.bo, MAP_READ | MAP_WRITE);
> @@ -183,6 +185,8 @@ intel_batchbuffer_reset(struct brw_context *brw)
>     batch->state.bo = brw_bo_alloc(bufmgr, "statebuffer", STATE_SZ, 4096);
>     batch->state.bo->kflags =
>        can_do_exec_capture(screen) ? EXEC_OBJECT_CAPTURE : 0;
> +   batch->state.partial_bo = NULL;
> +   batch->state.partial_bytes = 0;

Smells a bit repetitive. alloc_growing_bo()?

>     if (!batch->state.cpu_map) {
>        batch->state.map =
>           brw_bo_map(brw, batch->state.bo, MAP_READ | MAP_WRITE);
> @@ -261,6 +265,30 @@ intel_batchbuffer_free(struct intel_batchbuffer *batch)
>        _mesa_hash_table_destroy(batch->state_batch_sizes, NULL);
>  }
>  
> +/**
> + * Finish copying the old batch/state buffer's contents to the new one
> + * after we tried to "grow" the buffer in an earlier operation.
> + */
> +static void
> +finish_growing_bos(struct brw_growing_bo *grow)
> +{
> +   struct brw_bo *old_bo = grow->partial_bo;
> +   if (!old_bo)
> +      return;
> +
> +   if (!grow->cpu_map) {
> +      /* This works because we only ever do one type of map */
> +      void *old_map = old_bo->map_cpu ? old_bo->map_cpu :
> +                      (old_bo->map_wc ? old_bo->map_wc : old_bo->map_gtt);

But the bo may be recycled from something that used a different map.
Right?

I'd much rather see grow->partial_map explicitly tracked.

struct batch_bo {
	struct brw_bo *bo;
	uint32_t *map;
	uint32_t *cpu_map;
};

struct growing_bo {
	struct batch_bo final;
	struct batch_bo partial;
};
?
-Chris