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@xxxxxxxxxxxxxxxxxx> > >Reviewed-by: Michał Winiarski <michal.winiarski@xxxxxxxxx> > >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> > >Link: https://patchwork.freedesktop.org/patch/122697/msgid/1479728666-25333-1-git-send-email-zhi.a.wang@xxxxxxxxx > >--- > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
