<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br><div dir="ltr">Hello,<br><br>Thanks for your reply.<br>Please find my comments below:<div dir="ltr"><div class="gmail_quote"><div dir="ltr"><br></div><div dir="ltr">On Mon, Sep 10, 2018 at 2:45 PM Chris Wilson <<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting andrey simiklit (2018-08-21 13:00:57)<br>
> Hi all,<br>
> <br>
> The bug for this issue was created:<br>
> <a href="https://bugs.freedesktop.org/show_bug.cgi?id=107626" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=107626</a><br>
<br>
What about something like<br></blockquote><div> </div>Yes I think it is good idea.<br>One more point, there are multiple places where 'saving/restoring' api is used and<br>I guess that each should be fixed.<br>That is why I added my solution into location where it could fix each of them.<br>But your solution will be better for performance I think.<br>Is it acceptable for you to merge our solutions to:</div><div class="gmail_quote"><div><br></div><div>---<br> src/mesa/drivers/dri/i965/brw_compute.c | 3 ++-<br> src/mesa/drivers/dri/i965/brw_draw.c | 3 ++-<br> src/mesa/drivers/dri/i965/genX_blorp_exec.c | 3 ++-<br> src/mesa/drivers/dri/i965/intel_batchbuffer.c | 12 ++++++++++++<br> src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +<br> 5 files changed, 19 insertions(+), 3 deletions(-)<br><br>diff --git a/src/mesa/drivers/dri/i965/brw_compute.c b/src/mesa/drivers/dri/i965/brw_compute.c<br>index de08fc3..5c8e3a5 100644<br>--- a/src/mesa/drivers/dri/i965/brw_compute.c<br>+++ b/src/mesa/drivers/dri/i965/brw_compute.c<br>@@ -167,7 +167,7 @@ static void<br> brw_dispatch_compute_common(struct gl_context *ctx)<br> {<br> struct brw_context *brw = brw_context(ctx);<br>- bool fail_next = false;<br>+ bool fail_next;<br> <br> if (!_mesa_check_conditional_render(ctx))<br> return;<br>@@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)<br> intel_batchbuffer_require_space(brw, 600);<br> brw_require_statebuffer_space(brw, 2500);<br> intel_batchbuffer_save_state(brw);<br>+ fail_next = intel_batchbuffer_saved_state_is_empty(brw);<br> <br> retry:<br> brw->batch.no_wrap = true;<br>diff --git a/src/mesa/drivers/dri/i965/brw_draw.c b/src/mesa/drivers/dri/i965/brw_draw.c<br>index 8536c04..19ee396 100644<br>--- a/src/mesa/drivers/dri/i965/brw_draw.c<br>+++ b/src/mesa/drivers/dri/i965/brw_draw.c<br>@@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,<br> {<br> struct brw_context *brw = brw_context(ctx);<br> const struct gen_device_info *devinfo = &brw->screen->devinfo;<br>- bool fail_next = false;<br>+ bool fail_next;<br> <br> /* Flag BRW_NEW_DRAW_CALL on every draw. This allows us to have<br> * atoms that happen on every draw call.<br>@@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,<br> intel_batchbuffer_require_space(brw, 1500);<br> brw_require_statebuffer_space(brw, 2400);<br> intel_batchbuffer_save_state(brw);<br>+ fail_next = intel_batchbuffer_saved_state_is_empty(brw);<br> <br> if (brw->num_instances != prim->num_instances ||<br> brw->basevertex != prim->basevertex ||<br>diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c b/src/mesa/drivers/dri/i965/genX_blorp_exec.c<br>index 34bfcad..fd9ce93 100644<br>--- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c<br>+++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c<br>@@ -268,7 +268,7 @@ genX(blorp_exec)(struct blorp_batch *batch,<br> assert(batch->blorp->driver_ctx == batch->driver_batch);<br> struct brw_context *brw = batch->driver_batch;<br> struct gl_context *ctx = &brw->ctx;<br>- bool check_aperture_failed_once = false;<br>+ bool check_aperture_failed_once;<br> <br> #if GEN_GEN >= 11<br> /* The PIPE_CONTROL command description says:<br>@@ -309,6 +309,7 @@ retry:<br> intel_batchbuffer_require_space(brw, 1400);<br> brw_require_statebuffer_space(brw, 600);<br> intel_batchbuffer_save_state(brw);<br>+ check_aperture_failed_once = intel_batchbuffer_saved_state_is_empty(brw);<br> brw->batch.no_wrap = true;<br> <br> #if GEN_GEN == 6<br>diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c<br>index 4363b14..e0b7c94 100644<br>--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c<br>+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c<br>@@ -57,6 +57,9 @@ static void<br> intel_batchbuffer_reset(struct brw_context *brw);<br> <br> static void<br>+brw_new_batch(struct brw_context *brw);<br>+<br>+static void<br> dump_validation_list(struct intel_batchbuffer *batch)<br> {<br> fprintf(stderr, "Validation list (length %d):\n", batch->exec_count);<br>@@ -299,6 +302,13 @@ intel_batchbuffer_save_state(struct brw_context *brw)<br> brw->batch.saved.exec_count = brw->batch.exec_count;<br> }<br> <br>+bool<br>+intel_batchbuffer_saved_state_is_empty(struct brw_context *brw)<br>+{<br>+ struct intel_batchbuffer *batch = &brw->batch;<br>+ return (batch->saved.map_next == batch->batch.map);<br>+}<br>+<br> void<br> intel_batchbuffer_reset_to_saved(struct brw_context *brw)<br> {<br>@@ -311,6 +321,8 @@ intel_batchbuffer_reset_to_saved(struct brw_context *brw)<br> brw->batch.exec_count = brw->batch.saved.exec_count;<br> <br> brw->batch.map_next = brw->batch.saved.map_next;<br>+ if (USED_BATCH(brw->batch) == 0)<br>+ brw_new_batch(brw);<br> }<br> <br> void<br>diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h b/src/mesa/drivers/dri/i965/intel_batchbuffer.h<br>index 0632142..91720da 100644<br>--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h<br>+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h<br>@@ -24,6 +24,7 @@ struct intel_batchbuffer;<br> void intel_batchbuffer_init(struct brw_context *brw);<br> void intel_batchbuffer_free(struct intel_batchbuffer *batch);<br> void intel_batchbuffer_save_state(struct brw_context *brw);<br>+bool intel_batchbuffer_saved_state_is_empty(struct brw_context *brw);<br> void intel_batchbuffer_reset_to_saved(struct brw_context *brw);<br> void intel_batchbuffer_require_space(struct brw_context *brw, GLuint sz);<br> int _intel_batchbuffer_flush_fence(struct brw_context *brw,<br>-- <br></div><br>I think it could make this api more flexible/understandable for users.<br>The 'intel_batchbuffer_reset_to_saved' is created to reset the batch state<br>therefore it should be able to reset even to the 'empty batch' or<br>at least some error should be returned for 'empty batch' case.<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_draw.c b/src/mesa/drivers/dri/i965/brw_draw.c<br>
index 8536c040109..babb8d4676d 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_draw.c<br>
+++ b/src/mesa/drivers/dri/i965/brw_draw.c<br>
@@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,<br>
{<br>
struct brw_context *brw = brw_context(ctx);<br>
const struct gen_device_info *devinfo = &brw->screen->devinfo;<br>
- bool fail_next = false;<br>
+ bool fail_next;<br>
<br>
/* Flag BRW_NEW_DRAW_CALL on every draw. This allows us to have<br>
* atoms that happen on every draw call.<br>
@@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,<br>
intel_batchbuffer_require_space(brw, 1500);<br>
brw_require_statebuffer_space(brw, 2400);<br>
intel_batchbuffer_save_state(brw);<br>
+ fail_next = brw->batch.saved.map_next == 0;<br></blockquote><div><br></div><div>I guess it is should be like:</div><div>fail_next = (brw->batch.saved.map_next == brw->batch.batch.map);</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
if (brw->num_instances != prim->num_instances ||<br>
brw->basevertex != prim->basevertex ||<br>
<br>
There's no point reverting to the last saved point if that save point is<br>
the empty batch, we will just repeat ourselves.<br>
-Chris<br></blockquote><div><br></div><div>Regards,</div><div>Andrii.<br></div></div></div></div></div></div></div></div></div></div></div></div></div>