Re: [Mesa-dev] [PATCH 1/6] i965: Don't grow batch/state buffer on every emit after an overflow.

[Jordan Justen]

On 2017-11-28 18:41:44, Kenneth Graunke wrote:
> On Tuesday, November 28, 2017 6:15:59 PM PST Ian Romanick wrote:
> > On 11/28/2017 04:13 PM, Kenneth Graunke wrote:
> > > Once we reach the intended size of the buffer (BATCH_SZ or STATE_SZ), we
> > > try and flush.  If we're not allowed to flush, we resort to growing the
> > > buffer so that there's space for the data we need to emit.
> > > 
> > > We accidentally got the threshold wrong.  The first non-wrappable call
> > > beyond (e.g.) STATE_SZ would grow the buffer to floor(1.5 * STATE_SZ),
> > > The next call would see we were beyond STATE_SZ and think we needed to
> > > grow a second time - when the buffer was already large enough.
> > > 
> > > We still want to flush when we hit STATE_SZ, but for growing, we should
> > > use the actual size of the buffer as the threshold.  This way, we only
> > > grow when actually necessary.
> > > 
> > > Fixes: 2dfc119f22f257082ab0 "i965: Grow the batch/state buffers if we 
> > > need space and can't flush."
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103101
> > > ---
> > >  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
> > > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > > index 216073129ba..1d0292b4b80 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > > @@ -373,7 +373,7 @@ intel_batchbuffer_require_space(struct brw_context 
> > > *brw, GLuint sz,
> > > if (batch_used + sz >= BATCH_SZ) {
> > 
> > Why not just change this test to >= batch->bo->size?
> 
> We want to flush when we reach BATCH_SZ (the intended flush point).
> If we reach that point but are in the middle of something which makes
> makes it unsafe to flush (no_wrap), then we keep going, growing the BO
> as necessary.  When growing, we only want to grow if we exceed the
> actual buffer size.
> 
> I'm not a huge fan of how this is structured, so if you have an idea
> for how to make it more obvious, I'm open to suggestions.

I also found this a bit confusing while trying to investigate the DiRT
Rally hang.

How about this?

if (batch_used + sz >= BATCH_SZ && !batch->no_wrap) {
   intel_batchbuffer_flush(brw);
} else if (batch_used + sz >= batch->bo->size) {
   ...
}

I don't think it is a huge deal, and either way:

Reviewed-by: Jordan Justen 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/6] i965: Don't grow batch/state buffer on every emit after an overflow.

[Kenneth Graunke]

On Tuesday, November 28, 2017 6:15:59 PM PST Ian Romanick wrote:
> On 11/28/2017 04:13 PM, Kenneth Graunke wrote:
> > Once we reach the intended size of the buffer (BATCH_SZ or STATE_SZ), we
> > try and flush.  If we're not allowed to flush, we resort to growing the
> > buffer so that there's space for the data we need to emit.
> > 
> > We accidentally got the threshold wrong.  The first non-wrappable call
> > beyond (e.g.) STATE_SZ would grow the buffer to floor(1.5 * STATE_SZ),
> > The next call would see we were beyond STATE_SZ and think we needed to
> > grow a second time - when the buffer was already large enough.
> > 
> > We still want to flush when we hit STATE_SZ, but for growing, we should
> > use the actual size of the buffer as the threshold.  This way, we only
> > grow when actually necessary.
> > 
> > Fixes: 2dfc119f22f257082ab0 "i965: Grow the batch/state buffers if we need 
> > space and can't flush."
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103101
> > ---
> >  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > index 216073129ba..1d0292b4b80 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -373,7 +373,7 @@ intel_batchbuffer_require_space(struct brw_context 
> > *brw, GLuint sz,
> > if (batch_used + sz >= BATCH_SZ) {
> 
> Why not just change this test to >= batch->bo->size?

We want to flush when we reach BATCH_SZ (the intended flush point).
If we reach that point but are in the middle of something which makes
makes it unsafe to flush (no_wrap), then we keep going, growing the BO
as necessary.  When growing, we only want to grow if we exceed the
actual buffer size.

I'm not a huge fan of how this is structured, so if you have an idea
for how to make it more obvious, I'm open to suggestions.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/6] i965: Don't grow batch/state buffer on every emit after an overflow.

[Ian Romanick]

On 11/28/2017 04:13 PM, Kenneth Graunke wrote:
> Once we reach the intended size of the buffer (BATCH_SZ or STATE_SZ), we
> try and flush.  If we're not allowed to flush, we resort to growing the
> buffer so that there's space for the data we need to emit.
> 
> We accidentally got the threshold wrong.  The first non-wrappable call
> beyond (e.g.) STATE_SZ would grow the buffer to floor(1.5 * STATE_SZ),
> The next call would see we were beyond STATE_SZ and think we needed to
> grow a second time - when the buffer was already large enough.
> 
> We still want to flush when we hit STATE_SZ, but for growing, we should
> use the actual size of the buffer as the threshold.  This way, we only
> grow when actually necessary.
> 
> Fixes: 2dfc119f22f257082ab0 "i965: Grow the batch/state buffers if we need 
> space and can't flush."
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103101
> ---
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 216073129ba..1d0292b4b80 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -373,7 +373,7 @@ intel_batchbuffer_require_space(struct brw_context *brw, 
> GLuint sz,
> if (batch_used + sz >= BATCH_SZ) {

Why not just change this test to >= batch->bo->size?

>if (!batch->no_wrap) {
>   intel_batchbuffer_flush(brw);
> -  } else {
> +  } else if (batch_used + sz >= batch->bo->size) {
>   const unsigned new_size =
>  MIN2(batch->bo->size + batch->bo->size / 2, MAX_BATCH_SIZE);
>   grow_buffer(brw, &batch->bo, &batch->map, &batch->batch_cpu_map,
> @@ -1075,7 +1075,7 @@ brw_state_batch(struct brw_context *brw,
>if (!batch->no_wrap) {
>   intel_batchbuffer_flush(brw);
>   offset = ALIGN(batch->state_used, alignment);
> -  } else {
> +  } else if (offset + size >= batch->state_bo->size) {
>   const unsigned new_size =
>  MIN2(batch->state_bo->size + batch->state_bo->size / 2,
>   MAX_STATE_SIZE);
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/6] i965: Don't grow batch/state buffer on every emit after an overflow.

[Kenneth Graunke]

Once we reach the intended size of the buffer (BATCH_SZ or STATE_SZ), we
try and flush.  If we're not allowed to flush, we resort to growing the
buffer so that there's space for the data we need to emit.

We accidentally got the threshold wrong.  The first non-wrappable call
beyond (e.g.) STATE_SZ would grow the buffer to floor(1.5 * STATE_SZ),
The next call would see we were beyond STATE_SZ and think we needed to
grow a second time - when the buffer was already large enough.

We still want to flush when we hit STATE_SZ, but for growing, we should
use the actual size of the buffer as the threshold.  This way, we only
grow when actually necessary.

Fixes: 2dfc119f22f257082ab0 "i965: Grow the batch/state buffers if we need 
space and can't flush."
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103101
---
 src/mesa/drivers/dri/i965/intel_batchbuffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index 216073129ba..1d0292b4b80 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -373,7 +373,7 @@ intel_batchbuffer_require_space(struct brw_context *brw, 
GLuint sz,
if (batch_used + sz >= BATCH_SZ) {
   if (!batch->no_wrap) {
  intel_batchbuffer_flush(brw);
-  } else {
+  } else if (batch_used + sz >= batch->bo->size) {
  const unsigned new_size =
 MIN2(batch->bo->size + batch->bo->size / 2, MAX_BATCH_SIZE);
  grow_buffer(brw, &batch->bo, &batch->map, &batch->batch_cpu_map,
@@ -1075,7 +1075,7 @@ brw_state_batch(struct brw_context *brw,
   if (!batch->no_wrap) {
  intel_batchbuffer_flush(brw);
  offset = ALIGN(batch->state_used, alignment);
-  } else {
+  } else if (offset + size >= batch->state_bo->size) {
  const unsigned new_size =
 MIN2(batch->state_bo->size + batch->state_bo->size / 2,
  MAX_STATE_SIZE);
-- 
2.15.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev