Hello, Thanks for your reply. Please find my comments below:
On Mon, Sep 10, 2018 at 2:45 PM Chris Wilson <ch...@chris-wilson.co.uk> wrote: > Quoting andrey simiklit (2018-08-21 13:00:57) > > Hi all, > > > > The bug for this issue was created: > > https://bugs.freedesktop.org/show_bug.cgi?id=107626 > > What about something like > Yes I think it is good idea. One more point, there are multiple places where 'saving/restoring' api is used and I guess that each should be fixed. That is why I added my solution into location where it could fix each of them. But your solution will be better for performance I think. Is it acceptable for you to merge our solutions to: --- 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 | 12 ++++++++++++ src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 + 5 files changed, 19 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); 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..e0b7c94 100644 --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c @@ -57,6 +57,9 @@ static void intel_batchbuffer_reset(struct brw_context *brw); static void +brw_new_batch(struct brw_context *brw); + +static void dump_validation_list(struct intel_batchbuffer *batch) { fprintf(stderr, "Validation list (length %d):\n", batch->exec_count); @@ -299,6 +302,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) { @@ -311,6 +321,8 @@ 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) + brw_new_batch(brw); } void 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, -- I think it could make this api more flexible/understandable for users. The 'intel_batchbuffer_reset_to_saved' is created to reset the batch state therefore it should be able to reset even to the 'empty batch' or at least some error should be returned for 'empty batch' case. > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c > b/src/mesa/drivers/dri/i965/brw_draw.c > index 8536c040109..babb8d4676d 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 = brw->batch.saved.map_next == 0; > I guess it is should be like: fail_next = (brw->batch.saved.map_next == brw->batch.batch.map); > > if (brw->num_instances != prim->num_instances || > brw->basevertex != prim->basevertex || > > There's no point reverting to the last saved point if that save point is > the empty batch, we will just repeat ourselves. > -Chris > Regards, Andrii.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
