Re: [PATCH 02/53] drm/i915: Rename ctx->obj to ctx->render_obj

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 13, 2014 at 04:37:20PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> The reason for doing this will be better explained in the following
> patch. For now, suffice it to say that this backing object is only
> used with the render ring, so we're making this fact more explicit.
> 
> Done with the following Coccinelle patch (plus manual renaming of the
> struct field):
> 
> 	@@
> 	struct intel_context c;
> 	@@
> 	- (c).obj
> 	+ c.render_obj
> 
> 	@@
> 	struct intel_context *c;
> 	@@
> 	- (c)->obj
> 	+ c->render_obj
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

Just screamed at this code reviewing a bugfix from Chris and I really like
this. Can we have a s/is_initialized/render_is_initialized/ on top pls?

Or does that interfere too much with the series? I didn't look ahead ...
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  4 +--
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>  drivers/gpu/drm/i915/i915_gem_context.c | 63 +++++++++++++++++----------------
>  3 files changed, 35 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7b83297..b09cab4 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1735,7 +1735,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
>  	}
>  
>  	list_for_each_entry(ctx, &dev_priv->context_list, link) {
> -		if (ctx->obj == NULL)
> +		if (ctx->render_obj == NULL)
>  			continue;
>  
>  		seq_puts(m, "HW context ");
> @@ -1744,7 +1744,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
>  			if (ring->default_context == ctx)
>  				seq_printf(m, "(default context %s) ", ring->name);
>  
> -		describe_obj(m, ctx->obj);
> +		describe_obj(m, ctx->render_obj);
>  		seq_putc(m, '\n');
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 24f084d..1cebbd4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -592,7 +592,7 @@ struct intel_context {
>  	uint8_t remap_slice;
>  	struct drm_i915_file_private *file_priv;
>  	struct intel_engine_cs *last_ring;
> -	struct drm_i915_gem_object *obj;
> +	struct drm_i915_gem_object *render_obj;
>  	struct i915_ctx_hang_stats hang_stats;
>  	struct i915_address_space *vm;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 4efa5ca..f27886a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -182,14 +182,14 @@ void i915_gem_context_free(struct kref *ctx_ref)
>  						   typeof(*ctx), ref);
>  	struct i915_hw_ppgtt *ppgtt = NULL;
>  
> -	if (ctx->obj) {
> +	if (ctx->render_obj) {
>  		/* We refcount even the aliasing PPGTT to keep the code symmetric */
> -		if (USES_PPGTT(ctx->obj->base.dev))
> +		if (USES_PPGTT(ctx->render_obj->base.dev))
>  			ppgtt = ctx_to_ppgtt(ctx);
>  
>  		/* XXX: Free up the object before tearing down the address space, in
>  		 * case we're bound in the PPGTT */
> -		drm_gem_object_unreference(&ctx->obj->base);
> +		drm_gem_object_unreference(&ctx->render_obj->base);
>  	}
>  
>  	if (ppgtt)
> @@ -270,7 +270,7 @@ __create_hw_context(struct drm_device *dev,
>  			ret = PTR_ERR(obj);
>  			goto err_out;
>  		}
> -		ctx->obj = obj;
> +		ctx->render_obj = obj;
>  	}
>  
>  	/* Default context will never have a file_priv */
> @@ -317,7 +317,7 @@ i915_gem_create_context(struct drm_device *dev,
>  	if (IS_ERR(ctx))
>  		return ctx;
>  
> -	if (is_global_default_ctx && ctx->obj) {
> +	if (is_global_default_ctx && ctx->render_obj) {
>  		/* We may need to do things with the shrinker which
>  		 * require us to immediately switch back to the default
>  		 * context. This can cause a problem as pinning the
> @@ -325,7 +325,7 @@ i915_gem_create_context(struct drm_device *dev,
>  		 * be available. To avoid this we always pin the default
>  		 * context.
>  		 */
> -		ret = i915_gem_obj_ggtt_pin(ctx->obj,
> +		ret = i915_gem_obj_ggtt_pin(ctx->render_obj,
>  					    get_context_alignment(dev), 0);
>  		if (ret) {
>  			DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret);
> @@ -365,8 +365,8 @@ i915_gem_create_context(struct drm_device *dev,
>  	return ctx;
>  
>  err_unpin:
> -	if (is_global_default_ctx && ctx->obj)
> -		i915_gem_object_ggtt_unpin(ctx->obj);
> +	if (is_global_default_ctx && ctx->render_obj)
> +		i915_gem_object_ggtt_unpin(ctx->render_obj);
>  err_destroy:
>  	i915_gem_context_unreference(ctx);
>  	return ERR_PTR(ret);
> @@ -390,12 +390,12 @@ void i915_gem_context_reset(struct drm_device *dev)
>  		if (!ring->last_context)
>  			continue;
>  
> -		if (dctx->obj && i == RCS) {
> -			WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
> +		if (dctx->render_obj && i == RCS) {
> +			WARN_ON(i915_gem_obj_ggtt_pin(dctx->render_obj,
>  						      get_context_alignment(dev), 0));
>  			/* Fake a finish/inactive */
> -			dctx->obj->base.write_domain = 0;
> -			dctx->obj->active = 0;
> +			dctx->render_obj->base.write_domain = 0;
> +			dctx->render_obj->active = 0;
>  		}
>  
>  		i915_gem_context_unreference(ring->last_context);
> @@ -445,7 +445,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
>  	int i;
>  
> -	if (dctx->obj) {
> +	if (dctx->render_obj) {
>  		/* The only known way to stop the gpu from accessing the hw context is
>  		 * to reset it. Do this as the very last operation to avoid confusing
>  		 * other code, leading to spurious errors. */
> @@ -460,13 +460,13 @@ void i915_gem_context_fini(struct drm_device *dev)
>  		WARN_ON(!dev_priv->ring[RCS].last_context);
>  		if (dev_priv->ring[RCS].last_context == dctx) {
>  			/* Fake switch to NULL context */
> -			WARN_ON(dctx->obj->active);
> -			i915_gem_object_ggtt_unpin(dctx->obj);
> +			WARN_ON(dctx->render_obj->active);
> +			i915_gem_object_ggtt_unpin(dctx->render_obj);
>  			i915_gem_context_unreference(dctx);
>  			dev_priv->ring[RCS].last_context = NULL;
>  		}
>  
> -		i915_gem_object_ggtt_unpin(dctx->obj);
> +		i915_gem_object_ggtt_unpin(dctx->render_obj);
>  	}
>  
>  	for (i = 0; i < I915_NUM_RINGS; i++) {
> @@ -586,7 +586,7 @@ mi_set_context(struct intel_engine_cs *ring,
>  
>  	intel_ring_emit(ring, MI_NOOP);
>  	intel_ring_emit(ring, MI_SET_CONTEXT);
> -	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->obj) |
> +	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->render_obj) |
>  			MI_MM_SPACE_GTT |
>  			MI_SAVE_EXT_STATE_EN |
>  			MI_RESTORE_EXT_STATE_EN |
> @@ -617,8 +617,8 @@ static int do_switch(struct intel_engine_cs *ring,
>  	int ret, i;
>  
>  	if (from != NULL && ring == &dev_priv->ring[RCS]) {
> -		BUG_ON(from->obj == NULL);
> -		BUG_ON(!i915_gem_obj_is_pinned(from->obj));
> +		BUG_ON(from->render_obj == NULL);
> +		BUG_ON(!i915_gem_obj_is_pinned(from->render_obj));
>  	}
>  
>  	if (from == to && from->last_ring == ring && !to->remap_slice)
> @@ -626,7 +626,7 @@ static int do_switch(struct intel_engine_cs *ring,
>  
>  	/* Trying to pin first makes error handling easier. */
>  	if (ring == &dev_priv->ring[RCS]) {
> -		ret = i915_gem_obj_ggtt_pin(to->obj,
> +		ret = i915_gem_obj_ggtt_pin(to->render_obj,
>  					    get_context_alignment(ring->dev), 0);
>  		if (ret)
>  			return ret;
> @@ -659,14 +659,14 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 *
>  	 * XXX: We need a real interface to do this instead of trickery.
>  	 */
> -	ret = i915_gem_object_set_to_gtt_domain(to->obj, false);
> +	ret = i915_gem_object_set_to_gtt_domain(to->render_obj, false);
>  	if (ret)
>  		goto unpin_out;
>  
> -	if (!to->obj->has_global_gtt_mapping) {
> -		struct i915_vma *vma = i915_gem_obj_to_vma(to->obj,
> +	if (!to->render_obj->has_global_gtt_mapping) {
> +		struct i915_vma *vma = i915_gem_obj_to_vma(to->render_obj,
>  							   &dev_priv->gtt.base);
> -		vma->bind_vma(vma, to->obj->cache_level, GLOBAL_BIND);
> +		vma->bind_vma(vma, to->render_obj->cache_level, GLOBAL_BIND);
>  	}
>  
>  	if (!to->is_initialized || i915_gem_context_is_default(to))
> @@ -695,8 +695,9 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 * MI_SET_CONTEXT instead of when the next seqno has completed.
>  	 */
>  	if (from != NULL) {
> -		from->obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> -		i915_vma_move_to_active(i915_gem_obj_to_ggtt(from->obj), ring);
> +		from->render_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> +		i915_vma_move_to_active(i915_gem_obj_to_ggtt(from->render_obj),
> +					ring);
>  		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
>  		 * whole damn pipeline, we don't need to explicitly mark the
>  		 * object dirty. The only exception is that the context must be
> @@ -704,11 +705,11 @@ static int do_switch(struct intel_engine_cs *ring,
>  		 * able to defer doing this until we know the object would be
>  		 * swapped, but there is no way to do that yet.
>  		 */
> -		from->obj->dirty = 1;
> -		BUG_ON(from->obj->ring != ring);
> +		from->render_obj->dirty = 1;
> +		BUG_ON(from->render_obj->ring != ring);
>  
>  		/* obj is kept alive until the next request by its active ref */
> -		i915_gem_object_ggtt_unpin(from->obj);
> +		i915_gem_object_ggtt_unpin(from->render_obj);
>  		i915_gem_context_unreference(from);
>  	}
>  
> @@ -729,7 +730,7 @@ done:
>  
>  unpin_out:
>  	if (ring->id == RCS)
> -		i915_gem_object_ggtt_unpin(to->obj);
> +		i915_gem_object_ggtt_unpin(to->render_obj);
>  	return ret;
>  }
>  
> @@ -750,7 +751,7 @@ int i915_switch_context(struct intel_engine_cs *ring,
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>  
> -	if (to->obj == NULL) { /* We have the fake context */
> +	if (to->render_obj == NULL) { /* We have the fake context */
>  		if (to != ring->last_context) {
>  			i915_gem_context_reference(to);
>  			if (ring->last_context)
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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