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

[PATCH] 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>
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
---
 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

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

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

[Zhi Wang]

Thanks Chirs and Michal :P Please take this patch. :)

On 11/29/16 14:55, 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>
> 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
> ---
>   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);
>   		}
>   	}
>   }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: Move the release of PT page to the upper caller (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Move the release of PT page to the upper caller (rev2)
URL   : https://patchwork.freedesktop.org/series/15651/
State : warning

== Summary ==

Series 15651v2 drm/i915: Move the release of PT page to the upper caller
https://patchwork.freedesktop.org/api/1.0/series/15651/revisions/2/mbox/

Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                dmesg-warn -> PASS       (fi-skl-6770hq)

fi-bdw-5557u     total:245  pass:230  dwarn:0   dfail:0   fail:0   skip:15 
fi-byt-j1900     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-ilk-650       total:245  pass:191  dwarn:1   dfail:0   fail:0   skip:53 
fi-skl-6260u     total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:245  pass:224  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:245  pass:223  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-bxt-t5700 failed to collect. IGT log at Patchwork_3132/fi-bxt-t5700/igt.log
fi-snb-2600 failed to collect. IGT log at Patchwork_3132/fi-snb-2600/igt.log

e9e1f8451437763fddfc9e53d008dcacd3ca2ad1 drm-tip: 2016y-11m-29d-04h-19m-22s UTC integration manifest
306982a drm/i915: Move the release of PT page to the upper caller

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3132/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 29/11/2016 06:55, 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>
> 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
> ---
>  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;
>  }

In this function you remove dev_priv local but it is still used in the 
function. And you also add one usage of to_i915(vm->dev).

I would leave dev_priv and use it. Avoids some head scratching at least. :)

Regards,

Tvrtko


> @@ -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);
>  		}
>  	}
>  }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Tue, Nov 29, 2016 at 08:48:08AM +0000, Tvrtko Ursulin wrote:
> 
> On 29/11/2016 06:55, 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>
> >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
> >---
> > 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;
> > }
> 
> In this function you remove dev_priv local but it is still used in
> the function. And you also add one usage of to_i915(vm->dev).

For the sake of saving another round of patches, I'm going to apply this
patch as is, and follow up with another suggestion...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] 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.

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>
Signed-off-by: Zhi Wang <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

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

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

[Zhi Wang]

Hi guys:
     Would you mind to have a quick review on this patch? :P The linux 
guest under GVT-g couldn't boot up without this patch in the newer kernel.

Thanks,
Zhi.

On 11/21/16 19:44, 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.
>
> 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>
> Signed-off-by: Zhi Wang <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);
>   		}
>   	}
>   }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Tue, Nov 22, 2016 at 09:29:40PM +0800, Zhi Wang wrote:
> Hi guys:
>     Would you mind to have a quick review on this patch? :P The
> linux guest under GVT-g couldn't boot up without this patch in the
> newer kernel.
> 
> Thanks,
> Zhi.
> 
> On 11/21/16 19:44, 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.
> >
> >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>
> >Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>

The original didn't make it to me, so I just assumed a list issue.

For the record, I'd like to have some more detail on just when the hv is
doing the page scanning. It makes it sound like you are actively
scanning an idle range.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhenyu Wang]

[-- Attachment #1.1: Type: text/plain, Size: 2053 bytes --]

On 2016.11.22 14:38:19 +0000, Chris Wilson wrote:
> On Tue, Nov 22, 2016 at 09:29:40PM +0800, Zhi Wang wrote:
> > Hi guys:
> >     Would you mind to have a quick review on this patch? :P The
> > linux guest under GVT-g couldn't boot up without this patch in the
> > newer kernel.
> > 
> > Thanks,
> > Zhi.
> > 
> > On 11/21/16 19:44, 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.
> > >
> > >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>
> > >Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> 
> The original didn't make it to me, so I just assumed a list issue.
> 
> For the record, I'd like to have some more detail on just when the hv is
> doing the page scanning. It makes it sound like you are actively
> scanning an idle range.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Note that this should be queued for 4.9 fix otherwise 4.9 will be broken
as guest kernel version for GVT-g.

Thanks

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Wang, Zhi A]

