[Mesa-dev] [PATCH v3 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
andrey simiklit
asimiklit.work at gmail.com
Fri Oct 26 08:06:52 UTC 2018
Hi,
Could you please help me with a push. I don't have a right for it :-)
Thanks,
Andrii.
On Thu, Oct 11, 2018 at 9:22 PM Jordan Justen <jordan.l.justen at intel.com>
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 <
> jordan.l.justen at intel.com>
> > wrote:
> >
> > > 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?
> > >
> >
> > 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 <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
> >
> > Regards,
> > Andrii.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181026/56400a18/attachment.html>
More information about the mesa-dev
mailing list