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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev