Re: [PATCH 6/7] drm/i915: Only track real ppgtt for a context

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 30, 2014 at 09:42:03PM +0200, Daniel Vetter wrote:
> There's a bit a confusion since we track the global gtt,
> the aliasing and real ppgtt in the ctx->vm pointer. And not
> all callers really bother to check for the different cases and just
> presume that it points to a real ppgtt.
> 
> Now looking closely we don't actually need ->vm to always point at an
> address space - the only place that cares actually has fixup code
> already to decide whether to look at the per-proces or the global
> address space.
> 
> So switch to just tracking the ppgtt directly and ditch all the
> extraneous code.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Jesse looked up the Jira for this one for me, so

OTC-Jira: VIZ-3724

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h            |  3 +--
>  drivers/gpu/drm/i915/i915_gem_context.c    | 28 +++++++---------------------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 +++--
>  drivers/gpu/drm/i915/i915_gpu_error.c      | 10 +++++++---
>  5 files changed, 19 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3bf1d20c598b..aaf07e230cb0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1774,7 +1774,7 @@ static int per_file_ctx(int id, void *ptr, void *data)
>  {
>  	struct intel_context *ctx = ptr;
>  	struct seq_file *m = data;
> -	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(ctx);
> +	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
>  
>  	if (i915_gem_context_is_default(ctx))
>  		seq_puts(m, "  default context:\n");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0762e19f9bc6..3230b08aff13 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -616,7 +616,7 @@ struct intel_context {
>  	uint8_t remap_slice;
>  	struct drm_i915_file_private *file_priv;
>  	struct i915_ctx_hang_stats hang_stats;
> -	struct i915_address_space *vm;
> +	struct i915_hw_ppgtt *ppgtt;
>  
>  	struct {
>  		struct drm_i915_gem_object *rcs_state;
> @@ -2504,7 +2504,6 @@ i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
>  void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
>  
>  /* i915_gem_context.c */
> -#define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base)
>  int __must_check i915_gem_context_init(struct drm_device *dev);
>  void i915_gem_context_fini(struct drm_device *dev);
>  void i915_gem_context_reset(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 7a455fcee3a7..c00e5d027774 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -136,15 +136,8 @@ void i915_gem_context_free(struct kref *ctx_ref)
>  {
>  	struct intel_context *ctx = container_of(ctx_ref,
>  						   typeof(*ctx), ref);
> -	struct i915_hw_ppgtt *ppgtt = NULL;
>  
> -	if (ctx->legacy_hw_ctx.rcs_state) {
> -		/* We refcount even the aliasing PPGTT to keep the code symmetric */
> -		if (USES_PPGTT(ctx->legacy_hw_ctx.rcs_state->base.dev))
> -			ppgtt = ctx_to_ppgtt(ctx);
> -	}
> -
> -	i915_ppgtt_put(ppgtt);
> +	i915_ppgtt_put(ctx->ppgtt);
>  	if (ctx->legacy_hw_ctx.rcs_state)
>  		drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
>  	list_del(&ctx->link);
> @@ -240,7 +233,6 @@ i915_gem_create_context(struct drm_device *dev,
>  			bool create_vm)
>  {
>  	const bool is_global_default_ctx = file_priv == NULL;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_context *ctx;
>  	int ret = 0;
>  
> @@ -274,15 +266,10 @@ i915_gem_create_context(struct drm_device *dev,
>  					 PTR_ERR(ppgtt));
>  			ret = PTR_ERR(ppgtt);
>  			goto err_unpin;
> -		} else
> -			ctx->vm = &ppgtt->base;
> -	} else if (USES_PPGTT(dev)) {
> -		/* For platforms which only have aliasing PPGTT, we fake the
> -		 * address space and refcounting. */
> -		ctx->vm = &dev_priv->mm.aliasing_ppgtt->base;
> -		i915_ppgtt_get(dev_priv->mm.aliasing_ppgtt);
> -	} else
> -		ctx->vm = &dev_priv->gtt.base;
> +		}
> +
> +		ctx->ppgtt = ppgtt;
> +	}
>  
>  	return ctx;
>  
> @@ -538,7 +525,6 @@ static int do_switch(struct intel_engine_cs *ring,
>  {
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	struct intel_context *from = ring->last_context;
> -	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
>  	u32 hw_flags = 0;
>  	bool uninitialized = false;
>  	int ret, i;
> @@ -566,8 +552,8 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 */
>  	from = ring->last_context;
>  
> -	if (USES_FULL_PPGTT(ring->dev)) {
> -		ret = ppgtt->switch_mm(ppgtt, ring, false);
> +	if (to->ppgtt) {
> +		ret = to->ppgtt->switch_mm(to->ppgtt, ring, false);
>  		if (ret)
>  			goto unpin_out;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 60998fc4e5b2..54b3d8c8b228 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1318,8 +1318,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  
>  	i915_gem_context_reference(ctx);
>  
> -	vm = ctx->vm;
> -	if (!USES_FULL_PPGTT(dev))
> +	if (ctx->ppgtt)
> +		vm = &ctx->ppgtt->base;
> +	else
>  		vm = &dev_priv->gtt.base;
>  
>  	eb = eb_create(args);
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 0b3f69439451..06072120b15d 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -958,6 +958,12 @@ static void i915_gem_record_rings(struct drm_device *dev,
>  
>  		request = i915_gem_find_active_request(ring);
>  		if (request) {
> +			struct i915_address_space *vm;
> +
> +			vm = request->ctx && request->ctx->ppgtt ?
> +				&request->ctx->ppgtt->base :
> +				&dev_priv->gtt.base;
> +
>  			/* We need to copy these to an anonymous buffer
>  			 * as the simplest method to avoid being overwritten
>  			 * by userspace.
> @@ -965,9 +971,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
>  			error->ring[i].batchbuffer =
>  				i915_error_object_create(dev_priv,
>  							 request->batch_obj,
> -							 request->ctx ?
> -							 request->ctx->vm :
> -							 &dev_priv->gtt.base);
> +							 vm);
>  
>  			if (HAS_BROKEN_CS_TLB(dev_priv->dev) &&
>  			    ring->scratch.obj)
> -- 
> 1.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch