[Intel-gfx] [PATCH 06/15] drm/i915: Allow the caller to create a intel_context without PPGTT

[Wang, Zhi A]

Yes. Thanks! Seems there is an assumption if ctx->ppgtt == NULL then go back to use aliasing ppgtt. I see. I will drop that patch, without that patch we could also work

-----Original Message-----
From: Chris Wilson [mailto:chris at chris-wilson.co.uk] 
Sent: Monday, May 16, 2016 8:13 AM
To: Wang, Zhi A <zhi.a.wang at intel.com>
Cc: intel-gfx at lists.freedesktop.org; Gordon, David S <david.s.gordon at intel.com>; joonas.lahtinen at linux.intel.com; Tian, Kevin <kevin.tian at intel.com>; Lv, Zhiyuan <zhiyuan.lv at intel.com>
Subject: Re: [Intel-gfx] [PATCH 06/15] drm/i915: Allow the caller to create a intel_context without PPGTT

On Mon, May 16, 2016 at 01:32:44AM +0800, Zhi Wang wrote:
> GVT context will use its own shadow PPGTT, and it doesn't need a 
> i915_hw_ppgtt.
> 
> This patch adds a "has_ppgtt" param to i915_gem_context(), which 
> allows the caller to create a context without PPGTT
> 
> Signed-off-by: Zhi Wang <zhi.a.wang at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 34 
> ++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 21498e5..b952e37 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -298,7 +298,8 @@ err_out:
>   */
>  static struct intel_context *
>  i915_gem_create_context(struct drm_device *dev,
> -			struct drm_i915_file_private *file_priv)
> +			struct drm_i915_file_private *file_priv,
> +			bool has_ppgtt)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	const bool is_global_default_ctx = file_priv == NULL; @@ -327,19 
> +328,22 @@ i915_gem_create_context(struct drm_device *dev,
>  		}
>  	}
>  
> -	if (USES_FULL_PPGTT(dev)) {
> -		struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv);
> +	if (has_ppgtt) {
> +		if (USES_FULL_PPGTT(dev)) {
> +			struct i915_hw_ppgtt *ppgtt =
> +				i915_ppgtt_create(dev, file_priv);
>  
> -		if (IS_ERR_OR_NULL(ppgtt)) {
> -			DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
> -					 PTR_ERR(ppgtt));
> -			ret = PTR_ERR(ppgtt);
> -			goto err_unpin;
> -		}
> +			if (IS_ERR_OR_NULL(ppgtt)) {
> +				DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
> +						PTR_ERR(ppgtt));
> +				ret = PTR_ERR(ppgtt);
> +				goto err_unpin;
> +			}
>  
> -		ctx->ppgtt = ppgtt;
> -	} else
> -		ctx->ppgtt = dev_priv->mm.aliasing_ppgtt;
> +			ctx->ppgtt = ppgtt;
> +		} else
> +			ctx->ppgtt = dev_priv->mm.aliasing_ppgtt;

You have to first go through the driver and update the sematics for
ctx->ppgtt == NULL. (Note in some instances you have to use the
ggtt pointer and not the appgtt, just a minor case in execbuf!).
Then tell us how you didn't spot the explosion when trying to use the aliasing_ppgtt after freeing it.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre