[Intel-gfx] [PATCH 5/9] drm/i915/bdw: Reorganize PT allocations

[Ben Widawsky]

On Wed, Feb 12, 2014 at 11:45:59PM +0000, Chris Wilson wrote:
> On Wed, Feb 12, 2014 at 02:28:48PM -0800, Ben Widawsky wrote:
> > -		for (i = first_pte; i < last_pte; i++)
> > +		for (i = which_pte; i < last_pte; i++) {
> >  			pt_vaddr[i] = scratch_pte;
> > +			num_entries--;
> > +			BUG_ON(num_entries < 0);
> > +		}
> >  
> >  		kunmap_atomic(pt_vaddr);
> >  
> > -		num_entries -= last_pte - first_pte;
> 
> I'm going to moan about this being replaced by a BUG_ON inside the inner
> loop.
> 

I'm fine with removing it. I guess doing any sort of perf with BUG_ON is
ill advised, but running without BUG_ON is perhaps equally ill advised.

> > -		first_pte = 0;
> > -		act_pt++;
> > +		which_pte = 0;
> 
> > +		if (which_pde + 1 == GEN8_PDES_PER_PAGE)
> > +			which_pdpe++;
> > +		which_pde = (which_pde + 1) & GEN8_PDE_MASK;
> 
> I think this would be clearer written as
>   if (++which_pde == GEN8_PDES_PER_PAGE) {
>      which_pdpe++;
>      which_pde = 0;
>    }

I'm fine with that change.

> as then the relationship between pdpe and pde is much more apparent to
> me. Do we feel that which_pte, which_pde, which_pdpe are really any
> better than pte, pde, pdpe? Or is it important to question ourselves
> every step of the way?

I actually just don't like act_, and first_, dropping the "which_" is
perfectly acceptable to me.

> 
> And I may as well moan about having to preallocate everything. ;-)
> -Chris
> 

Deferring allocation is an important but separate step.

> -- 
> Chris Wilson, Intel Open Source Technology Centre

I'll give Imre a bit to leave comments and then respin.

-- 
Ben Widawsky, Intel Open Source Technology Center