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

Mon Aug 27 09:39:08 UTC 2018

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.work at 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.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/20180827/8a41b068/attachment.html>