| Message ID | 1402673891-14618-3-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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. Try rcs_state for size, render_obj I think is confusing. -Chris
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Friday, June 13, 2014 6:01 PM > To: Mateo Lozano, Oscar > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 02/53] drm/i915: Rename ctx->obj to ctx- > >render_obj > > 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 No problem, I can add that on top without any hassle. I´ll send it together with a bunch of other prep-work patches. -- Oscar
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)