On 11/11/2013 01:35 AM, Kevin Rogovin wrote:
> This patch adds a function interface for enabling no wrap on batch commands,
> adds to it assert enforcement that the number bytes added to the
> batch buffer does not exceed a passed value and finally this is used
> in brw_try_draw_prims() to help make sure that estimated_max_prim_size
> is a good value.
My style comments in addition to Matt's are below.
> ---
> src/mesa/drivers/dri/i965/brw_context.h | 64
> +++++++++++++++++++++++++++
> src/mesa/drivers/dri/i965/brw_draw.c | 4 +-
> src/mesa/drivers/dri/i965/brw_state_batch.c | 15 ++++++-
> src/mesa/drivers/dri/i965/intel_batchbuffer.h | 5 +++
> 4 files changed, 84 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index 8b1cbb3..953f2cf 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1028,8 +1028,29 @@ struct brw_context
> uint32_t reset_count;
>
> struct intel_batchbuffer batch;
> +
> + /*!\var no_batch_wrap
> + While no_batch_wrap is true, the batch buffer must not
> + be flushed. Use the functions begin_no_batch_wrap() and
> + end_no_batch_wrap() to mark the start and end points
> + that the batch buffer must not be flushed.
> + */
The way we've been using Doxygen comments is more like:
/**
* Brief description
*
* Detailed description in the cases where it's actually
* necessary.
*/
> bool no_batch_wrap;
>
> + /*!\var max_expected_batch_size_during_no_batch_wrap
> + If \ref no_batch_wrap is true, specifies the number
> + of bytes that are expected before \ref no_batch_wrap
> + is set to false.
> + */
> + int max_expected_batch_size_during_no_batch_wrap;
> +
> + /*!\var number_bytes_consumed_during_no_batch_wrap
> + records the number of bytes consumed so far
> + in the batch buffer since the last time \ref
> + no_batch_wrap was set to true
> + */
> + int number_bytes_consumed_during_no_batch_wrap;
> +
> struct {
> drm_intel_bo *bo;
> GLuint offset;
> @@ -1450,6 +1471,49 @@ is_power_of_two(uint32_t value)
> return (value & (value - 1)) == 0;
> }
>
> +/*!\fn begin_no_batch_wrap
> + Function to mark the start of a sequence of commands and state
> + added to the batch buffer that must not be partitioned by
> + a flush.
> + Requirements:
> + - no_batch_wrap is false
> +
> + Output/side effects:
> + - no_batch_wrap set to true
> + - max_expected_batch_size_during_no_batch_wrap set
> + - number_bytes_consumed_during_no_batch_wrap reset to 0
> +
> + \ref brw "GL context"
> + \ref pmax_expected_batch_size value specifying expected maximum number of
> bytes to
> + be consumed in the batch buffer
> + */
Likewise for functions:
/**
* Brief description of function
*
* Details about the implementation assumptions, etc.
*
* \param brw Context pointer
* \param pmax_expected_batch_size Value specifying expected maximum
* number of bytes to be consumed in
* the batch buffer.
*/
> +static INLINE void
> +begin_no_batch_wrap(struct brw_context *brw, int pmax_expected_batch_size)
> +{
> + assert(!brw->no_batch_wrap);
> + brw->no_batch_wrap=true;
> +
> brw->max_expected_batch_size_during_no_batch_wrap=pmax_expected_batch_size;
> + brw->number_bytes_consumed_during_no_batch_wrap=0;
> +}
> +
> +/*!\fn end_no_batch_wrap
> + Function to mark the end of a sequence of commands and state
> + added to the batch buffer that must not be partitioned by
> + a flush.
> + Requirements:
> + - no_batch_wrap is true
> +
> + Output/side effects:
> + - no_batch_wrap set to false
> + */
> +static INLINE void
> +end_no_batch_wrap(struct brw_context *brw)
> +{
> + assert(brw->no_batch_wrap);
> + brw->no_batch_wrap=false;
> +}
> +
> +
> /*======================================================================
> * brw_vtbl.c
> */
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> index 7b33b76..12f0ffe 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -416,14 +416,14 @@ retry:
> * *_set_prim or intel_batchbuffer_flush(), which only impacts
> * brw->state.dirty.brw.
> */
> + begin_no_batch_wrap(brw, estimated_max_prim_size);
> if (brw->state.dirty.brw) {
> - brw->no_batch_wrap = true;
> brw_upload_state(brw);
> }
>
> brw_emit_prim(brw, &prims[i], brw->primitive);
> + end_no_batch_wrap(brw);
>
> - brw->no_batch_wrap = false;
>
> if (dri_bufmgr_check_aperture_space(&brw->batch.bo, 1)) {
> if (!fail_next) {
> diff --git a/src/mesa/drivers/dri/i965/brw_state_batch.c
> b/src/mesa/drivers/dri/i965/brw_state_batch.c
> index c71d2f3..ff51c21 100644
> --- a/src/mesa/drivers/dri/i965/brw_state_batch.c
> +++ b/src/mesa/drivers/dri/i965/brw_state_batch.c
> @@ -9,8 +9,7 @@
> without limitation the rights to use, copy, modify, merge, publish,
> distribute, sublicense, and/or sell copies of the Software, and to
> permit persons to whom the Software is furnished to do so, subject to
> - the following conditions:
> -
> + the following conditions:
> The above copyright notice and this permission notice (including the
> next paragraph) shall be included in all copies or substantial
> portions of the Software.
> @@ -127,6 +126,18 @@ brw_state_batch(struct brw_context *brw,
> assert(size < batch->bo->size);
> offset = ROUND_DOWN_TO(batch->state_batch_offset - size, alignment);
>
> +#ifdef DEBUG
> + if(brw->no_batch_wrap) {
> + /*
> + although the request is for size bytes, the consumption can be
> greater
> + because of alignment, thus we use the differences of the new-to-be
> offset
> + with the old offset value.
> + */
> + brw->number_bytes_consumed_during_no_batch_wrap+=
> (batch->state_batch_offset-offset);
> + assert(brw->number_bytes_consumed_during_no_batch_wrap <=
> brw->max_expected_batch_size_during_no_batch_wrap);
> + }
> +#endif
> +
> /* If allocating from the top would wrap below the batchbuffer, or
> * if the batch's used space (plus the reserved pad) collides with our
> * space, then flush and try again.
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> index d46f48e..776f98e 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> @@ -111,7 +111,12 @@ intel_batchbuffer_require_space(struct brw_context *brw,
> GLuint sz, int is_blit)
>
> #ifdef DEBUG
> assert(sz < BATCH_SZ - BATCH_RESERVED);
> + if(brw->no_batch_wrap) {
> + brw->number_bytes_consumed_during_no_batch_wrap+=sz;
> + assert(brw->number_bytes_consumed_during_no_batch_wrap <=
> brw->max_expected_batch_size_during_no_batch_wrap);
> + }
> #endif
> +
> if (intel_batchbuffer_space(brw) < sz)
> intel_batchbuffer_flush(brw);
> }
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
