[Mesa-dev] [PATCH v4] i965: Avoid problems from referencing orphaned BOs after growing.

[Chris Wilson]

Quoting Kenneth Graunke (2018-01-18 07:36:05)
> 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.
> 
> v2/v3:
> - Handle stale pointers in the shadow copy case, where realloc may or
>   may not move our shadow copy to a new address.
> - Track the partial map explicitly, to avoid problems with buffer reuse
>   where multiple map modes exist (caught by Chris Wilson).
> 
> v4:
> - Don't use realloc in the CPU shadow case, it isn't safe.
> 
> Fixes: 2dfc119f22f257082ab0 "i965: Grow the batch/state buffers if we need space and can't flush."
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com> [v3]
> ---
>  src/mesa/drivers/dri/i965/brw_context.h       |   3 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 126 +++++++++++++++++++++-----
>  2 files changed, 105 insertions(+), 24 deletions(-)
> 
> I'm planning to land this before 18.0 branches unless there are any
> objections.

I've gone over this many times and I haven't spotted any problems, not
to say I haven't missed any, but

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris