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 <[email protected]> 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, <[email protected]> wrote: > >> From: Andrii Simiklit <[email protected]> >> >> 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 <[email protected]> >> --- >> 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
