| Message ID | 1480402516-22275-1-git-send-email-zhi.a.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > } > } > } >
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); > } > } > } >
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
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); } } }