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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
