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

[Tom Stellard]

On Sat, Jan 04, 2014 at 06:28:27PM +0100, Bruno Jimenez wrote:
> 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.
>

Thanks for the explanation.  I understand what is happening now.
This change should be OK, but I'd like to do at least one piglit run
just to verify.  Do you have the HW to test this?

-Tom

> 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
>