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

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

[Mika Kuoppala]

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@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Setup all page directories for gen8

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5878
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -9              278/278              269/278
ILK                                  308/308              308/308
SNB                                  284/284              284/284
IVB                                  380/380              380/380
BYT                                  294/294              294/294
HSW                                  387/387              387/387
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_fence_thrash_bo-write-verify-none      NRUN(1)PASS(6)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-x      PASS(7)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-y      NO_RESULT(1)PASS(6)      FAIL(1)PASS(1)
 PNV  igt_gem_userptr_blits_coherency-sync      NO_RESULT(1)CRASH(7)NRUN(1)PASS(8)      CRASH(2)
 PNV  igt_gem_userptr_blits_coherency-unsync      NO_RESULT(1)CRASH(6)PASS(7)      CRASH(2)
*PNV  igt_gem_userptr_blits_forked-unsync-swapping-multifd-mempressure-interruptible      PASS(2)      NRUN(1)PASS(1)
*PNV  igt_gem_userptr_blits_input-checking      PASS(2)      NRUN(1)PASS(1)
 PNV  igt_gem_userptr_blits_minor-unsync-interruptible      DMESG_WARN(1)PASS(5)      DMESG_WARN(1)PASS(1)
 PNV  igt_gem_fence_thrash_bo-write-verify-threaded-none      FAIL(2)CRASH(4)PASS(6)      CRASH(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(19)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mika Kuoppala]

If the requested size is less than what the full range
of pdps can address, we end up setting pdps for only the
requested area.

The logical context however needs all pdp entries to be valid.
Prior to commit 06fda602dbca ("drm/i915: Create page table allocators")
we 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 in some cases.

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. And let the clear range point them to scratch
page. This way we populate the lrc with valid pdps and wait
for dynamic page allocation work to land, and do the heavy lifting
for truncating page table tree according to usage.

The regression of oopsing in init was introduced by
commit 06fda602dbca ("drm/i915: Create page table allocators")

v2: Clear the range for the unused part also (Ville)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89350
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Valtteri Rantala <valtteri.rantala@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index bd95776..1aa2a2c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -716,15 +716,19 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 	if (size % (1<<30))
 		DRM_INFO("Pages will be wasted unless GTT size (%llu) is divisible by 1GB\n", size);
 
-	/* 1. Do all our allocations for page directories and page tables. */
-	ret = gen8_ppgtt_alloc(ppgtt, max_pdp);
+	/* 1. Do all our allocations for page directories and page tables.
+	 * We allocate more than was asked so that we can point the unused parts
+	 * to valid entries that point to scratch page. Dynamic page tables
+	 * will fix this eventually.
+	 */
+	ret = gen8_ppgtt_alloc(ppgtt, GEN8_LEGACY_PDPES);
 	if (ret)
 		return ret;
 
 	/*
 	 * 2. Create DMA mappings for the page directories and page tables.
 	 */
-	for (i = 0; i < max_pdp; i++) {
+	for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
 		ret = gen8_ppgtt_setup_page_directories(ppgtt, i);
 		if (ret)
 			goto bail;
@@ -744,7 +748,7 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 	 * plugged in correctly. So we do that now/here. For aliasing PPGTT, we
 	 * will never need to touch the PDEs again.
 	 */
-	for (i = 0; i < max_pdp; i++) {
+	for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
 		struct i915_page_directory_entry *pd = ppgtt->pdp.page_directory[i];
 		gen8_ppgtt_pde_t *pd_vaddr;
 		pd_vaddr = kmap_atomic(ppgtt->pdp.page_directory[i]->page);
@@ -764,9 +768,14 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
 	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
 	ppgtt->base.start = 0;
-	ppgtt->base.total = ppgtt->num_pd_entries * GEN8_PTES_PER_PAGE * PAGE_SIZE;
 
-	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
+	/* This is the area that we advertise as usable for the caller */
+	ppgtt->base.total = max_pdp * GEN8_PDES_PER_PAGE * GEN8_PTES_PER_PAGE * PAGE_SIZE;
+
+	/* Set all ptes to a valid scratch page. Also above requested space */
+	ppgtt->base.clear_range(&ppgtt->base, 0,
+				ppgtt->num_pd_pages * GEN8_PTES_PER_PAGE * PAGE_SIZE,
+				true);
 
 	DRM_DEBUG_DRIVER("Allocated %d pages for page directories (%d wasted)\n",
 			 ppgtt->num_pd_pages, ppgtt->num_pd_pages - max_pdp);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Setup all page directories for gen8

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5884
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -6              278/278              272/278
ILK                 -1              308/308              307/308
SNB                 -1              284/284              283/284
IVB                                  380/380              380/380
BYT                                  294/294              294/294
HSW                                  387/387              387/387
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_fence_thrash_bo-write-verify-none      NRUN(1)PASS(7)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-x      PASS(8)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-y      NO_RESULT(1)PASS(7)      FAIL(1)PASS(1)
*PNV  igt_gem_userptr_blits_minor-normal-sync      PASS(5)      DMESG_WARN(1)PASS(1)
 PNV  igt_gen3_render_mixed_blits      FAIL(10)PASS(9)      FAIL(2)
 PNV  igt_gem_fence_thrash_bo-write-verify-threaded-none      FAIL(2)CRASH(5)PASS(7)      CRASH(2)
 ILK  igt_gem_unfence_active_buffers      DMESG_WARN(1)PASS(3)      DMESG_WARN(1)PASS(1)
*SNB  igt_gem_exec_params_sol-reset-not-gen7      PASS(2)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(20)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Setup all page directories for gen8

[Ville Syrjälä]

On Wed, Mar 04, 2015 at 02:55:17PM +0200, Mika Kuoppala wrote:
> If the requested size is less than what the full range
> of pdps can address, we end up setting pdps for only the
> requested area.
> 
> The logical context however needs all pdp entries to be valid.
> Prior to commit 06fda602dbca ("drm/i915: Create page table allocators")
> we 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 in some cases.
> 
> 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. And let the clear range point them to scratch
> page. This way we populate the lrc with valid pdps and wait
> for dynamic page allocation work to land, and do the heavy lifting
> for truncating page table tree according to usage.
> 
> The regression of oopsing in init was introduced by
> commit 06fda602dbca ("drm/i915: Create page table allocators")
> 
> v2: Clear the range for the unused part also (Ville)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89350
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Tested-by: Valtteri Rantala <valtteri.rantala@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Seems sane enough assuming we're willing to pay the cost.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

And then we could also throw another patch on top to actually increase the
PPGTT size to 4GiB since there's no extra cost anymore :) Well, assuming
everything else is 4GiB safe. But I guess it ought to be if BDW machines
have already been running with 4GiB GGTT/PPGTT.

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index bd95776..1aa2a2c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -716,15 +716,19 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	if (size % (1<<30))
>  		DRM_INFO("Pages will be wasted unless GTT size (%llu) is divisible by 1GB\n", size);
>  
> -	/* 1. Do all our allocations for page directories and page tables. */
> -	ret = gen8_ppgtt_alloc(ppgtt, max_pdp);
> +	/* 1. Do all our allocations for page directories and page tables.
> +	 * We allocate more than was asked so that we can point the unused parts
> +	 * to valid entries that point to scratch page. Dynamic page tables
> +	 * will fix this eventually.
> +	 */
> +	ret = gen8_ppgtt_alloc(ppgtt, GEN8_LEGACY_PDPES);
>  	if (ret)
>  		return ret;
>  
>  	/*
>  	 * 2. Create DMA mappings for the page directories and page tables.
>  	 */
> -	for (i = 0; i < max_pdp; i++) {
> +	for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
>  		ret = gen8_ppgtt_setup_page_directories(ppgtt, i);
>  		if (ret)
>  			goto bail;
> @@ -744,7 +748,7 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	 * plugged in correctly. So we do that now/here. For aliasing PPGTT, we
>  	 * will never need to touch the PDEs again.
>  	 */
> -	for (i = 0; i < max_pdp; i++) {
> +	for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
>  		struct i915_page_directory_entry *pd = ppgtt->pdp.page_directory[i];
>  		gen8_ppgtt_pde_t *pd_vaddr;
>  		pd_vaddr = kmap_atomic(ppgtt->pdp.page_directory[i]->page);
> @@ -764,9 +768,14 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
>  	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
>  	ppgtt->base.start = 0;
> -	ppgtt->base.total = ppgtt->num_pd_entries * GEN8_PTES_PER_PAGE * PAGE_SIZE;
>  
> -	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
> +	/* This is the area that we advertise as usable for the caller */
> +	ppgtt->base.total = max_pdp * GEN8_PDES_PER_PAGE * GEN8_PTES_PER_PAGE * PAGE_SIZE;
> +
> +	/* Set all ptes to a valid scratch page. Also above requested space */
> +	ppgtt->base.clear_range(&ppgtt->base, 0,
> +				ppgtt->num_pd_pages * GEN8_PTES_PER_PAGE * PAGE_SIZE,
> +				true);
>  
>  	DRM_DEBUG_DRIVER("Allocated %d pages for page directories (%d wasted)\n",
>  			 ppgtt->num_pd_pages, ppgtt->num_pd_pages - max_pdp);
> -- 
> 1.9.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Setup all page directories for gen8

[Ben Widawsky]

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@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@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


-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Setup all page directories for gen8

[Daniel Vetter]

On Wed, Mar 04, 2015 at 09:14:39PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 04, 2015 at 02:55:17PM +0200, Mika Kuoppala wrote:
> > If the requested size is less than what the full range
> > of pdps can address, we end up setting pdps for only the
> > requested area.
> > 
> > The logical context however needs all pdp entries to be valid.
> > Prior to commit 06fda602dbca ("drm/i915: Create page table allocators")
> > we 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 in some cases.
> > 
> > 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. And let the clear range point them to scratch
> > page. This way we populate the lrc with valid pdps and wait
> > for dynamic page allocation work to land, and do the heavy lifting
> > for truncating page table tree according to usage.
> > 
> > The regression of oopsing in init was introduced by
> > commit 06fda602dbca ("drm/i915: Create page table allocators")
> > 
> > v2: Clear the range for the unused part also (Ville)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89350
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Tested-by: Valtteri Rantala <valtteri.rantala@intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> Seems sane enough assuming we're willing to pay the cost.
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> And then we could also throw another patch on top to actually increase the
> PPGTT size to 4GiB since there's no extra cost anymore :) Well, assuming
> everything else is 4GiB safe. But I guess it ought to be if BDW machines
> have already been running with 4GiB GGTT/PPGTT.

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx