<div dir="ltr"><div>Hello,</div><div> </div><div>Thanks a lot) </div><div> </div><div>Regards,</div><div>Andrii.</div> <div class="gmail_quote"><div dir="ltr">On Tue, Oct 30, 2018 at 9:22 PM Jordan Justen <<a href="mailto:jordan.l.justen@intel.com">jordan.l.justen@intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 2018-10-26 01:06:52, andrey simiklit wrote: > Hi, > > Could you please help me with a push. I don't have a right for it :-) > I pushed these two patches. Thanks! e4e0fd5ffe1 i965/batch: don't ignore the 'brw_new_batch' call for a 'new batch' a9031bf9b55 i965/batch: avoid reverting batch buffer if saved state is an empty -Jordan </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > On Thu, Oct 11, 2018 at 9:22 PM Jordan Justen <<a href="mailto:jordan.l.justen@intel.com" target="_blank">jordan.l.justen@intel.com</a>> > wrote: > > > On 2018-10-11 03:01:56, andrey simiklit wrote: > > > Hello, > > > > > > Thanks for reviewing. > > > Please find my comment below. > > > > > > On Thu, Oct 11, 2018 at 12:37 AM Jordan Justen < > > <a href="mailto:jordan.l.justen@intel.com" target="_blank">jordan.l.justen@intel.com</a>> > > > wrote: > > > > > > > On 2018-09-12 09:05:44, wrote: > > > > > From: Andrii Simiklit <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>> > > > > > > > > > > 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 <<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>> > > > > > Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring > > > > > the batchbuffer state." > > > > > Bugzilla: <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> > > > > > --- > > > > > 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? > > > > > > > > > > We just predict that first step (non-failed branch without WARN_ONCE) > > > which should make the batch an empty will not make sense > > > because it is already empty and we immediately pass into a failed branch. > > > All WARN_ONCE calls are conditional ('ret == -ENOSPC') there. > > > I guess that if we came to the failed branch (I mean branch which > > contains > > > WARN_ONCE) > > > then regardless a reason (due to 'empty state' or 'failed try') we rather > > > should log a warning > > > that there isn't left space to transfer a batch if it is true. > > > > Oh. You are right. It sounds like a reasonable warning message for > > that case. So, there's no need to change the patch. > > > > -Jordan > > > > > > > > What do you think about it? > > > Should we log a warning if calls of > > > 'brw_upload_render_state' + 'brw_emit_prim' functions > > > for an empty batch lead to ENOSPC error? > > > > > > Anyway if it is unacceptable for you > > > I can implement the solution which you suggested. > > > > > > > > > > > > > > With that change, > > > > series Reviewed-by: Jordan Justen <<a href="mailto:jordan.l.justen@intel.com" target="_blank">jordan.l.justen@intel.com</a>> > > > > > > > > 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 > > > > > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > > > > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > > > Regards, > > > Andrii. > > </blockquote></div></div>