Hi Chris, thanks for the reply! Without this patch, we have to do pattern scan to identify if the page is still being used as a PT page. :( It's complicated.

Originally, all the guest shadow PPGTT pages will be set to write-protected by HV. When guest makes a change in its page table, it will be trapped by HV.

For example:
Guest updates a PTE entry in a PTE PT page, then HV will be notified and populate the shadow PPGTT page table accordingly. Now let's say a PTE PT is empty (all mappings are pointing to scratch page) and will be freed in the newer kernel. If guest freed the PT page first, this page could be used by anyone while HV is still treating this page as PT page, then things goes wrong here. HV has to identify the page is not being used as PT page anymore at this time.

While if the guest updated the upper level PT entry first, HV will know the PTE PT page will not be used as a page table page, and stop track that page at this time. Then following accesses to that page will not be trapped by HV and surely HV will not see unrecognized PT entry writing (this page may be used by other guy at this time.)

Thanks,
Zhi.

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Tuesday, November 22, 2016 10:38 PM
> To: Wang, Zhi A
> Cc: intel-gfx@lists.freedesktop.org; Winiarski, Michal; Thierry, Michel; Joonas
> Lahtinen; Zhenyu Wang; Lv, Zhiyuan
> Subject: Re: [PATCH] drm/i915: Move the release of PT page to the upper caller
> 
> On Tue, Nov 22, 2016 at 09:29:40PM +0800, Zhi Wang wrote:
> > Hi guys:
> >     Would you mind to have a quick review on this patch? :P The linux
> > guest under GVT-g couldn't boot up without this patch in the newer
> > kernel.
> >
> > Thanks,
> > Zhi.
> >
> > On 11/21/16 19:44, 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.
> > >
> > >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>
> > >Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> 
> The original didn't make it to me, so I just assumed a list issue.
> 
> For the record, I'd like to have some more detail on just when the hv is doing
> the page scanning. It makes it sound like you are actively scanning an idle
> range.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tian, Kevin]

It's also the style how CPU page table is managed...

> From: Wang, Zhi A
> Sent: Wednesday, November 23, 2016 3:37 PM
> 
> Hi Chris, thanks for the reply! Without this patch, we have to do pattern scan to identify
> if the page is still being used as a PT page. :( It's complicated.
> 
> Originally, all the guest shadow PPGTT pages will be set to write-protected by HV. When
> guest makes a change in its page table, it will be trapped by HV.
> 
> For example:
> Guest updates a PTE entry in a PTE PT page, then HV will be notified and populate the
> shadow PPGTT page table accordingly. Now let's say a PTE PT is empty (all mappings are
> pointing to scratch page) and will be freed in the newer kernel. If guest freed the PT page
> first, this page could be used by anyone while HV is still treating this page as PT page, then
> things goes wrong here. HV has to identify the page is not being used as PT page anymore
> at this time.
> 
> While if the guest updated the upper level PT entry first, HV will know the PTE PT page will
> not be used as a page table page, and stop track that page at this time. Then following
> accesses to that page will not be trapped by HV and surely HV will not see unrecognized
> PT entry writing (this page may be used by other guy at this time.)
> 
> Thanks,
> Zhi.
> 
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Tuesday, November 22, 2016 10:38 PM
> > To: Wang, Zhi A
> > Cc: intel-gfx@lists.freedesktop.org; Winiarski, Michal; Thierry, Michel; Joonas
> > Lahtinen; Zhenyu Wang; Lv, Zhiyuan
> > Subject: Re: [PATCH] drm/i915: Move the release of PT page to the upper caller
> >
> > On Tue, Nov 22, 2016 at 09:29:40PM +0800, Zhi Wang wrote:
> > > Hi guys:
> > >     Would you mind to have a quick review on this patch? :P The linux
> > > guest under GVT-g couldn't boot up without this patch in the newer
> > > kernel.
> > >
> > > Thanks,
> > > Zhi.
> > >
> > > On 11/21/16 19:44, 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.
> > > >
> > > >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>
> > > >Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> >
> > The original didn't make it to me, so I just assumed a list issue.
> >
> > For the record, I'd like to have some more detail on just when the hv is doing
> > the page scanning. It makes it sound like you are actively scanning an idle
> > range.
> >
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
> >
> > --
> > Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Nov 23, 2016 at 07:43:21AM +0000, Tian, Kevin wrote:
> It's also the style how CPU page table is managed...

But it is not the style of how the GPU page table behaves, which is what
the code expects. My only concern here is if the fact that the page is
not idle has further rammifactions.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michał Winiarski]

On Mon, Nov 21, 2016 at 07:44:26PM +0800, 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: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> 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>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhi Wang]

Thanks Michał! Have a good day. :P

On 11/22/16 22:39, Michał Winiarski wrote:
> On Mon, Nov 21, 2016 at 07:44:26PM +0800, 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: Michał Winiarski <michal.winiarski@intel.com>
>
> -Michał
>
>> 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>
>> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 18 +++++++-----------
>>   1 file changed, 7 insertions(+), 11 deletions(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx