[Intel-gfx] [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 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).

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);
>  		}
>  	}
>  }
>