[Intel-gfx] [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 at chris-wilson.co.uk>
> >Reviewed-by: Michał Winiarski <michal.winiarski at intel.com>
> >Cc: Michał Winiarski <michal.winiarski at intel.com>
> >Cc: Michel Thierry <michel.thierry at intel.com>
> >Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >Cc: Zhenyu Wang <zhenyuw at linux.intel.com>
> >Cc: Zhiyuan Lv <zhiyuan.lv at intel.com>
> >Signed-off-by: Zhi Wang <zhi.a.wang at 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