Hi Guys:
Please don't merge this patch at this time as I found another problem about alias PPGTT.
It seems the alias PPGTT on GEN8+ is also broken under native i915. The first allocate_va_range() in alias PPGTT initialization path will allocate all the page tables and the next clear_range() will shrink the allocated page table. It's OK at this time, but when doing VMA binding, alias_ppgtt_bind_vma() will directly insert PTE entries without calling a allocate_va_range() to create the page table first. This causes an OOPS on my machine.
It looks like we have two options:
1. Let the alias PPGTT page table also be shrinkable.
2. Follow the previous approach, don't shrink the alias PPGTT page table(Probably add some hack in clear_range() path.
-----Original Message-----
From: Winiarski, Michal
Sent: Thursday, January 19, 2017 7:53 PM
To: Wang, Zhi A <zhi.a.wang@xxxxxxxxx>
Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Thierry, Michel <michel.thierry@xxxxxxxxx>; Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>; Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>; Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>; Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>
Subject: Re: [PATCH] drm/i915: Re-enable preallocated top level PDPs support
On Tue, Jan 17, 2017 at 10:06:12PM +0800, Zhi Wang wrote:
> After PPGTT page table is able to be shrinken, the preallocated PDPs
> and PDE pages can be freed even they are preallocated under 3-level
> PPGTT mode. This patch re-enables preallocated top level PDPs and PDE
> pages like before.
>
> Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
> Cc: Zhiyuan Lv <zhiyuan.lv@xxxxxxxxx>
> Signed-off-by: Zhi Wang <zhi.a.wang@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 10 +++++++++-
> drivers/gpu/drm/i915/i915_gem_gtt.h | 1 +
> drivers/gpu/drm/i915/intel_lrc.c | 2 +-
> 3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8aca11f..f0e1992 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -793,12 +793,18 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
> struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> struct i915_page_directory *pd;
> uint64_t pdpe;
> + bool pd_is_empty;
>
> gen8_for_each_pdpe(pd, pdp, start, length, pdpe) {
> if (WARN_ON(!pdp->page_directory[pdpe]))
> break;
>
> - if (gen8_ppgtt_clear_pd(vm, pd, start, length)) {
> + pd_is_empty = gen8_ppgtt_clear_pd(vm, pd, start, length);
Why the extra pd_is_empty variable?
Just adding if (preallocate) continue; here is more readable imho.
We should also assert that we're not in 4-level paging mode when shrinking is skipped.
With those changes:
Reviewed-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
-Michał
> + /* Do not free pd pages if pdps are preallocated. */
> + if (ppgtt->preallocate_top_level_pdps)
> + continue;
> +
> + if (pd_is_empty) {
> __clear_bit(pdpe, pdp->used_pdpes);
> gen8_setup_pdpe(ppgtt, pdp, vm->scratch_pd, pdpe);
> free_pd(vm->i915, pd);
> @@ -1545,6 +1551,8 @@ static int
> gen8_preallocate_top_level_pdps(struct i915_hw_ppgtt *ppgtt)
>
> free_gen8_temp_bitmaps(new_page_dirs, new_page_tables);
>
> + ppgtt->preallocate_top_level_pdps = true;
> +
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
> b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 34a4fd5..f325cb8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -349,6 +349,7 @@ struct i915_hw_ppgtt {
> struct kref ref;
> struct drm_mm_node node;
> unsigned long pd_dirty_rings;
> + bool preallocate_top_level_pdps;
> union {
> struct i915_pml4 pml4; /* GEN8+ & 48b PPGTT */
> struct i915_page_directory_pointer pdp; /* GEN8+ */
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index db714dc..766a91a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1488,7 +1488,7 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
> if (req->ctx->ppgtt &&
> (intel_engine_flag(req->engine) & req->ctx->ppgtt->pd_dirty_rings)) {
> if (!USES_FULL_48BIT_PPGTT(req->i915) &&
> - !intel_vgpu_active(req->i915)) {
> + !req->ctx->ppgtt->preallocate_top_level_pdps) {
> ret = intel_logical_ring_emit_pdps(req);
> if (ret)
> return ret;
> --
> 1.9.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
