[Mesa-dev] [PATCH v3 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty

[Jordan Justen]

On 2018-09-12 09:05:44,  wrote:
> From: Andrii Simiklit <andrii.simiklit at globallogic.com>
> 
> There's no point reverting to the last saved point if that save point is
> the empty batch, we will just repeat ourselves.
> 
> CC: Chris Wilson <chris at chris-wilson.co.uk>
> Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring
>                      the batchbuffer state."
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107626
> ---
>  src/mesa/drivers/dri/i965/brw_compute.c       | 3 ++-
>  src/mesa/drivers/dri/i965/brw_draw.c          | 3 ++-
>  src/mesa/drivers/dri/i965/genX_blorp_exec.c   | 3 ++-
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++++++
>  src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
>  5 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_compute.c b/src/mesa/drivers/dri/i965/brw_compute.c
> index de08fc3..5c8e3a5 100644
> --- a/src/mesa/drivers/dri/i965/brw_compute.c
> +++ b/src/mesa/drivers/dri/i965/brw_compute.c
> @@ -167,7 +167,7 @@ static void
>  brw_dispatch_compute_common(struct gl_context *ctx)
>  {
>     struct brw_context *brw = brw_context(ctx);
> -   bool fail_next = false;
> +   bool fail_next;
>  
>     if (!_mesa_check_conditional_render(ctx))
>        return;
> @@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
>     intel_batchbuffer_require_space(brw, 600);
>     brw_require_statebuffer_space(brw, 2500);
>     intel_batchbuffer_save_state(brw);
> +   fail_next = intel_batchbuffer_saved_state_is_empty(brw);
>  
>   retry:
>     brw->batch.no_wrap = true;
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c b/src/mesa/drivers/dri/i965/brw_draw.c
> index 8536c04..19ee396 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,
>  {
>     struct brw_context *brw = brw_context(ctx);
>     const struct gen_device_info *devinfo = &brw->screen->devinfo;
> -   bool fail_next = false;
> +   bool fail_next;
>  
>     /* Flag BRW_NEW_DRAW_CALL on every draw.  This allows us to have
>      * atoms that happen on every draw call.
> @@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,
>     intel_batchbuffer_require_space(brw, 1500);
>     brw_require_statebuffer_space(brw, 2400);
>     intel_batchbuffer_save_state(brw);
> +   fail_next = intel_batchbuffer_saved_state_is_empty(brw);

It seems like this will cause the WARN_ONCE to be hit incorrectly.
What about adding a 'bool empty_state', and checking that before
fail_next in the code that follows. If empty_state is true, I guess
you want to flush, but not emit the WARN_ONCE?

With that change,
series Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>

As always, it could be nice to get an r-b, or acked-by from Ken. :)

-Jordan

>  
>     if (brw->num_instances != prim->num_instances ||
>         brw->basevertex != prim->basevertex ||
> diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> index 34bfcad..fd9ce93 100644
> --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> @@ -268,7 +268,7 @@ genX(blorp_exec)(struct blorp_batch *batch,
>     assert(batch->blorp->driver_ctx == batch->driver_batch);
>     struct brw_context *brw = batch->driver_batch;
>     struct gl_context *ctx = &brw->ctx;
> -   bool check_aperture_failed_once = false;
> +   bool check_aperture_failed_once;
>  
>  #if GEN_GEN >= 11
>     /* The PIPE_CONTROL command description says:
> @@ -309,6 +309,7 @@ retry:
>     intel_batchbuffer_require_space(brw, 1400);
>     brw_require_statebuffer_space(brw, 600);
>     intel_batchbuffer_save_state(brw);
> +   check_aperture_failed_once = intel_batchbuffer_saved_state_is_empty(brw);
>     brw->batch.no_wrap = true;
>  
>  #if GEN_GEN == 6
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 4363b14..2dc6eb8 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -299,6 +299,13 @@ intel_batchbuffer_save_state(struct brw_context *brw)
>     brw->batch.saved.exec_count = brw->batch.exec_count;
>  }
>  
> +bool
> +intel_batchbuffer_saved_state_is_empty(struct brw_context *brw)
> +{
> +   struct intel_batchbuffer *batch = &brw->batch;
> +   return (batch->saved.map_next == batch->batch.map);
> +}
> +
>  void
>  intel_batchbuffer_reset_to_saved(struct brw_context *brw)
>  {
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> index 0632142..91720da 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> @@ -24,6 +24,7 @@ struct intel_batchbuffer;
>  void intel_batchbuffer_init(struct brw_context *brw);
>  void intel_batchbuffer_free(struct intel_batchbuffer *batch);
>  void intel_batchbuffer_save_state(struct brw_context *brw);
> +bool intel_batchbuffer_saved_state_is_empty(struct brw_context *brw);
>  void intel_batchbuffer_reset_to_saved(struct brw_context *brw);
>  void intel_batchbuffer_require_space(struct brw_context *brw, GLuint sz);
>  int _intel_batchbuffer_flush_fence(struct brw_context *brw,
> -- 
> 2.7.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev