[Intel-gfx] [PATCH 05/15] drm/i915: Set ctx->ppgtt for aliasing PPGTT in context creation

[Wang, Zhi A]

I understand what you say. Let me think about other approach. My requirement is to do nothing if a context doesn't have a ppgtt, but ctx->ppgtt == NULL means context is using aliasing ppgtt, so I need to find another way to achieve this. 

-----Original Message-----
From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com] 
Sent: Monday, May 16, 2016 6:31 AM
To: Wang, Zhi A <zhi.a.wang at intel.com>; 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 05/15] drm/i915: Set ctx->ppgtt for aliasing PPGTT in context creation


On 15/05/16 18:32, Zhi Wang wrote:
> Currently ctx->ppgtt would only be initialized when full PPGTT is used.
> For aliasing PPGTT mode, ctx->ppgtt will be set when LRC context is 
> populated.
>
> This patch moves the assignment into i915_gem_create_context() for 
> better code structure.

Hm, it doesn't move the assignment it adds it.

Previously ctx->ppgtt was always NULL when !USES_FULL_PPGTT. Now it will point to aliasing ppgtt.

Since there are various places in the code which do "if (ctx->ppgtt)" 
(more or less) I think you need to explain why those will still work OK.

i915_gem_context_free for example?

Regards,

Tvrtko


> Signed-off-by: Zhi Wang <zhi.a.wang at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 4 +++-
>   drivers/gpu/drm/i915/intel_lrc.c        | 3 ---
>   2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 2aedd18..21498e5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -300,6 +300,7 @@ static struct intel_context *
>   i915_gem_create_context(struct drm_device *dev,
>   			struct drm_i915_file_private *file_priv)
>   {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>   	const bool is_global_default_ctx = file_priv == NULL;
>   	struct intel_context *ctx;
>   	int ret = 0;
> @@ -337,7 +338,8 @@ i915_gem_create_context(struct drm_device *dev,
>   		}
>
>   		ctx->ppgtt = ppgtt;
> -	}
> +	} else
> +		ctx->ppgtt = dev_priv->mm.aliasing_ppgtt;
>
>   	trace_i915_context_create(ctx);
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> b/drivers/gpu/drm/i915/intel_lrc.c
> index db10c96..397fe65 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2286,9 +2286,6 @@ populate_lr_context(struct intel_context *ctx,
>   	u32 *reg_state;
>   	int ret;
>
> -	if (!ppgtt)
> -		ppgtt = dev_priv->mm.aliasing_ppgtt;
> -
>   	ret = i915_gem_object_set_to_cpu_domain(ctx_obj, true);
>   	if (ret) {
>   		DRM_DEBUG_DRIVER("Could not set to CPU domain\n");
>