On Fri, Jun 13, 2014 at 04:37:20PM +0100, oscar.mateo@xxxxxxxxx wrote:
> From: Oscar Mateo <oscar.mateo@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
