[Intel-gfx] [PATCH] drm/i915: Setup all page directories for gen8

[Daniel Vetter]

On Wed, Mar 04, 2015 at 11:58:41AM -0800, Ben Widawsky wrote:
> On Tue, Mar 03, 2015 at 05:03:29PM +0200, Mika Kuoppala wrote:
> > If the mappable size is less than what the full range
> > of pdps can address, we end up setting pdps for only the
> > mappable area.
> > 
> > The logical context however needs valid pdp entries.
> > Prior to commit 06fda602dbca ("drm/i915: Create page table allocators")
> > we just have been writing pdp entries with dma address of zero instead
> > of valid pdps. This is supposedly bad even if those pdps are not
> > addressed.
> > 
> > As commit 06fda602dbca ("drm/i915: Create page table allocators")
> > introduced more dynamic structure for pdps, we ended up oopsing
> > when we populated the lrc context. Analyzing this oops revealed
> > the fact that we have not been writing valid pdps with bsw, as
> > it is doing the ppgtt init with 2gb limit.
> > 
> > We should do the right thing and setup the non addressable part
> > pdps/pde/pte to scratch page through the minimal structure by
> > having just pdp with pde entries pointing to same page with
> > pte entries pointing to scratch page.
> > 
> > But instead of going through that trouble, setup all the pdps
> > through individual pd pages and pt entries, even for non
> > addressable parts. This way we populate the lrc with valid
> > pdps and gives us a base for dynamic page allocation to
> > introduce code that truncates the page table structure.
> > 
> > The regression of oopsing in init was introduced by
> > commit 06fda602dbca ("drm/i915: Create page table allocators")
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89350
> > Tested-by: Valtteri Rantala <valtteri.rantala at intel.com>
> > Cc: Michel Thierry <michel.thierry at intel.com>
> > Cc: Ben Widawsky <benjamin.widawsky at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index bd95776..848a821 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -709,7 +709,7 @@ static int gen8_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt,
> >   */
> >  static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
> >  {
> > -	const int max_pdp = DIV_ROUND_UP(size, 1 << 30);
> > +	const int max_pdp = GEN8_LEGACY_PDPES;
> >  	const int min_pt_pages = GEN8_PDES_PER_PAGE * max_pdp;
> >  	int i, j, ret;
> >  
> 
> FWIW, I think I solved this later in my original series:
> http://lists.freedesktop.org/archives/intel-gfx/2014-August/051162.html

Yeah that looks a lot saner I agree. But since dynamic pt alloc
essentially amounts to the same (we'll start out with all zero entries in
the pds) the indirection with the additional scratch_pd doesn't help I
think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch