On 28 August 2013 15:31, Eric Anholt <e...@anholt.net> wrote:
> Paul Berry <stereotype...@gmail.com> writes:
>
> > On 27 August 2013 15:21, Eric Anholt <e...@anholt.net> wrote:
> >
> >> This brings over the batch-wrap-prevention and aperture space checking
> >> code from the normal brw_draw.c path, so that we don't need to flush the
> >> batch every time.
> >>
> >> There's a risk here if the intel_emit_post_sync_nonzero_flush() call
> isn't
> >> high enough up in the state emit sequences -- before, we implicitly had
> >> one at the batch flush before any state was emitted, so Mesa's
> workaround
> >> emits didn't really matter.
> >>
> >> Improves cairo-gl performance by 13.7733% +/- 1.74876% (n=30/32)
> >> Improves minecraft apitrace performance by 1.03183% +/- 0.482297%
> (n=90).
> >> Reduces low-resolution GLB 2.7 performance by 1.17553% +/- 0.432263%
> (n=88)
> >> Reduces Lightsmark performance by 3.70246% +/- 0.322432% (n=126)
> >> No statistically significant performance difference on unigine tropics
> >> (n=10)
> >> No statistically significant performance difference on openarena (n=755)
> >>
> >> The two apps that are hurt happen to include stalls on busy buffer
> >> objects, so I think this is an effect of missing out on an opportune
> >> flush.
> >> ---
> >> src/mesa/drivers/dri/i965/brw_blorp.cpp | 50
> >> ++++++++++++++++++++++++++++++++
> >> src/mesa/drivers/dri/i965/brw_blorp.h | 4 ---
> >> src/mesa/drivers/dri/i965/gen6_blorp.cpp | 12 --------
> >> src/mesa/drivers/dri/i965/gen7_blorp.cpp | 1 -
> >> 4 files changed, 50 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> >> b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> >> index 1576ff2..c566d1d 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> >> @@ -21,6 +21,7 @@
> >> * IN THE SOFTWARE.
> >> */
> >>
> >> +#include <errno.h>
> >> #include "intel_batchbuffer.h"
> >> #include "intel_fbo.h"
> >>
> >> @@ -191,6 +192,26 @@ intel_hiz_exec(struct brw_context *brw, struct
> >> intel_mipmap_tree *mt,
> >> void
> >> brw_blorp_exec(struct brw_context *brw, const brw_blorp_params *params)
> >> {
> >> + struct gl_context *ctx = &brw->ctx;
> >> + uint32_t estimated_max_batch_usage = 1500;
> >> + bool check_aperture_failed_once = false;
> >> +
> >> + /* Flush the sampler and render caches. We definitely need to flush
> >> the
> >> + * sampler cache so that we get updated contents from the render
> cache
> >> for
> >> + * the glBlitFramebuffer() source. Also, we are sometimes warned in
> >> the
> >> + * docs to flush the cache between reinterpretations of the same
> >> surface
> >> + * data with different formats, which blorp does for stencil and
> depth
> >> + * data.
> >> + */
> >> + intel_batchbuffer_emit_mi_flush(brw);
> >> +
> >> +retry:
> >> + intel_batchbuffer_require_space(brw, estimated_max_batch_usage,
> false);
> >> + intel_batchbuffer_save_state(brw);
> >> + drm_intel_bo *saved_bo = brw->batch.bo;
> >> + uint32_t saved_used = brw->batch.used;
> >> + uint32_t saved_state_batch_offset = brw->batch.state_batch_offset;
> >> +
> >> switch (brw->gen) {
> >> case 6:
> >> gen6_blorp_exec(brw, params);
> >> @@ -204,6 +225,35 @@ brw_blorp_exec(struct brw_context *brw, const
> >> brw_blorp_params *params)
> >> break;
> >> }
> >>
> >>
> > Would it be feasible to add an assertion here to verify that the amount
> of
> > batch space actually used by this blorp call is less than or equal to
> > estimated_max_batch_usage? That would give me a lot of increased
> > confidence that the magic number 1500 is correct.
> >
> > With the added assertion, the series is:
> >
> > Reviewed-by: Paul Berry <stereotype...@gmail.com>
>
> That's this code:
>
> + /* Make sure we didn't wrap the batch unintentionally, and make sure we
> + * reserved enough space that a wrap will never happen.
> + */
> + assert(brw->batch.bo == saved_bo);
> + assert((brw->batch.used - saved_used) * 4 +
> + (saved_state_batch_offset - brw->batch.state_batch_offset) <
> + estimated_max_batch_usage);
>
You're right. I misunderstood and thought these assertions were merely
checking that we didn't run out of room. Patch is:
Reviewed-by: Paul Berry <stereotype...@gmail.com>
as is.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
