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

[Ville Syrjälä]

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.

mappable is not a factor here. The global gtt is 2GiB and we just used
the same size for the ppgtt, which made sense for aliasing ppgtt I
suppose.

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

This means using an extra 4+MiB of kernel memroy per address space. But
I guess the dynamic page table stuff is coming along so it'll get sorted
out eventually.

But this won't actually prevent the GPU from faulting for >=2GiB
addresses since we leave the extra PTEs zeroed (ie. valid=0). We'd
need to extend the initial .clear_range() to make sure all the new
PTEs point to the scratch page. If we go to the trouble of allocating
the page tables I think we might as well set them up fully.

Previously when we just left the PDPs zeroed the GPU might or might not
fault depending on what kind of data was in the page at bus address 0.
I've occasionally wondered why the hardware designers didn't use the the
normal PTE/PDE encoding for the PDP registers so that you could have a
valid bit already at the top level.

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

-- 
Ville Syrjälä
Intel OTC