Re: [Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-12 Thread Eric Anholt 
Rogovin, Kevin kevin.rogo...@intel.com writes:

 Hi all, I will later submit a patch taking into account comments, however one 
 comment I will address *now*.

 Eric Anholt [e...@anholt.net] writes:

Kevin Rogovin kevin.rogo...@intel.com writes:

 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.

I don't like adding overhead to every batch operation.  You can just do
an assert like I did in 185b5a54c94ce11487146042c8eec24909187ed6

 That approach used in brw_blorp_exec.cpp will not work here because the 
 estimate
 is (and should be) computed in brw_try_draw_prims() and the assert needs to 
 be done
 whenever commands or state are added to the batch buffer. Additionally it is 
 literally an
 overhead or exactly writing one boolean and two integers _per_ draw call. 
 This overhead
 is literally insignificant next to the overhead of the call stack to reach 
 brw_try_draw_primis().

You added code to intel_batchbuffer_require_space, which is called from
every BEGIN_BATCH.

You should simply verify at the end of the brw_try_draw_prims that we
didn't exceed the space we estimated, same as the strategy in blorp.  It
doesn't help to know which particular BEGIN_BATCH pushed you over the
limit.


pgpfFlOSbobNb.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-11 Thread Kevin Rogovin 
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.

---
 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.
+*/
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  
+ */ 
+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

Re: [Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-11 Thread Matt Turner 
Need a better commit message. I like the kernel's guidelines which
remind us that the summary becomes the globally-unique identifier for
the patch.

Style comments:

On Mon, Nov 11, 2013 at 1:35 AM, Kevin Rogovin kevin.rogo...@intel.com 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.

 ---
  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.
 +*/

Multiline comments should be

/*
 *
 *
 */

and put a space after /*

 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
 + */
 +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:

Don't think you meant to make this change?

   The above

Re: [Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-11 Thread Ian Romanick 
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
 +++

Re: [Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-11 Thread Eric Anholt 
Kevin Rogovin kevin.rogo...@intel.com writes:

 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.

I don't like adding overhead to every batch operation.  You can just do
an assert like I did in 185b5a54c94ce11487146042c8eec24909187ed6


pgpgsiOZon9hC.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-11 Thread Rogovin, Kevin 
Hi all, I will later submit a patch taking into account comments, however one 
comment I will address *now*.

Eric Anholt [e...@anholt.net] writes:

Kevin Rogovin kevin.rogo...@intel.com writes:

 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.

I don't like adding overhead to every batch operation.  You can just do
an assert like I did in 185b5a54c94ce11487146042c8eec24909187ed6

That approach used in brw_blorp_exec.cpp will not work here because the estimate
is (and should be) computed in brw_try_draw_prims() and the assert needs to be 
done
whenever commands or state are added to the batch buffer. Additionally it is 
literally an
overhead or exactly writing one boolean and two integers _per_ draw call. This 
overhead
is literally insignificant next to the overhead of the call stack to reach 
brw_try_draw_primis().

However, I will make it so that the write to those variables only occurs in 
debug, since the
assert is only for the purpose of debug; going further those members will only 
exists in debug
for that matter.

From: Eric Anholt [e...@anholt.net]
Sent: Monday, November 11, 2013 9:56 PM
To: Rogovin, Kevin; mesa-dev@lists.freedesktop.org
Cc: Rogovin, Kevin
Subject: Re: [Mesa-dev] [PATCH] nicer-no-wrap-patch

Kevin Rogovin kevin.rogo...@intel.com writes:

 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.

I don't like adding overhead to every batch operation.  You can just do
an assert like I did in 185b5a54c94ce11487146042c8eec24909187ed6
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-11 Thread Kevin Rogovin 
feedback integrated

space-pace
---
 src/mesa/drivers/dri/i965/brw_context.h   | 85 +++
 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 |  6 ++
 4 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index 8b1cbb3..638e570 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -1028,8 +1028,37 @@ struct brw_context
uint32_t reset_count;
 
struct intel_batchbuffer batch;
+
+   /**
+* Debug-only: flag to indicate to that batch buffer flush 
+* should not happen
+* 
+* 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.
+*/
bool no_batch_wrap;
 
+   /**
+* Debug-only: expected maximum number bytes added to 
+* batch buffer
+*
+* 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;
+
+   /**
+* Debug-only: current number of bytes added to batch buffer
+*
+* 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 +1479,62 @@ is_power_of_two(uint32_t value)
return (value  (value - 1)) == 0;
 }
 
+/**
+ * Affect only in debug: Begin mark of don't flush batch buffer
+ *
+ * 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 context pointer
+ * \ref 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)
+{
+#ifdef DEBUG
+   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;
+#else
+   (void)brw;
+   (void)pmax_expected_batch_size;
+#endif
+}
+
+/**
+ * Affect only in Debug only: end mark of don't flush batch buffer
+ *
+ * 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)
+{
+#ifdef DEBUG
+   assert(brw-no_batch_wrap);
+   brw-no_batch_wrap=false;
+#else
+   (void)brw;
+#endif
+}
+
+
 /*==
  * 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..60d3c2d 100644
--- a/src/mesa/drivers/dri/i965/brw_state_batch.c
+++ b/src/mesa/drivers/dri/i965/brw_state_batch.c
@@ -127,6 +127,21 @@ 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 = 
+

[Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-11 Thread Kevin Rogovin 
Track bytes written during no flush phases for debug builds 


---
 src/mesa/drivers/dri/i965/brw_context.h   | 85 +++
 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 |  6 ++
 4 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index 8b1cbb3..638e570 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -1028,8 +1028,37 @@ struct brw_context
uint32_t reset_count;
 
struct intel_batchbuffer batch;
+
+   /**
+* Debug-only: flag to indicate to that batch buffer flush 
+* should not happen
+* 
+* 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.
+*/
bool no_batch_wrap;
 
+   /**
+* Debug-only: expected maximum number bytes added to 
+* batch buffer
+*
+* 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;
+
+   /**
+* Debug-only: current number of bytes added to batch buffer
+*
+* 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 +1479,62 @@ is_power_of_two(uint32_t value)
return (value  (value - 1)) == 0;
 }
 
+/**
+ * Affect only in debug: Begin mark of don't flush batch buffer
+ *
+ * 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 context pointer
+ * \ref 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)
+{
+#ifdef DEBUG
+   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;
+#else
+   (void)brw;
+   (void)pmax_expected_batch_size;
+#endif
+}
+
+/**
+ * Affect only in Debug only: end mark of don't flush batch buffer
+ *
+ * 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)
+{
+#ifdef DEBUG
+   assert(brw-no_batch_wrap);
+   brw-no_batch_wrap=false;
+#else
+   (void)brw;
+#endif
+}
+
+
 /*==
  * 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..60d3c2d 100644
--- a/src/mesa/drivers/dri/i965/brw_state_batch.c
+++ b/src/mesa/drivers/dri/i965/brw_state_batch.c
@@ -127,6 +127,21 @@ 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 = 
+

[Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-11 Thread Kevin Rogovin 
feedback integrated

space-pace
---
 src/mesa/drivers/dri/i965/brw_context.h   | 85 +++
 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 |  6 ++
 4 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index 8b1cbb3..638e570 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -1028,8 +1028,37 @@ struct brw_context
uint32_t reset_count;
 
struct intel_batchbuffer batch;
+
+   /**
+* Debug-only: flag to indicate to that batch buffer flush 
+* should not happen
+* 
+* 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.
+*/
bool no_batch_wrap;
 
+   /**
+* Debug-only: expected maximum number bytes added to 
+* batch buffer
+*
+* 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;
+
+   /**
+* Debug-only: current number of bytes added to batch buffer
+*
+* 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 +1479,62 @@ is_power_of_two(uint32_t value)
return (value  (value - 1)) == 0;
 }
 
+/**
+ * Affect only in debug: Begin mark of don't flush batch buffer
+ *
+ * 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 context pointer
+ * \ref 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)
+{
+#ifdef DEBUG
+   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;
+#else
+   (void)brw;
+   (void)pmax_expected_batch_size;
+#endif
+}
+
+/**
+ * Affect only in Debug only: end mark of don't flush batch buffer
+ *
+ * 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)
+{
+#ifdef DEBUG
+   assert(brw-no_batch_wrap);
+   brw-no_batch_wrap=false;
+#else
+   (void)brw;
+#endif
+}
+
+
 /*==
  * 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..60d3c2d 100644
--- a/src/mesa/drivers/dri/i965/brw_state_batch.c
+++ b/src/mesa/drivers/dri/i965/brw_state_batch.c
@@ -127,6 +127,21 @@ 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 = 
+

Re: [Mesa-dev] [PATCH] nicer-no-wrap-patch

2013-11-11 Thread Rogovin, Kevin 
Hello,
  A subsequent patch has been sent out, and to put my head firmly into a paper 
bag, what should have been a two dry runs were not dry runs and got sent out;  
worse, it was sent three times... Sighs. My sincere apologies. Command lines 
are dangerous. As a side note, I do not know how the 2nd dry was recorded as 
sent after the correct run.

 If people want, I can send it out again with a different subject, but the 
details of the message are:

 http://lists.freedesktop.org/archives/mesa-dev/2013-November/048257.html

[Mesa-dev] [PATCH] nicer-no-wrap-patch
Kevin Rogovin kevin.rogovin at intel.com 
Mon Nov 11 23:26:47 PST 2013

with commit message:

Track bytes written during no flush phases for debug builds 


-Kevin


From: Rogovin, Kevin
Sent: Tuesday, November 12, 2013 8:39 AM
To: mesa-dev@lists.freedesktop.org
Subject: RE: [Mesa-dev] [PATCH] nicer-no-wrap-patch

Hi all, I will later submit a patch taking into account comments, however one 
comment I will address *now*.

Eric Anholt [e...@anholt.net] writes:

Kevin Rogovin kevin.rogo...@intel.com writes:

 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.

I don't like adding overhead to every batch operation.  You can just do
an assert like I did in 185b5a54c94ce11487146042c8eec24909187ed6

That approach used in brw_blorp_exec.cpp will not work here because the estimate
is (and should be) computed in brw_try_draw_prims() and the assert needs to be 
done
whenever commands or state are added to the batch buffer. Additionally it is 
literally an
overhead or exactly writing one boolean and two integers _per_ draw call. This 
overhead
is literally insignificant next to the overhead of the call stack to reach 
brw_try_draw_primis().

However, I will make it so that the write to those variables only occurs in 
debug, since the
assert is only for the purpose of debug; going further those members will only 
exists in debug
for that matter.

From: Eric Anholt [e...@anholt.net]
Sent: Monday, November 11, 2013 9:56 PM
To: Rogovin, Kevin; mesa-dev@lists.freedesktop.org
Cc: Rogovin, Kevin
Subject: Re: [Mesa-dev] [PATCH] nicer-no-wrap-patch

Kevin Rogovin kevin.rogo...@intel.com writes:

 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.

I don't like adding overhead to every batch operation.  You can just do
an assert like I did in 185b5a54c94ce11487146042c8eec24909187ed6
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev