[PATCH 1/3] drm/i915: Move the release of PT page to the upper caller

[PATCH 1/3] drm/i915: Move the release of PT page to the upper caller

[Zhi Wang]

a PT page will be released if it doesn't contain any meaningful mappings
during PPGTT page table shrinking. The PT entry in the upper level will
be set to a scratch entry.

Normally this works nicely, but in virtualization world, the PPGTT page
table is tracked by hypervisor. Releasing the PT page before modifying
the upper level PT entry would cause extra efforts.

As the tracked page has been returned to OS before losing track from
hypervisor, it could be written in any pattern. Hypervisor has to recognize
if a page is still being used as a PT page by validating these writing
patterns. It's complicated. Better let the guest modify the PT entry in
upper level PT first, then release the PT page.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhiyuan Lv <zhiyuan.lv@intel.com> #v4.10+
Cc: stable@vger.kernel.org
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
Link: https://patchwork.freedesktop.org/patch/122697/msgid/1479728666-25333-1-git-send-email-zhi.a.wang@intel.com
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Link: http://patchwork.freedesktop.org/patch/msgid/1480402516-22275-1-git-send-email-zhi.a.wang@intel.com
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b4bde14..6cee707 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -736,10 +736,8 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 
 	bitmap_clear(pt->used_ptes, pte, num_entries);
 
-	if (bitmap_empty(pt->used_ptes, GEN8_PTES)) {
-		free_pt(to_i915(vm->dev), pt);
+	if (bitmap_empty(pt->used_ptes, GEN8_PTES))
 		return true;
-	}
 
 	pt_vaddr = kmap_px(pt);
 
@@ -775,13 +773,12 @@ static bool gen8_ppgtt_clear_pd(struct i915_address_space *vm,
 			pde_vaddr = kmap_px(pd);
 			pde_vaddr[pde] = scratch_pde;
 			kunmap_px(ppgtt, pde_vaddr);
+			free_pt(to_i915(vm->dev), pt);
 		}
 	}
 
-	if (bitmap_empty(pd->used_pdes, I915_PDES)) {
-		free_pd(to_i915(vm->dev), pd);
+	if (bitmap_empty(pd->used_pdes, I915_PDES))
 		return true;
-	}
 
 	return false;
 }
@@ -795,7 +792,6 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
 				 uint64_t length)
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
-	struct drm_i915_private *dev_priv = to_i915(vm->dev);
 	struct i915_page_directory *pd;
 	uint64_t pdpe;
 	gen8_ppgtt_pdpe_t *pdpe_vaddr;
@@ -813,16 +809,14 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
 				pdpe_vaddr[pdpe] = scratch_pdpe;
 				kunmap_px(ppgtt, pdpe_vaddr);
 			}
+			free_pd(to_i915(vm->dev), pd);
 		}
 	}
 
 	mark_tlbs_dirty(ppgtt);
 
-	if (USES_FULL_48BIT_PPGTT(dev_priv) &&
-	    bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv))) {
-		free_pdp(dev_priv, pdp);
+	if (bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv)))
 		return true;
-	}
 
 	return false;
 }
@@ -836,6 +830,7 @@ static void gen8_ppgtt_clear_pml4(struct i915_address_space *vm,
 				  uint64_t start,
 				  uint64_t length)
 {
+	struct drm_i915_private *dev_priv = to_i915(vm->dev);
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 	struct i915_page_directory_pointer *pdp;
 	uint64_t pml4e;
@@ -854,6 +849,7 @@ static void gen8_ppgtt_clear_pml4(struct i915_address_space *vm,
 			pml4e_vaddr = kmap_px(pml4);
 			pml4e_vaddr[pml4e] = scratch_pml4e;
 			kunmap_px(ppgtt, pml4e_vaddr);
+			free_pdp(dev_priv, pdp);
 		}
 	}
 }
-- 
1.9.1

[PATCH 2/3] drm/i915: Let execlist_update_context() cover !FULL_PPGTT mode.

[Zhi Wang]

execlist_update_context() will try to update PDPs in a context before a
ELSP submission only for full PPGTT mode, while PDPs was populated during
context initialization. Now the latter code path is removed. Let
execlist_update_context() also cover !FULL_PPGTT mode.

Fixes: 34869776c76b ("drm/i915: check ppgtt validity when init reg state")
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhiyuan Lv <zhiyuan.lv@intel.com> #v4.10+
Cc: stable@vger.kernel.org
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1486377436-15380-1-git-send-email-zhi.a.wang@intel.com
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index beabc17..2af4522 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -362,7 +362,8 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
 static u64 execlists_update_context(struct drm_i915_gem_request *rq)
 {
 	struct intel_context *ce = &rq->ctx->engine[rq->engine->id];
-	struct i915_hw_ppgtt *ppgtt = rq->ctx->ppgtt;
+	struct i915_hw_ppgtt *ppgtt =
+		rq->ctx->ppgtt ?: rq->i915->mm.aliasing_ppgtt;
 	u32 *reg_state = ce->lrc_reg_state;
 
 	reg_state[CTX_RING_TAIL+1] = rq->tail;
-- 
1.9.1

[PATCH 3/3] drm/i915: A hotfix for making aliasing PPGTT work for GVT-g

[Zhi Wang]

This patch makes PPGTT page table non-shrinkable when using aliasing PPGTT
mode. It's just a temporary solution for making GVT-g work.

Fixes: 2ce5179fe826 ("drm/i915/gtt: Free unused lower-level page tables")
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1486559013-25251-2-git-send-email-zhi.a.wang@intel.com
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v4.10+
Cc: stable@vger.kernel.org
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6cee707..6924a8e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -735,9 +735,10 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 	GEM_BUG_ON(pte_end > GEN8_PTES);
 
 	bitmap_clear(pt->used_ptes, pte, num_entries);
-
-	if (bitmap_empty(pt->used_ptes, GEN8_PTES))
-		return true;
+	if (USES_FULL_PPGTT(vm->i915)) {
+		if (bitmap_empty(pt->used_ptes, GEN8_PTES))
+			return true;
+	}
 
 	pt_vaddr = kmap_px(pt);
 
-- 
1.9.1

Re: [PATCH 1/3] drm/i915: Move the release of PT page to the upper caller

[Zhi Wang]

Hi Greg:
	Is there anything else I should do to let patches that has already been 
merged by linux-stable land in new 4.10.x tag? I have re-sent those 
patches with the #v4.10+ tag, but looks they still haven't been merged.

Thanks,
Zhi.

On 03/27/17 09:57, Zhi Wang wrote:
> a PT page will be released if it doesn't contain any meaningful mappings
> during PPGTT page table shrinking. The PT entry in the upper level will
> be set to a scratch entry.
>
> Normally this works nicely, but in virtualization world, the PPGTT page
> table is tracked by hypervisor. Releasing the PT page before modifying
> the upper level PT entry would cause extra efforts.
>
> As the tracked page has been returned to OS before losing track from
> hypervisor, it could be written in any pattern. Hypervisor has to recognize
> if a page is still being used as a PT page by validating these writing
> patterns. It's complicated. Better let the guest modify the PT entry in
> upper level PT first, then release the PT page.
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhiyuan Lv <zhiyuan.lv@intel.com> #v4.10+
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> Link: https://patchwork.freedesktop.org/patch/122697/msgid/1479728666-25333-1-git-send-email-zhi.a.wang@intel.com
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Link: http://patchwork.freedesktop.org/patch/msgid/1480402516-22275-1-git-send-email-zhi.a.wang@intel.com
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 18 +++++++-----------
>   1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index b4bde14..6cee707 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -736,10 +736,8 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>
>   	bitmap_clear(pt->used_ptes, pte, num_entries);
>
> -	if (bitmap_empty(pt->used_ptes, GEN8_PTES)) {
> -		free_pt(to_i915(vm->dev), pt);
> +	if (bitmap_empty(pt->used_ptes, GEN8_PTES))
>   		return true;
> -	}
>
>   	pt_vaddr = kmap_px(pt);
>
> @@ -775,13 +773,12 @@ static bool gen8_ppgtt_clear_pd(struct i915_address_space *vm,
>   			pde_vaddr = kmap_px(pd);
>   			pde_vaddr[pde] = scratch_pde;
>   			kunmap_px(ppgtt, pde_vaddr);
> +			free_pt(to_i915(vm->dev), pt);
>   		}
>   	}
>
> -	if (bitmap_empty(pd->used_pdes, I915_PDES)) {
> -		free_pd(to_i915(vm->dev), pd);
> +	if (bitmap_empty(pd->used_pdes, I915_PDES))
>   		return true;
> -	}
>
>   	return false;
>   }
> @@ -795,7 +792,6 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
>   				 uint64_t length)
>   {
>   	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> -	struct drm_i915_private *dev_priv = to_i915(vm->dev);
>   	struct i915_page_directory *pd;
>   	uint64_t pdpe;
>   	gen8_ppgtt_pdpe_t *pdpe_vaddr;
> @@ -813,16 +809,14 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
>   				pdpe_vaddr[pdpe] = scratch_pdpe;
>   				kunmap_px(ppgtt, pdpe_vaddr);
>   			}
> +			free_pd(to_i915(vm->dev), pd);
>   		}
>   	}
>
>   	mark_tlbs_dirty(ppgtt);
>
> -	if (USES_FULL_48BIT_PPGTT(dev_priv) &&
> -	    bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv))) {
> -		free_pdp(dev_priv, pdp);
> +	if (bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv)))
>   		return true;
> -	}
>
>   	return false;
>   }
> @@ -836,6 +830,7 @@ static void gen8_ppgtt_clear_pml4(struct i915_address_space *vm,
>   				  uint64_t start,
>   				  uint64_t length)
>   {
> +	struct drm_i915_private *dev_priv = to_i915(vm->dev);
>   	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
>   	struct i915_page_directory_pointer *pdp;
>   	uint64_t pml4e;
> @@ -854,6 +849,7 @@ static void gen8_ppgtt_clear_pml4(struct i915_address_space *vm,
>   			pml4e_vaddr = kmap_px(pml4);
>   			pml4e_vaddr[pml4e] = scratch_pml4e;
>   			kunmap_px(ppgtt, pml4e_vaddr);
> +			free_pdp(dev_priv, pdp);
>   		}
>   	}
>   }
>

