[Mesa-dev] [PATCH 4/5] Tidy a bit compute_memory_finalize_pending

[Bruno Jimenez]

On Sat, 2014-01-04 at 08:52 -0800, Tom Stellard wrote:
> On Sat, Jan 04, 2014 at 01:27:32AM +0100, Bruno Jiménez wrote:
> > ---
> >  src/gallium/drivers/r600/compute_memory_pool.c | 18 +++++++-----------
> >  1 file changed, 7 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/gallium/drivers/r600/compute_memory_pool.c b/src/gallium/drivers/r600/compute_memory_pool.c
> > index 5374a48..954c890 100644
> > --- a/src/gallium/drivers/r600/compute_memory_pool.c
> > +++ b/src/gallium/drivers/r600/compute_memory_pool.c
> > @@ -320,21 +320,17 @@ int compute_memory_finalize_pending(struct compute_memory_pool* pool,
> >  			int64_t need = item->size_in_dw+2048 -
> >  						(pool->size_in_dw - allocated);
> >  
> > -			need += 1024 - (need % 1024);
> >  
> > -			if (need > 0) {
> > -				err = compute_memory_grow_pool(pool,
> > -						pipe,
> > -						pool->size_in_dw + need);
> > -			}
> > -			else {
> > +			if (need <= 0) {
> 
> It looks like we are changing the behavior of the code here,
> because we are no longer doing need += 1024 - (need % 1024) before
> checking the value of need.
> 
> This code really does need to be cleaned up, but I want to make sure we
> aren't changing its behavior, or if we are we have a test case to show
> we aren't breaking things.
> 
> If you don't think we are changing behavior here, can you explain why
> not?
> 
> Thanks,
> Tom

You are completely wright, it changes the behaviour for need == 0 when
calculated. Still, I think it can be addressed just by checking need <
< 0.

I'll explain myself:

Suppose that need is 0 or positive, with
	need += 1024 - (need % 1024)
we do something like ceil to the nearest multiple of
1024, thus 0 will be 1024, 512 will also be 1024,
1025 will be 2048 and so on. So now need is always
positive, and we do compute_memory_grow_pool, check
its output and continue.

If need was negative it after
	need += 1024 - (need % 1024)
it will still be negative OR 0, so now we take the
else and recalculate need as
	need = pool->size_in_dw/10
	need += 1024 - (need % 1024)
then we do compute_memory_grow, check its output
and continue.

What I changed is that we just check need for being
positive or negative or 0, in the second case, need
is just pool->size_in_dw/10, after that, we calculate
	need += 1024 - (need % 1024)
and do compute_memory_grow, check its output and
continue.

So, you were wright, I changed the behaviour for
need == 0. But just checking for need < 0 should
address it.

If this is OK, I'll prepare a version 2 of the patches
later for submission.

Thanks!
Bruno
> >  				need = pool->size_in_dw / 10;
> > -				need += 1024 - (need % 1024);
> > -				err = compute_memory_grow_pool(pool,
> > -						pipe,
> > -						pool->size_in_dw + need);
> >  			}
> >  
> > +			need += 1024 - (need % 1024);
> > +
> > +			err = compute_memory_grow_pool(pool,
> > +					pipe,
> > +					pool->size_in_dw + need);
> > +
> >  			if(err == -1)
> >  				return -1;
> >  		}
> > -- 
> > 1.8.5.2
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev