Re: [PATCH 5/9] [v5] drm/i915/bdw: Reorganize PT allocations

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, Feb 24, 2014 at 07:03:12PM +0200, Imre Deak wrote:
> On Thu, 2014-02-20 at 11:51 -0800, Ben Widawsky wrote:
> > The previous allocation mechanism would get 2 contiguous allocations,
> > one for the page directories, and one for the page tables. As each page
> > table is 1 page, and there are 512 of these per page directory, this
> > goes to 2MB. An unfriendly request at best. Worse still, our HW now
> > supports 4 page directories, and a 2MB allocation is not allowed.
> > 
> > In order to fix this, this patch attempts to split up each page table
> > allocation into a single, discrete allocation. There is nothing really
> > fancy about the patch itself, it just has to manage an extra pointer
> > indirection, and have a fancier bit of logic to free up the pages.
> > 
> > To accommodate some of the added complexity, two new helpers are
> > introduced to allocate, and free the page table pages.
> > 
> > NOTE: I really wanted to split the way we do allocations, and the way in
> > which we identify the page table/page directory being used. I found
> > splitting this functionality up to be too unwieldy. I apologize in
> > advance to the reviewer. I'd recommend looking at the result, rather
> > than the diff.
> > 
> > v2/NOTE2: This patch predated commit:
> > 6f1cc993518462ccf039e195fabd47e7aa5bfd13
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Tue Dec 31 15:50:31 2013 +0000
> > 
> >     drm/i915: Avoid dereference past end of page arr
> > 
> > It fixed the same issue as that patch, but because of the limbo state of
> > PPGTT, Chris patch was merged instead. The excess churn is a result of
> > my using my original patch, which has my preferred naming. Primarily
> > act_* is changed to which_*, but it's mostly the same otherwise. I've
> > kept the convention Chris used for the pte wrap (I had something
> > slightly different, and broken - but fixable)
> > 
> > v3: Rename which_p[..]e to drop which_ (Chris)
> > Remove BUG_ON in inner loop (Chris)
> > Redo the pde/pdpe wrap logic (Chris)
> > 
> > v4: s/1MB/2MB in commit message (Imre)
> > Plug leaking gen8_pt_pages in both the error path, as well as general
> > free case (Imre)
> > 
> > v5: Rename leftover "which_" variables (Imre)
> > Add the pde = 0 wrap that was missed from v3 (Imre)
> > 
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 

Thanks very much for your thorough review of the series.


-- 
Ben Widawsky, Intel Open Source Technology Center