Hi all, Could somebody check it on CI to confirm that this patch do not add regressions or maybe even take a look at this patch.
Regards, Andrii. On Mon, Aug 27, 2018 at 12:39 PM andrey simiklit <asimiklit.w...@gmail.com> wrote: > Hi all, > > It would be great if somebody look at this. > I guess that this issue can affect every place where we use > 'intel_batchbuffer_save_state/intel_batchbuffer_reset_to_saved' but only > when the 'brw_batch_has_aperture_space' function returns false several > times in a row. > Pay attention that the last batch > <https://bugs.freedesktop.org/attachment.cgi?id=141200> of log has an > aperture with negative value "(-1023.8Mb aperture)". > Please correct me if I am incorrect. > > Regards, > Andrii. > On Tue, Aug 21, 2018 at 3:00 PM andrey simiklit <asimiklit.w...@gmail.com> > wrote: > >> Hi all, >> >> The bug for this issue was created: >> https://bugs.freedesktop.org/show_bug.cgi?id=107626 >> >> Regards, >> Andrii. >> >> >> On Mon, Aug 20, 2018 at 5:43 PM, <asimiklit.w...@gmail.com> wrote: >> >>> From: Andrii Simiklit <andrii.simik...@globallogic.com> >>> >>> If we restore the 'new batch' using 'intel_batchbuffer_reset_to_saved' >>> function we must restore the default state of the batch using >>> 'brw_new_batch' function because the 'intel_batchbuffer_flush' >>> function will not do it for the 'new batch' again. >>> At least the following fields of the batch >>> 'state_base_address_emitted','aperture_space', 'state_used' >>> should be restored to default values to avoid: >>> 1. the aperture_space overflow >>> 2. the missed STATE_BASE_ADDRESS commad in the batch >>> 3. the memory overconsumption of the 'statebuffer' >>> due to uncleared 'state_used' field. >>> etc. >>> >>> Signed-off-by: Andrii Simiklit <andrii.simik...@globallogic.com> >>> --- >>> src/mesa/drivers/dri/i965/intel_batchbuffer.c | 104 >>> +++++++++++++++----------- >>> 1 file changed, 59 insertions(+), 45 deletions(-) >>> >>> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c >>> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c >>> index 65d2c64..b8c5fb4 100644 >>> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c >>> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c >>> @@ -219,7 +219,7 @@ add_exec_bo(struct intel_batchbuffer *batch, struct >>> brw_bo *bo) >>> bo->index = batch->exec_count; >>> batch->exec_bos[batch->exec_count] = bo; >>> batch->aperture_space += bo->size; >>> - >>> + assert((batch->aperture_space >= 0) && "error >>> 'batch->aperture_space' field is overflown!"); >>> return batch->exec_count++; >>> } >>> >>> @@ -290,6 +290,51 @@ >>> intel_batchbuffer_reset_and_clear_render_cache(struct brw_context *brw) >>> brw_cache_sets_clear(brw); >>> } >>> >>> +/** >>> + * Called when starting a new batch buffer. >>> + */ >>> +static void >>> +brw_new_batch(struct brw_context *brw) >>> +{ >>> + /* Unreference any BOs held by the previous batch, and reset counts. >>> */ >>> + for (int i = 0; i < brw->batch.exec_count; i++) { >>> + brw_bo_unreference(brw->batch.exec_bos[i]); >>> + brw->batch.exec_bos[i] = NULL; >>> + } >>> + brw->batch.batch_relocs.reloc_count = 0; >>> + brw->batch.state_relocs.reloc_count = 0; >>> + brw->batch.exec_count = 0; >>> + brw->batch.aperture_space = 0; >>> + >>> + brw_bo_unreference(brw->batch.state.bo); >>> + >>> + /* Create a new batchbuffer and reset the associated state: */ >>> + intel_batchbuffer_reset_and_clear_render_cache(brw); >>> + >>> + /* If the kernel supports hardware contexts, then most hardware >>> state is >>> + * preserved between batches; we only need to re-emit state that is >>> required >>> + * to be in every batch. Otherwise we need to re-emit all the state >>> that >>> + * would otherwise be stored in the context (which for all intents >>> and >>> + * purposes means everything). >>> + */ >>> + if (brw->hw_ctx == 0) { >>> + brw->ctx.NewDriverState |= BRW_NEW_CONTEXT; >>> + brw_upload_invariant_state(brw); >>> + } >>> + >>> + brw->ctx.NewDriverState |= BRW_NEW_BATCH; >>> + >>> + brw->ib.index_size = -1; >>> + >>> + /* We need to periodically reap the shader time results, because >>> rollover >>> + * happens every few seconds. We also want to see results every >>> once in a >>> + * while, because many programs won't cleanly destroy our context, >>> so the >>> + * end-of-run printout may not happen. >>> + */ >>> + if (INTEL_DEBUG & DEBUG_SHADER_TIME) >>> + brw_collect_and_report_shader_time(brw); >>> +} >>> + >>> void >>> intel_batchbuffer_save_state(struct brw_context *brw) >>> { >>> @@ -311,6 +356,19 @@ intel_batchbuffer_reset_to_saved(struct brw_context >>> *brw) >>> brw->batch.exec_count = brw->batch.saved.exec_count; >>> >>> brw->batch.map_next = brw->batch.saved.map_next; >>> + if (USED_BATCH(brw->batch) == 0) >>> + { >>> + /** >>> + * The 'intel_batchbuffer_flush' function will not call >>> + * the 'brw_new_batch' function when the USED_BATCH returns 0. >>> + * It may leads to the few following issue: >>> + * The 'aperture_space' field can be overflown >>> + * The 'statebuffer' buffer contains the big unused space >>> + * The STATE_BASE_ADDRESS message is missed >>> + * etc >>> + **/ >>> + brw_new_batch(brw); >>> + } >>> } >>> >>> void >>> @@ -529,50 +587,6 @@ intel_batchbuffer_require_space(struct brw_context >>> *brw, GLuint sz) >>> } >>> } >>> >>> -/** >>> - * Called when starting a new batch buffer. >>> - */ >>> -static void >>> -brw_new_batch(struct brw_context *brw) >>> -{ >>> - /* Unreference any BOs held by the previous batch, and reset counts. >>> */ >>> - for (int i = 0; i < brw->batch.exec_count; i++) { >>> - brw_bo_unreference(brw->batch.exec_bos[i]); >>> - brw->batch.exec_bos[i] = NULL; >>> - } >>> - brw->batch.batch_relocs.reloc_count = 0; >>> - brw->batch.state_relocs.reloc_count = 0; >>> - brw->batch.exec_count = 0; >>> - brw->batch.aperture_space = 0; >>> - >>> - brw_bo_unreference(brw->batch.state.bo); >>> - >>> - /* Create a new batchbuffer and reset the associated state: */ >>> - intel_batchbuffer_reset_and_clear_render_cache(brw); >>> - >>> - /* If the kernel supports hardware contexts, then most hardware >>> state is >>> - * preserved between batches; we only need to re-emit state that is >>> required >>> - * to be in every batch. Otherwise we need to re-emit all the state >>> that >>> - * would otherwise be stored in the context (which for all intents >>> and >>> - * purposes means everything). >>> - */ >>> - if (brw->hw_ctx == 0) { >>> - brw->ctx.NewDriverState |= BRW_NEW_CONTEXT; >>> - brw_upload_invariant_state(brw); >>> - } >>> - >>> - brw->ctx.NewDriverState |= BRW_NEW_BATCH; >>> - >>> - brw->ib.index_size = -1; >>> - >>> - /* We need to periodically reap the shader time results, because >>> rollover >>> - * happens every few seconds. We also want to see results every >>> once in a >>> - * while, because many programs won't cleanly destroy our context, >>> so the >>> - * end-of-run printout may not happen. >>> - */ >>> - if (INTEL_DEBUG & DEBUG_SHADER_TIME) >>> - brw_collect_and_report_shader_time(brw); >>> -} >>> >>> /** >>> * Called from intel_batchbuffer_flush before emitting >>> MI_BATCHBUFFER_END and >>> -- >>> 2.7.4 >>> >>> >>
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