Re: [PATCH 1/3] drm/i915: Move the release of PT page to the upper caller

[Greg KH]

On Sat, Apr 01, 2017 at 02:08:27PM +0800, Zhi Wang wrote:
> Hi Greg:
> 	Is there anything else I should do to let patches that has already been
> merged by linux-stable land in new 4.10.x tag? I have re-sent those patches
> with the #v4.10+ tag, but looks they still haven't been merged.

Ah, I totally missed these as it was not obvious at all that they were
wanted to go into a stable tree, and were already upstream.

You need to be a bit more specific, if you can include the "this is
commit XXXXXX" at the top of the email, that would be great, as we need
that information (and I don't see it in these patches at all, so it is
why I missed them.)

Can you fix that up and resend these?

thanks,

greg k-h

Re: [PATCH 1/3] drm/i915: Move the release of PT page to the upper caller

[Zhi Wang]

Hi Greg:
     Thanks for the explanation! :P I should CC you also in those 
patches. My mistake! Wait a second! :P

On 04/01/17 14:51, Greg KH wrote:
> On Sat, Apr 01, 2017 at 02:08:27PM +0800, Zhi Wang wrote:
>> Hi Greg:
>> 	Is there anything else I should do to let patches that has already been
>> merged by linux-stable land in new 4.10.x tag? I have re-sent those patches
>> with the #v4.10+ tag, but looks they still haven't been merged.
> Ah, I totally missed these as it was not obvious at all that they were
> wanted to go into a stable tree, and were already upstream.
>
> You need to be a bit more specific, if you can include the "this is
> commit XXXXXX" at the top of the email, that would be great, as we need
> that information (and I don't see it in these patches at all, so it is
> why I missed them.)
>
> Can you fix that up and resend these?
>
> thanks,
>
> greg k-h

Re: [PATCH 1/3] drm/i915: Move the release of PT page to the upper caller

[Greg KH]

On Sat, Apr 01, 2017 at 03:53:34PM +0800, Zhi Wang wrote:
> Hi Greg:
>     Thanks for the explanation! :P I should CC you also in those patches. My
> mistake! Wait a second! :P

No, sending them to stable@ is correct, no need to cc: me as well.

thanks,

greg k-h

[PATCH 2/3] drm/i915: Let execlist_update_context() cover !FULL_PPGTT mode.

[Zhi Wang]

execlist_update_context() will try to update PDPs in a context before a
ELSP submission only for full PPGTT mode, while PDPs was populated during
context initialization. Now the latter code path is removed. Let
execlist_update_context() also cover !FULL_PPGTT mode.

Fixes: 34869776c76b ("drm/i915: check ppgtt validity when init reg state")
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhiyuan Lv <zhiyuan.lv@intel.com>
Cc: Greg KH <gregkh@linuxfoundation.org> #v4.10+
Cc: stable@vger.kernel.org
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1486377436-15380-1-git-send-email-zhi.a.wang@intel.com
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index beabc17..2af4522 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -362,7 +362,8 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
 static u64 execlists_update_context(struct drm_i915_gem_request *rq)
 {
 	struct intel_context *ce = &rq->ctx->engine[rq->engine->id];
-	struct i915_hw_ppgtt *ppgtt = rq->ctx->ppgtt;
+	struct i915_hw_ppgtt *ppgtt =
+		rq->ctx->ppgtt ?: rq->i915->mm.aliasing_ppgtt;
 	u32 *reg_state = ce->lrc_reg_state;
 
 	reg_state[CTX_RING_TAIL+1] = rq->tail;
-- 
1.9.1