[Mesa-dev] [PATCH] i965/batch: don't ignore the 'brw_new_batch' call for a 'new batch'

[andrey simiklit]

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.work at gmail.com> wrote:

> From: Andrii Simiklit <andrii.simiklit at 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.simiklit at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180821/026d0c3e/attachment.html>