* Re: [PATCH] drm/i915: Always use kref tracking for all contexts.
2014-04-03 7:06 [PATCH] drm/i915: Always use kref tracking for all contexts Chris Wilson
@ 2014-04-03 10:36 ` Mika Kuoppala
2014-04-03 10:44 ` Chris Wilson
2014-04-04 4:44 ` Ben Widawsky
2014-04-04 13:40 ` Chris Wilson
2 siblings, 1 reply; 10+ messages in thread
From: Mika Kuoppala @ 2014-04-03 10:36 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Ben Widawsky
Chris Wilson <chris@chris-wilson.co.uk> writes:
> If we always initialize kref for the context, even if we are using fake
> contexts for hangstats when there is no hw support, we can forgo the
> dance to dereference the ctx->obj and inspect whether we are permitted
> to use kref inside i915_gem_context_reference() and _unreference().
>
> My ulterior motive here is to improve the debugging of a use-after-free
> of ctx->obj. This patch avoids the dereference here and instead forces
> the assertion checks associated with kref.
>
> v2: Refactor the fake contexts to being even more like the real
> contexts, so that there is much less duplicated and special case code.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76671
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: lu hua <huax.lu@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 8 +-
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_context.c | 175 ++++++++++++-----------------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> 4 files changed, 76 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0e2de9d54b8..97bf793f6552 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2275,20 +2275,18 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> int i915_gem_context_enable(struct drm_i915_private *dev_priv);
> void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> int i915_switch_context(struct intel_ring_buffer *ring,
> - struct drm_file *file, struct i915_hw_context *to);
> + struct i915_hw_context *to);
> struct i915_hw_context *
> i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
> void i915_gem_context_free(struct kref *ctx_ref);
> static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
> {
> - if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> - kref_get(&ctx->ref);
> + kref_get(&ctx->ref);
> }
>
> static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
> {
> - if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> - kref_put(&ctx->ref, i915_gem_context_free);
> + kref_put(&ctx->ref, i915_gem_context_free);
> }
>
> static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 221f6ab3a498..0ff65666c5f7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2819,7 +2819,7 @@ int i915_gpu_idle(struct drm_device *dev)
>
> /* Flush everything onto the inactive list. */
> for_each_ring(ring, dev_priv, i) {
> - ret = i915_switch_context(ring, NULL, ring->default_context);
> + ret = i915_switch_context(ring, ring->default_context);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index edbad2787d65..1439cc6bd439 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -96,9 +96,6 @@
> #define GEN6_CONTEXT_ALIGN (64<<10)
> #define GEN7_CONTEXT_ALIGN 4096
>
> -static int do_switch(struct intel_ring_buffer *ring,
> - struct i915_hw_context *to);
> -
> static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
> {
> struct drm_device *dev = ppgtt->base.dev;
> @@ -185,13 +182,15 @@ void i915_gem_context_free(struct kref *ctx_ref)
> typeof(*ctx), ref);
> struct i915_hw_ppgtt *ppgtt = NULL;
>
> - /* We refcount even the aliasing PPGTT to keep the code symmetric */
> - if (USES_PPGTT(ctx->obj->base.dev))
> - ppgtt = ctx_to_ppgtt(ctx);
> + if (ctx->obj) {
> + /* We refcount even the aliasing PPGTT to keep the code symmetric */
> + if (USES_PPGTT(ctx->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);
> + /* 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);
> + }
>
> if (ppgtt)
> kref_put(&ppgtt->ref, ppgtt_release);
> @@ -232,32 +231,32 @@ __create_hw_context(struct drm_device *dev,
> return ERR_PTR(-ENOMEM);
>
> kref_init(&ctx->ref);
> - ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> - INIT_LIST_HEAD(&ctx->link);
> - if (ctx->obj == NULL) {
> - kfree(ctx);
> - DRM_DEBUG_DRIVER("Context object allocated failed\n");
> - return ERR_PTR(-ENOMEM);
> - }
> + list_add_tail(&ctx->link, &dev_priv->context_list);
>
> - if (INTEL_INFO(dev)->gen >= 7) {
> - ret = i915_gem_object_set_cache_level(ctx->obj,
> - I915_CACHE_L3_LLC);
> - /* Failure shouldn't ever happen this early */
> - if (WARN_ON(ret))
> + if (dev_priv->hw_context_size) {
> + ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> + if (ctx->obj == NULL) {
> + ret = -ENOMEM;
> goto err_out;
> - }
> + }
>
> - list_add_tail(&ctx->link, &dev_priv->context_list);
> + if (INTEL_INFO(dev)->gen >= 7) {
> + ret = i915_gem_object_set_cache_level(ctx->obj,
> + I915_CACHE_L3_LLC);
> + /* Failure shouldn't ever happen this early */
> + if (WARN_ON(ret))
> + goto err_out;
> + }
> + }
>
> /* Default context will never have a file_priv */
> - if (file_priv == NULL)
> - return ctx;
> -
> - ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID, 0,
> - GFP_KERNEL);
> - if (ret < 0)
> - goto err_out;
> + if (file_priv != NULL) {
> + ret = idr_alloc(&file_priv->context_idr, ctx,
> + DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
> + if (ret < 0)
> + goto err_out;
> + } else
> + ret = DEFAULT_CONTEXT_ID;
>
> ctx->file_priv = file_priv;
> ctx->id = ret;
> @@ -294,7 +293,7 @@ i915_gem_create_context(struct drm_device *dev,
> if (IS_ERR(ctx))
> return ctx;
>
> - if (is_global_default_ctx) {
> + if (is_global_default_ctx && ctx->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
> @@ -342,7 +341,7 @@ i915_gem_create_context(struct drm_device *dev,
> return ctx;
>
> err_unpin:
> - if (is_global_default_ctx)
> + if (is_global_default_ctx && ctx->obj)
> i915_gem_object_ggtt_unpin(ctx->obj);
> err_destroy:
> i915_gem_context_unreference(ctx);
> @@ -352,16 +351,14 @@ err_destroy:
> void i915_gem_context_reset(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_ring_buffer *ring;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return;
> -
> /* Prevent the hardware from restoring the last context (which hung) on
> * the next switch */
> for (i = 0; i < I915_NUM_RINGS; i++) {
> struct i915_hw_context *dctx;
> + struct intel_ring_buffer *ring;
> +
> if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> continue;
>
> @@ -377,7 +374,7 @@ void i915_gem_context_reset(struct drm_device *dev)
> if (ring->last_context == dctx)
> continue;
>
> - if (i == RCS) {
> + if (dctx->obj && i == RCS) {
> WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
> get_context_alignment(dev), 0));
> /* Fake a finish/inactive */
> @@ -394,44 +391,35 @@ void i915_gem_context_reset(struct drm_device *dev)
> int i915_gem_context_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_ring_buffer *ring;
> + struct i915_hw_context *ctx;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return 0;
> -
> /* Init should only be called once per module load. Eventually the
> * restriction on the context_disabled check can be loosened. */
> if (WARN_ON(dev_priv->ring[RCS].default_context))
> return 0;
>
> - dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> -
> - if (dev_priv->hw_context_size > (1<<20)) {
> - DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
> - return -E2BIG;
> + if (HAS_HW_CONTEXTS(dev)) {
> + dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> + if (dev_priv->hw_context_size > (1<<20)) {
> + DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
> + dev_priv->hw_context_size);
> + dev_priv->hw_context_size = 0;
> + }
> }
>
> - dev_priv->ring[RCS].default_context =
> - i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> -
> - if (IS_ERR_OR_NULL(dev_priv->ring[RCS].default_context)) {
> - DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %ld\n",
> - PTR_ERR(dev_priv->ring[RCS].default_context));
> - return PTR_ERR(dev_priv->ring[RCS].default_context);
> + ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> + if (IS_ERR(ctx)) {
> + DRM_ERROR("Failed to create default global context (error %ld)\n",
> + PTR_ERR(ctx));
> + return PTR_ERR(ctx);
> }
>
> - for (i = RCS + 1; i < I915_NUM_RINGS; i++) {
> - if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> - continue;
> + /* NB: RCS will hold a ref for all rings */
> + for (i = 0; i < I915_NUM_RINGS; i++)
> + dev_priv->ring[i].default_context = ctx;
>
> - ring = &dev_priv->ring[i];
> -
> - /* NB: RCS will hold a ref for all rings */
> - ring->default_context = dev_priv->ring[RCS].default_context;
> - }
> -
> - DRM_DEBUG_DRIVER("HW context support initialized\n");
> + DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
> return 0;
> }
>
> @@ -441,13 +429,11 @@ void i915_gem_context_fini(struct drm_device *dev)
> struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return;
> -
> - /* 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. */
> - intel_gpu_reset(dev);
> + if (dev_priv->hw_context_size)
> + /* 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. */
> + intel_gpu_reset(dev);
>
> /* When default context is created and switched to, base object refcount
> * will be 2 (+1 from object creation and +1 from do_switch()).
> @@ -456,7 +442,7 @@ void i915_gem_context_fini(struct drm_device *dev)
> * to offset the do_switch part, so that i915_gem_context_unreference()
> * can then free the base object correctly. */
> WARN_ON(!dev_priv->ring[RCS].last_context);
> - if (dev_priv->ring[RCS].last_context == dctx) {
> + if (dctx->obj && dev_priv->ring[RCS].last_context == dctx) {
> /* Fake switch to NULL context */
> WARN_ON(dctx->obj->active);
> i915_gem_object_ggtt_unpin(dctx->obj);
> @@ -466,6 +452,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>
> for (i = 0; i < I915_NUM_RINGS; i++) {
> struct intel_ring_buffer *ring = &dev_priv->ring[i];
> +
> if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> continue;
>
> @@ -478,7 +465,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>
> i915_gem_object_ggtt_unpin(dctx->obj);
> i915_gem_context_unreference(dctx);
> - dev_priv->mm.aliasing_ppgtt = NULL;
> }
>
> int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> @@ -486,9 +472,6 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> struct intel_ring_buffer *ring;
> int ret, i;
>
> - if (!HAS_HW_CONTEXTS(dev_priv->dev))
> - return 0;
> -
> /* This is the only place the aliasing PPGTT gets enabled, which means
> * it has to happen before we bail on reset */
> if (dev_priv->mm.aliasing_ppgtt) {
> @@ -503,7 +486,7 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> BUG_ON(!dev_priv->ring[RCS].default_context);
>
> for_each_ring(ring, dev_priv, i) {
> - ret = do_switch(ring, ring->default_context);
> + ret = i915_switch_context(ring, ring->default_context);
> if (ret)
> return ret;
> }
> @@ -526,19 +509,6 @@ static int context_idr_cleanup(int id, void *p, void *data)
> int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> -
> - if (!HAS_HW_CONTEXTS(dev)) {
> - /* Cheat for hang stats */
> - file_priv->private_default_ctx =
> - kzalloc(sizeof(struct i915_hw_context), GFP_KERNEL);
> -
> - if (file_priv->private_default_ctx == NULL)
> - return -ENOMEM;
> -
> - file_priv->private_default_ctx->vm = &dev_priv->gtt.base;
> - return 0;
> - }
>
> idr_init(&file_priv->context_idr);
>
> @@ -559,14 +529,13 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
>
> - if (!HAS_HW_CONTEXTS(dev)) {
> - kfree(file_priv->private_default_ctx);
> + if (IS_ERR_OR_NULL(file_priv->private_default_ctx))
> return;
> - }
>
> idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
> - i915_gem_context_unreference(file_priv->private_default_ctx);
> idr_destroy(&file_priv->context_idr);
> +
> + i915_gem_context_unreference(file_priv->private_default_ctx);
> }
>
> struct i915_hw_context *
> @@ -574,9 +543,6 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> {
> struct i915_hw_context *ctx;
>
> - if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
> - return file_priv->private_default_ctx;
> -
> ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> if (!ctx)
> return ERR_PTR(-ENOENT);
> @@ -758,7 +724,6 @@ unpin_out:
> /**
> * i915_switch_context() - perform a GPU context switch.
> * @ring: ring for which we'll execute the context switch
> - * @file_priv: file_priv associated with the context, may be NULL
> * @to: the context to switch to
> *
> * The context life cycle is simple. The context refcount is incremented and
> @@ -767,18 +732,20 @@ unpin_out:
> * object while letting the normal object tracking destroy the backing BO.
> */
> int i915_switch_context(struct intel_ring_buffer *ring,
> - struct drm_file *file,
> struct i915_hw_context *to)
> {
> struct drm_i915_private *dev_priv = ring->dev->dev_private;
>
> WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>
> - BUG_ON(file && to == NULL);
> -
> - /* We have the fake context */
> - if (!HAS_HW_CONTEXTS(ring->dev)) {
> - ring->last_context = to;
> + if (to->obj == NULL) { /* We have the fake context */
> + if (to != ring->last_context) {
> + if (to)
> + i915_gem_context_reference(to);
^^
Not needed as you have already oopsed earlier if it doesnt exist.
-Mika
> + if (ring->last_context)
> + i915_gem_context_unreference(ring->last_context);
> + ring->last_context = to;
> + }
> return 0;
> }
>
> @@ -793,7 +760,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> struct i915_hw_context *ctx;
> int ret;
>
> - if (!HAS_HW_CONTEXTS(dev))
> + if (to_i915(dev)->hw_context_size == 0)
> return -ENODEV;
>
> ret = i915_mutex_lock_interruptible(dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 7447160155a3..2c9d9cbaf653 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1221,7 +1221,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> if (ret)
> goto err;
>
> - ret = i915_switch_context(ring, file, ctx);
> + ret = i915_switch_context(ring, ctx);
> if (ret)
> goto err;
>
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: Always use kref tracking for all contexts.
2014-04-03 10:36 ` Mika Kuoppala
@ 2014-04-03 10:44 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2014-04-03 10:44 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx, Ben Widawsky
On Thu, Apr 03, 2014 at 01:36:25PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > + if (to->obj == NULL) { /* We have the fake context */
> > + if (to != ring->last_context) {
> > + if (to)
> > + i915_gem_context_reference(to);
> ^^
> Not needed as you have already oopsed earlier if it doesnt exist.
Force of habit.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Always use kref tracking for all contexts.
2014-04-03 7:06 [PATCH] drm/i915: Always use kref tracking for all contexts Chris Wilson
2014-04-03 10:36 ` Mika Kuoppala
@ 2014-04-04 4:44 ` Ben Widawsky
2014-04-04 7:07 ` Chris Wilson
2014-04-04 13:40 ` Chris Wilson
2 siblings, 1 reply; 10+ messages in thread
From: Ben Widawsky @ 2014-04-04 4:44 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, Ben Widawsky
On Thu, Apr 03, 2014 at 08:06:30AM +0100, Chris Wilson wrote:
> If we always initialize kref for the context, even if we are using fake
> contexts for hangstats when there is no hw support, we can forgo the
> dance to dereference the ctx->obj and inspect whether we are permitted
> to use kref inside i915_gem_context_reference() and _unreference().
>
> My ulterior motive here is to improve the debugging of a use-after-free
> of ctx->obj. This patch avoids the dereference here and instead forces
> the assertion checks associated with kref.
>
> v2: Refactor the fake contexts to being even more like the real
> contexts, so that there is much less duplicated and special case code.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76671
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: lu hua <huax.lu@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 8 +-
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_context.c | 175 ++++++++++++-----------------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> 4 files changed, 76 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0e2de9d54b8..97bf793f6552 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2275,20 +2275,18 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> int i915_gem_context_enable(struct drm_i915_private *dev_priv);
> void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> int i915_switch_context(struct intel_ring_buffer *ring,
> - struct drm_file *file, struct i915_hw_context *to);
> + struct i915_hw_context *to);
> struct i915_hw_context *
> i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
> void i915_gem_context_free(struct kref *ctx_ref);
> static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
> {
> - if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> - kref_get(&ctx->ref);
> + kref_get(&ctx->ref);
> }
>
> static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
> {
> - if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> - kref_put(&ctx->ref, i915_gem_context_free);
> + kref_put(&ctx->ref, i915_gem_context_free);
> }
>
> static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 221f6ab3a498..0ff65666c5f7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2819,7 +2819,7 @@ int i915_gpu_idle(struct drm_device *dev)
>
> /* Flush everything onto the inactive list. */
> for_each_ring(ring, dev_priv, i) {
> - ret = i915_switch_context(ring, NULL, ring->default_context);
> + ret = i915_switch_context(ring, ring->default_context);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index edbad2787d65..1439cc6bd439 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -96,9 +96,6 @@
> #define GEN6_CONTEXT_ALIGN (64<<10)
> #define GEN7_CONTEXT_ALIGN 4096
>
> -static int do_switch(struct intel_ring_buffer *ring,
> - struct i915_hw_context *to);
> -
> static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
> {
> struct drm_device *dev = ppgtt->base.dev;
> @@ -185,13 +182,15 @@ void i915_gem_context_free(struct kref *ctx_ref)
> typeof(*ctx), ref);
> struct i915_hw_ppgtt *ppgtt = NULL;
>
> - /* We refcount even the aliasing PPGTT to keep the code symmetric */
> - if (USES_PPGTT(ctx->obj->base.dev))
> - ppgtt = ctx_to_ppgtt(ctx);
> + if (ctx->obj) {
> + /* We refcount even the aliasing PPGTT to keep the code symmetric */
> + if (USES_PPGTT(ctx->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);
> + /* 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);
> + }
>
> if (ppgtt)
> kref_put(&ppgtt->ref, ppgtt_release);
> @@ -232,32 +231,32 @@ __create_hw_context(struct drm_device *dev,
> return ERR_PTR(-ENOMEM);
>
> kref_init(&ctx->ref);
> - ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> - INIT_LIST_HEAD(&ctx->link);
> - if (ctx->obj == NULL) {
> - kfree(ctx);
> - DRM_DEBUG_DRIVER("Context object allocated failed\n");
> - return ERR_PTR(-ENOMEM);
> - }
> + list_add_tail(&ctx->link, &dev_priv->context_list);
>
> - if (INTEL_INFO(dev)->gen >= 7) {
> - ret = i915_gem_object_set_cache_level(ctx->obj,
> - I915_CACHE_L3_LLC);
> - /* Failure shouldn't ever happen this early */
> - if (WARN_ON(ret))
> + if (dev_priv->hw_context_size) {
> + ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> + if (ctx->obj == NULL) {
> + ret = -ENOMEM;
> goto err_out;
> - }
> + }
>
> - list_add_tail(&ctx->link, &dev_priv->context_list);
> + if (INTEL_INFO(dev)->gen >= 7) {
> + ret = i915_gem_object_set_cache_level(ctx->obj,
> + I915_CACHE_L3_LLC);
> + /* Failure shouldn't ever happen this early */
> + if (WARN_ON(ret))
> + goto err_out;
> + }
> + }
>
> /* Default context will never have a file_priv */
> - if (file_priv == NULL)
> - return ctx;
> -
> - ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID, 0,
> - GFP_KERNEL);
> - if (ret < 0)
> - goto err_out;
> + if (file_priv != NULL) {
> + ret = idr_alloc(&file_priv->context_idr, ctx,
> + DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
> + if (ret < 0)
> + goto err_out;
> + } else
> + ret = DEFAULT_CONTEXT_ID;
>
> ctx->file_priv = file_priv;
> ctx->id = ret;
> @@ -294,7 +293,7 @@ i915_gem_create_context(struct drm_device *dev,
> if (IS_ERR(ctx))
> return ctx;
>
> - if (is_global_default_ctx) {
> + if (is_global_default_ctx && ctx->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
> @@ -342,7 +341,7 @@ i915_gem_create_context(struct drm_device *dev,
> return ctx;
>
> err_unpin:
> - if (is_global_default_ctx)
> + if (is_global_default_ctx && ctx->obj)
> i915_gem_object_ggtt_unpin(ctx->obj);
> err_destroy:
> i915_gem_context_unreference(ctx);
> @@ -352,16 +351,14 @@ err_destroy:
> void i915_gem_context_reset(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_ring_buffer *ring;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return;
> -
> /* Prevent the hardware from restoring the last context (which hung) on
> * the next switch */
> for (i = 0; i < I915_NUM_RINGS; i++) {
> struct i915_hw_context *dctx;
> + struct intel_ring_buffer *ring;
> +
> if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> continue;
>
> @@ -377,7 +374,7 @@ void i915_gem_context_reset(struct drm_device *dev)
> if (ring->last_context == dctx)
> continue;
>
> - if (i == RCS) {
> + if (dctx->obj && i == RCS) {
> WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
> get_context_alignment(dev), 0));
> /* Fake a finish/inactive */
I am beginning to feel ambivalent now as to whether or not the
simplicity gained by making fake contexts out weighs. For example,
before, last_context was always equal to dctx without HW contexts, which
made places like the above nice and simple.
> @@ -394,44 +391,35 @@ void i915_gem_context_reset(struct drm_device *dev)
> int i915_gem_context_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_ring_buffer *ring;
> + struct i915_hw_context *ctx;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return 0;
> -
> /* Init should only be called once per module load. Eventually the
> * restriction on the context_disabled check can be loosened. */
> if (WARN_ON(dev_priv->ring[RCS].default_context))
> return 0;
>
> - dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> -
> - if (dev_priv->hw_context_size > (1<<20)) {
> - DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
> - return -E2BIG;
> + if (HAS_HW_CONTEXTS(dev)) {
> + dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> + if (dev_priv->hw_context_size > (1<<20)) {
> + DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
> + dev_priv->hw_context_size);
> + dev_priv->hw_context_size = 0;
> + }
> }
>
> - dev_priv->ring[RCS].default_context =
> - i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> -
> - if (IS_ERR_OR_NULL(dev_priv->ring[RCS].default_context)) {
> - DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %ld\n",
> - PTR_ERR(dev_priv->ring[RCS].default_context));
> - return PTR_ERR(dev_priv->ring[RCS].default_context);
> + ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> + if (IS_ERR(ctx)) {
> + DRM_ERROR("Failed to create default global context (error %ld)\n",
> + PTR_ERR(ctx));
> + return PTR_ERR(ctx);
> }
>
> - for (i = RCS + 1; i < I915_NUM_RINGS; i++) {
> - if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> - continue;
> + /* NB: RCS will hold a ref for all rings */
> + for (i = 0; i < I915_NUM_RINGS; i++)
> + dev_priv->ring[i].default_context = ctx;
>
> - ring = &dev_priv->ring[i];
> -
> - /* NB: RCS will hold a ref for all rings */
> - ring->default_context = dev_priv->ring[RCS].default_context;
> - }
> -
> - DRM_DEBUG_DRIVER("HW context support initialized\n");
> + DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
> return 0;
Some nice cleanups here.
> }
>
> @@ -441,13 +429,11 @@ void i915_gem_context_fini(struct drm_device *dev)
> struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return;
> -
> - /* 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. */
> - intel_gpu_reset(dev);
> + if (dev_priv->hw_context_size)
> + /* 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. */
> + intel_gpu_reset(dev);
Perhaps it's time to
static inline bool i915_gem_hw_contexts_enabled(dev_priv)
{
return dev_priv->hw_context_size == 0;
}
or #define USES_HW_CONTEXTS?
>
> /* When default context is created and switched to, base object refcount
> * will be 2 (+1 from object creation and +1 from do_switch()).
> @@ -456,7 +442,7 @@ void i915_gem_context_fini(struct drm_device *dev)
> * to offset the do_switch part, so that i915_gem_context_unreference()
> * can then free the base object correctly. */
> WARN_ON(!dev_priv->ring[RCS].last_context);
> - if (dev_priv->ring[RCS].last_context == dctx) {
> + if (dctx->obj && dev_priv->ring[RCS].last_context == dctx) {
> /* Fake switch to NULL context */
> WARN_ON(dctx->obj->active);
> i915_gem_object_ggtt_unpin(dctx->obj);
> @@ -466,6 +452,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>
> for (i = 0; i < I915_NUM_RINGS; i++) {
> struct intel_ring_buffer *ring = &dev_priv->ring[i];
> +
> if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> continue;
>
> @@ -478,7 +465,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>
> i915_gem_object_ggtt_unpin(dctx->obj);
> i915_gem_context_unreference(dctx);
> - dev_priv->mm.aliasing_ppgtt = NULL;
> }
>
> int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> @@ -486,9 +472,6 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> struct intel_ring_buffer *ring;
> int ret, i;
>
> - if (!HAS_HW_CONTEXTS(dev_priv->dev))
> - return 0;
> -
> /* This is the only place the aliasing PPGTT gets enabled, which means
> * it has to happen before we bail on reset */
> if (dev_priv->mm.aliasing_ppgtt) {
> @@ -503,7 +486,7 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> BUG_ON(!dev_priv->ring[RCS].default_context);
>
> for_each_ring(ring, dev_priv, i) {
> - ret = do_switch(ring, ring->default_context);
> + ret = i915_switch_context(ring, ring->default_context);
> if (ret)
> return ret;
> }
> @@ -526,19 +509,6 @@ static int context_idr_cleanup(int id, void *p, void *data)
> int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> -
> - if (!HAS_HW_CONTEXTS(dev)) {
> - /* Cheat for hang stats */
> - file_priv->private_default_ctx =
> - kzalloc(sizeof(struct i915_hw_context), GFP_KERNEL);
> -
> - if (file_priv->private_default_ctx == NULL)
> - return -ENOMEM;
> -
> - file_priv->private_default_ctx->vm = &dev_priv->gtt.base;
> - return 0;
> - }
>
> idr_init(&file_priv->context_idr);
>
> @@ -559,14 +529,13 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
>
> - if (!HAS_HW_CONTEXTS(dev)) {
> - kfree(file_priv->private_default_ctx);
> + if (IS_ERR_OR_NULL(file_priv->private_default_ctx))
> return;
I am not convinced. This should be
BUG_ON(!file_priv->private_default_ctx). They don't get back an fd is
context_open failed.
> - }
>
> idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
> - i915_gem_context_unreference(file_priv->private_default_ctx);
> idr_destroy(&file_priv->context_idr);
> +
> + i915_gem_context_unreference(file_priv->private_default_ctx);
> }
>
> struct i915_hw_context *
> @@ -574,9 +543,6 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> {
> struct i915_hw_context *ctx;
>
> - if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
> - return file_priv->private_default_ctx;
> -
> ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> if (!ctx)
> return ERR_PTR(-ENOENT);
> @@ -758,7 +724,6 @@ unpin_out:
> /**
> * i915_switch_context() - perform a GPU context switch.
> * @ring: ring for which we'll execute the context switch
> - * @file_priv: file_priv associated with the context, may be NULL
> * @to: the context to switch to
> *
> * The context life cycle is simple. The context refcount is incremented and
> @@ -767,18 +732,20 @@ unpin_out:
> * object while letting the normal object tracking destroy the backing BO.
> */
> int i915_switch_context(struct intel_ring_buffer *ring,
> - struct drm_file *file,
> struct i915_hw_context *to)
> {
> struct drm_i915_private *dev_priv = ring->dev->dev_private;
>
> WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>
> - BUG_ON(file && to == NULL);
> -
> - /* We have the fake context */
> - if (!HAS_HW_CONTEXTS(ring->dev)) {
> - ring->last_context = to;
> + if (to->obj == NULL) { /* We have the fake context */
> + if (to != ring->last_context) {
> + if (to)
> + i915_gem_context_reference(to);
> + if (ring->last_context)
> + i915_gem_context_unreference(ring->last_context);
> + ring->last_context = to;
> + }
> return 0;
Here's some more ambivalence. Adding complexity to i915_switch_context
IMHO outweighs the gains elsewhere.
> }
>
> @@ -793,7 +760,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> struct i915_hw_context *ctx;
> int ret;
>
> - if (!HAS_HW_CONTEXTS(dev))
> + if (to_i915(dev)->hw_context_size == 0)
> return -ENODEV;
>
> ret = i915_mutex_lock_interruptible(dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 7447160155a3..2c9d9cbaf653 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1221,7 +1221,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> if (ret)
> goto err;
>
> - ret = i915_switch_context(ring, file, ctx);
> + ret = i915_switch_context(ring, ctx);
> if (ret)
> goto err;
>
> --
> 1.9.1
>
Other than Mika's comment about the unnecessary "if (to)" and mine about
what I think is a false condition for context_close() it looks okay to
me. I am a bit disappointed with the two spots I pointed out ambivalence,
but I do think it's a net-win.
So with the fix to my comment, and/or an explanation as to why I am an
idiot:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: Always use kref tracking for all contexts.
2014-04-04 4:44 ` Ben Widawsky
@ 2014-04-04 7:07 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2014-04-04 7:07 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, Ben Widawsky
On Thu, Apr 03, 2014 at 09:44:43PM -0700, Ben Widawsky wrote:
> On Thu, Apr 03, 2014 at 08:06:30AM +0100, Chris Wilson wrote:
> > @@ -377,7 +374,7 @@ void i915_gem_context_reset(struct drm_device *dev)
> > if (ring->last_context == dctx)
> > continue;
> >
> > - if (i == RCS) {
> > + if (dctx->obj && i == RCS) {
> > WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
> > get_context_alignment(dev), 0));
> > /* Fake a finish/inactive */
>
> I am beginning to feel ambivalent now as to whether or not the
> simplicity gained by making fake contexts out weighs. For example,
> before, last_context was always equal to dctx without HW contexts, which
> made places like the above nice and simple.
Apart from that we use per-file defaults for hangstats, so this should
not be true today. If you look at the code after the patch, it does make
more sense I think - contexts always exists for bookkeeping, sometimes
they have hardware blobs as well.
> > @@ -559,14 +529,13 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> > {
> > struct drm_i915_file_private *file_priv = file->driver_priv;
> >
> > - if (!HAS_HW_CONTEXTS(dev)) {
> > - kfree(file_priv->private_default_ctx);
> > + if (IS_ERR_OR_NULL(file_priv->private_default_ctx))
> > return;
>
> I am not convinced. This should be
> BUG_ON(!file_priv->private_default_ctx). They don't get back an fd is
> context_open failed.
When I started, it wasn't a false condition, iirc. But now it is.
> > int i915_switch_context(struct intel_ring_buffer *ring,
> > - struct drm_file *file,
> > struct i915_hw_context *to)
> > {
> > struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >
> > WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> >
> > - BUG_ON(file && to == NULL);
> > -
> > - /* We have the fake context */
> > - if (!HAS_HW_CONTEXTS(ring->dev)) {
> > - ring->last_context = to;
> > + if (to->obj == NULL) { /* We have the fake context */
> > + if (to != ring->last_context) {
> > + if (to)
> > + i915_gem_context_reference(to);
> > + if (ring->last_context)
> > + i915_gem_context_unreference(ring->last_context);
> > + ring->last_context = to;
> > + }
> > return 0;
>
> Here's some more ambivalence. Adding complexity to i915_switch_context
> IMHO outweighs the gains elsewhere.
Apart from we have bugs in the current code where context (even fake
context) lifetimes are not tracked correctly, so using kref around each
assignment seems sensible.
> Other than Mika's comment about the unnecessary "if (to)" and mine about
> what I think is a false condition for context_close() it looks okay to
> me. I am a bit disappointed with the two spots I pointed out ambivalence,
> but I do think it's a net-win.
I can apply some more synatic sugar and see how you feel then.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm/i915: Always use kref tracking for all contexts.
2014-04-03 7:06 [PATCH] drm/i915: Always use kref tracking for all contexts Chris Wilson
2014-04-03 10:36 ` Mika Kuoppala
2014-04-04 4:44 ` Ben Widawsky
@ 2014-04-04 13:40 ` Chris Wilson
2014-04-05 19:18 ` Ben Widawsky
2014-04-09 8:07 ` Chris Wilson
2 siblings, 2 replies; 10+ messages in thread
From: Chris Wilson @ 2014-04-04 13:40 UTC (permalink / raw)
To: intel-gfx; +Cc: Mika Kuoppala, Ben Widawsky
If we always initialize kref for the context, even if we are using fake
contexts for hangstats when there is no hw support, we can forgo the
dance to dereference the ctx->obj and inspect whether we are permitted
to use kref inside i915_gem_context_reference() and _unreference().
My ulterior motive here is to improve the debugging of a use-after-free
of ctx->obj. This patch avoids the dereference here and instead forces
the assertion checks associated with kref.
v2: Refactor the fake contexts to being even more like the real
contexts, so that there is much less duplicated and special case code.
v3: Tweaks
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76671
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: lu hua <huax.lu@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 8 +-
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_gem_context.c | 212 ++++++++++++-----------------
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
4 files changed, 89 insertions(+), 135 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 533bd8f6a5b1..0557bd92b26b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2276,20 +2276,18 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
int i915_gem_context_enable(struct drm_i915_private *dev_priv);
void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
int i915_switch_context(struct intel_ring_buffer *ring,
- struct drm_file *file, struct i915_hw_context *to);
+ struct i915_hw_context *to);
struct i915_hw_context *
i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
void i915_gem_context_free(struct kref *ctx_ref);
static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
{
- if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
- kref_get(&ctx->ref);
+ kref_get(&ctx->ref);
}
static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
{
- if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
- kref_put(&ctx->ref, i915_gem_context_free);
+ kref_put(&ctx->ref, i915_gem_context_free);
}
static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 221f6ab3a498..0ff65666c5f7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2819,7 +2819,7 @@ int i915_gpu_idle(struct drm_device *dev)
/* Flush everything onto the inactive list. */
for_each_ring(ring, dev_priv, i) {
- ret = i915_switch_context(ring, NULL, ring->default_context);
+ ret = i915_switch_context(ring, ring->default_context);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 8827892a099d..07b0b792a46c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -96,9 +96,6 @@
#define GEN6_CONTEXT_ALIGN (64<<10)
#define GEN7_CONTEXT_ALIGN 4096
-static int do_switch(struct intel_ring_buffer *ring,
- struct i915_hw_context *to);
-
static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
{
struct drm_device *dev = ppgtt->base.dev;
@@ -185,13 +182,15 @@ void i915_gem_context_free(struct kref *ctx_ref)
typeof(*ctx), ref);
struct i915_hw_ppgtt *ppgtt = NULL;
- /* We refcount even the aliasing PPGTT to keep the code symmetric */
- if (USES_PPGTT(ctx->obj->base.dev))
- ppgtt = ctx_to_ppgtt(ctx);
+ if (ctx->obj) {
+ /* We refcount even the aliasing PPGTT to keep the code symmetric */
+ if (USES_PPGTT(ctx->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);
+ /* 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);
+ }
if (ppgtt)
kref_put(&ppgtt->ref, ppgtt_release);
@@ -232,32 +231,32 @@ __create_hw_context(struct drm_device *dev,
return ERR_PTR(-ENOMEM);
kref_init(&ctx->ref);
- ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
- INIT_LIST_HEAD(&ctx->link);
- if (ctx->obj == NULL) {
- kfree(ctx);
- DRM_DEBUG_DRIVER("Context object allocated failed\n");
- return ERR_PTR(-ENOMEM);
- }
+ list_add_tail(&ctx->link, &dev_priv->context_list);
- if (INTEL_INFO(dev)->gen >= 7) {
- ret = i915_gem_object_set_cache_level(ctx->obj,
- I915_CACHE_L3_LLC);
- /* Failure shouldn't ever happen this early */
- if (WARN_ON(ret))
+ if (dev_priv->hw_context_size) {
+ ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
+ if (ctx->obj == NULL) {
+ ret = -ENOMEM;
goto err_out;
- }
+ }
- list_add_tail(&ctx->link, &dev_priv->context_list);
+ if (INTEL_INFO(dev)->gen >= 7) {
+ ret = i915_gem_object_set_cache_level(ctx->obj,
+ I915_CACHE_L3_LLC);
+ /* Failure shouldn't ever happen this early */
+ if (WARN_ON(ret))
+ goto err_out;
+ }
+ }
/* Default context will never have a file_priv */
- if (file_priv == NULL)
- return ctx;
-
- ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID, 0,
- GFP_KERNEL);
- if (ret < 0)
- goto err_out;
+ if (file_priv != NULL) {
+ ret = idr_alloc(&file_priv->context_idr, ctx,
+ DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
+ if (ret < 0)
+ goto err_out;
+ } else
+ ret = DEFAULT_CONTEXT_ID;
ctx->file_priv = file_priv;
ctx->id = ret;
@@ -294,7 +293,7 @@ i915_gem_create_context(struct drm_device *dev,
if (IS_ERR(ctx))
return ctx;
- if (is_global_default_ctx) {
+ if (is_global_default_ctx && ctx->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
@@ -342,7 +341,7 @@ i915_gem_create_context(struct drm_device *dev,
return ctx;
err_unpin:
- if (is_global_default_ctx)
+ if (is_global_default_ctx && ctx->obj)
i915_gem_object_ggtt_unpin(ctx->obj);
err_destroy:
i915_gem_context_unreference(ctx);
@@ -352,32 +351,22 @@ err_destroy:
void i915_gem_context_reset(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_ring_buffer *ring;
int i;
- if (!HAS_HW_CONTEXTS(dev))
- return;
-
/* Prevent the hardware from restoring the last context (which hung) on
* the next switch */
for (i = 0; i < I915_NUM_RINGS; i++) {
- struct i915_hw_context *dctx;
- if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
- continue;
+ struct intel_ring_buffer *ring = &dev_priv->ring[i];
+ struct i915_hw_context *dctx = ring->default_context;
/* Do a fake switch to the default context */
- ring = &dev_priv->ring[i];
- dctx = ring->default_context;
- if (WARN_ON(!dctx))
+ if (ring->last_context == dctx)
continue;
if (!ring->last_context)
continue;
- if (ring->last_context == dctx)
- continue;
-
- if (i == RCS) {
+ if (dctx->obj && i == RCS) {
WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
get_context_alignment(dev), 0));
/* Fake a finish/inactive */
@@ -394,44 +383,35 @@ void i915_gem_context_reset(struct drm_device *dev)
int i915_gem_context_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_ring_buffer *ring;
+ struct i915_hw_context *ctx;
int i;
- if (!HAS_HW_CONTEXTS(dev))
- return 0;
-
/* Init should only be called once per module load. Eventually the
* restriction on the context_disabled check can be loosened. */
if (WARN_ON(dev_priv->ring[RCS].default_context))
return 0;
- dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
-
- if (dev_priv->hw_context_size > (1<<20)) {
- DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
- return -E2BIG;
+ if (HAS_HW_CONTEXTS(dev)) {
+ dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
+ if (dev_priv->hw_context_size > (1<<20)) {
+ DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
+ dev_priv->hw_context_size);
+ dev_priv->hw_context_size = 0;
+ }
}
- dev_priv->ring[RCS].default_context =
- i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
-
- if (IS_ERR_OR_NULL(dev_priv->ring[RCS].default_context)) {
- DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %ld\n",
- PTR_ERR(dev_priv->ring[RCS].default_context));
- return PTR_ERR(dev_priv->ring[RCS].default_context);
+ ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
+ if (IS_ERR(ctx)) {
+ DRM_ERROR("Failed to create default global context (error %ld)\n",
+ PTR_ERR(ctx));
+ return PTR_ERR(ctx);
}
- for (i = RCS + 1; i < I915_NUM_RINGS; i++) {
- if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
- continue;
-
- ring = &dev_priv->ring[i];
+ /* NB: RCS will hold a ref for all rings */
+ for (i = 0; i < I915_NUM_RINGS; i++)
+ dev_priv->ring[i].default_context = ctx;
- /* NB: RCS will hold a ref for all rings */
- ring->default_context = dev_priv->ring[RCS].default_context;
- }
-
- DRM_DEBUG_DRIVER("HW context support initialized\n");
+ DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
return 0;
}
@@ -441,33 +421,30 @@ void i915_gem_context_fini(struct drm_device *dev)
struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
int i;
- if (!HAS_HW_CONTEXTS(dev))
- return;
-
- /* 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. */
- intel_gpu_reset(dev);
-
- /* When default context is created and switched to, base object refcount
- * will be 2 (+1 from object creation and +1 from do_switch()).
- * i915_gem_context_fini() will be called after gpu_idle() has switched
- * to default context. So we need to unreference the base object once
- * to offset the do_switch part, so that i915_gem_context_unreference()
- * can then free the base object correctly. */
- 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);
- i915_gem_context_unreference(dctx);
- dev_priv->ring[RCS].last_context = NULL;
+ if (dctx->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. */
+ intel_gpu_reset(dev);
+
+ /* When default context is created and switched to, base object refcount
+ * will be 2 (+1 from object creation and +1 from do_switch()).
+ * i915_gem_context_fini() will be called after gpu_idle() has switched
+ * to default context. So we need to unreference the base object once
+ * to offset the do_switch part, so that i915_gem_context_unreference()
+ * can then free the base object correctly. */
+ 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);
+ i915_gem_context_unreference(dctx);
+ dev_priv->ring[RCS].last_context = NULL;
+ }
}
for (i = 0; i < I915_NUM_RINGS; i++) {
struct intel_ring_buffer *ring = &dev_priv->ring[i];
- if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
- continue;
if (ring->last_context)
i915_gem_context_unreference(ring->last_context);
@@ -478,7 +455,6 @@ void i915_gem_context_fini(struct drm_device *dev)
i915_gem_object_ggtt_unpin(dctx->obj);
i915_gem_context_unreference(dctx);
- dev_priv->mm.aliasing_ppgtt = NULL;
}
int i915_gem_context_enable(struct drm_i915_private *dev_priv)
@@ -486,9 +462,6 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
struct intel_ring_buffer *ring;
int ret, i;
- if (!HAS_HW_CONTEXTS(dev_priv->dev))
- return 0;
-
/* This is the only place the aliasing PPGTT gets enabled, which means
* it has to happen before we bail on reset */
if (dev_priv->mm.aliasing_ppgtt) {
@@ -503,7 +476,7 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
BUG_ON(!dev_priv->ring[RCS].default_context);
for_each_ring(ring, dev_priv, i) {
- ret = do_switch(ring, ring->default_context);
+ ret = i915_switch_context(ring, ring->default_context);
if (ret)
return ret;
}
@@ -526,19 +499,6 @@ static int context_idr_cleanup(int id, void *p, void *data)
int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
{
struct drm_i915_file_private *file_priv = file->driver_priv;
- struct drm_i915_private *dev_priv = dev->dev_private;
-
- if (!HAS_HW_CONTEXTS(dev)) {
- /* Cheat for hang stats */
- file_priv->private_default_ctx =
- kzalloc(sizeof(struct i915_hw_context), GFP_KERNEL);
-
- if (file_priv->private_default_ctx == NULL)
- return -ENOMEM;
-
- file_priv->private_default_ctx->vm = &dev_priv->gtt.base;
- return 0;
- }
idr_init(&file_priv->context_idr);
@@ -559,14 +519,13 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
{
struct drm_i915_file_private *file_priv = file->driver_priv;
- if (!HAS_HW_CONTEXTS(dev)) {
- kfree(file_priv->private_default_ctx);
+ if (IS_ERR(file_priv->private_default_ctx))
return;
- }
idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
- i915_gem_context_unreference(file_priv->private_default_ctx);
idr_destroy(&file_priv->context_idr);
+
+ i915_gem_context_unreference(file_priv->private_default_ctx);
}
struct i915_hw_context *
@@ -574,9 +533,6 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
{
struct i915_hw_context *ctx;
- if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
- return file_priv->private_default_ctx;
-
ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
if (!ctx)
return ERR_PTR(-ENOENT);
@@ -758,7 +714,6 @@ unpin_out:
/**
* i915_switch_context() - perform a GPU context switch.
* @ring: ring for which we'll execute the context switch
- * @file_priv: file_priv associated with the context, may be NULL
* @to: the context to switch to
*
* The context life cycle is simple. The context refcount is incremented and
@@ -767,18 +722,19 @@ unpin_out:
* object while letting the normal object tracking destroy the backing BO.
*/
int i915_switch_context(struct intel_ring_buffer *ring,
- struct drm_file *file,
struct i915_hw_context *to)
{
struct drm_i915_private *dev_priv = ring->dev->dev_private;
WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
- BUG_ON(file && to == NULL);
-
- /* We have the fake context */
- if (!HAS_HW_CONTEXTS(ring->dev)) {
- ring->last_context = to;
+ if (to->obj == NULL) { /* We have the fake context */
+ if (to != ring->last_context) {
+ i915_gem_context_reference(to);
+ if (ring->last_context)
+ i915_gem_context_unreference(ring->last_context);
+ ring->last_context = to;
+ }
return 0;
}
@@ -793,7 +749,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
struct i915_hw_context *ctx;
int ret;
- if (!HAS_HW_CONTEXTS(dev))
+ if (to_i915(dev)->hw_context_size == 0)
return -ENODEV;
ret = i915_mutex_lock_interruptible(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 7447160155a3..2c9d9cbaf653 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1221,7 +1221,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
if (ret)
goto err;
- ret = i915_switch_context(ring, file, ctx);
+ ret = i915_switch_context(ring, ctx);
if (ret)
goto err;
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: Always use kref tracking for all contexts.
2014-04-04 13:40 ` Chris Wilson
@ 2014-04-05 19:18 ` Ben Widawsky
2014-04-05 20:14 ` Chris Wilson
2014-04-09 8:07 ` Chris Wilson
1 sibling, 1 reply; 10+ messages in thread
From: Ben Widawsky @ 2014-04-05 19:18 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, Ben Widawsky, Mika Kuoppala
On Fri, Apr 04, 2014 at 02:40:06PM +0100, Chris Wilson wrote:
> If we always initialize kref for the context, even if we are using fake
> contexts for hangstats when there is no hw support, we can forgo the
> dance to dereference the ctx->obj and inspect whether we are permitted
> to use kref inside i915_gem_context_reference() and _unreference().
>
> My ulterior motive here is to improve the debugging of a use-after-free
> of ctx->obj. This patch avoids the dereference here and instead forces
> the assertion checks associated with kref.
>
> v2: Refactor the fake contexts to being even more like the real
> contexts, so that there is much less duplicated and special case code.
>
> v3: Tweaks
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76671
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: lu hua <huax.lu@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 8 +-
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_context.c | 212 ++++++++++++-----------------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> 4 files changed, 89 insertions(+), 135 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 533bd8f6a5b1..0557bd92b26b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2276,20 +2276,18 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> int i915_gem_context_enable(struct drm_i915_private *dev_priv);
> void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> int i915_switch_context(struct intel_ring_buffer *ring,
> - struct drm_file *file, struct i915_hw_context *to);
> + struct i915_hw_context *to);
> struct i915_hw_context *
> i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
> void i915_gem_context_free(struct kref *ctx_ref);
> static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
> {
> - if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> - kref_get(&ctx->ref);
> + kref_get(&ctx->ref);
> }
>
> static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
> {
> - if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> - kref_put(&ctx->ref, i915_gem_context_free);
> + kref_put(&ctx->ref, i915_gem_context_free);
> }
>
> static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 221f6ab3a498..0ff65666c5f7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2819,7 +2819,7 @@ int i915_gpu_idle(struct drm_device *dev)
>
> /* Flush everything onto the inactive list. */
> for_each_ring(ring, dev_priv, i) {
> - ret = i915_switch_context(ring, NULL, ring->default_context);
> + ret = i915_switch_context(ring, ring->default_context);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 8827892a099d..07b0b792a46c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -96,9 +96,6 @@
> #define GEN6_CONTEXT_ALIGN (64<<10)
> #define GEN7_CONTEXT_ALIGN 4096
>
> -static int do_switch(struct intel_ring_buffer *ring,
> - struct i915_hw_context *to);
> -
> static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
> {
> struct drm_device *dev = ppgtt->base.dev;
> @@ -185,13 +182,15 @@ void i915_gem_context_free(struct kref *ctx_ref)
> typeof(*ctx), ref);
> struct i915_hw_ppgtt *ppgtt = NULL;
>
> - /* We refcount even the aliasing PPGTT to keep the code symmetric */
> - if (USES_PPGTT(ctx->obj->base.dev))
> - ppgtt = ctx_to_ppgtt(ctx);
> + if (ctx->obj) {
> + /* We refcount even the aliasing PPGTT to keep the code symmetric */
> + if (USES_PPGTT(ctx->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);
> + /* 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);
> + }
>
> if (ppgtt)
> kref_put(&ppgtt->ref, ppgtt_release);
> @@ -232,32 +231,32 @@ __create_hw_context(struct drm_device *dev,
> return ERR_PTR(-ENOMEM);
>
> kref_init(&ctx->ref);
> - ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> - INIT_LIST_HEAD(&ctx->link);
> - if (ctx->obj == NULL) {
> - kfree(ctx);
> - DRM_DEBUG_DRIVER("Context object allocated failed\n");
> - return ERR_PTR(-ENOMEM);
> - }
> + list_add_tail(&ctx->link, &dev_priv->context_list);
>
> - if (INTEL_INFO(dev)->gen >= 7) {
> - ret = i915_gem_object_set_cache_level(ctx->obj,
> - I915_CACHE_L3_LLC);
> - /* Failure shouldn't ever happen this early */
> - if (WARN_ON(ret))
> + if (dev_priv->hw_context_size) {
> + ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> + if (ctx->obj == NULL) {
> + ret = -ENOMEM;
> goto err_out;
> - }
> + }
>
> - list_add_tail(&ctx->link, &dev_priv->context_list);
> + if (INTEL_INFO(dev)->gen >= 7) {
> + ret = i915_gem_object_set_cache_level(ctx->obj,
> + I915_CACHE_L3_LLC);
> + /* Failure shouldn't ever happen this early */
> + if (WARN_ON(ret))
> + goto err_out;
> + }
> + }
>
> /* Default context will never have a file_priv */
> - if (file_priv == NULL)
> - return ctx;
> -
> - ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID, 0,
> - GFP_KERNEL);
> - if (ret < 0)
> - goto err_out;
> + if (file_priv != NULL) {
> + ret = idr_alloc(&file_priv->context_idr, ctx,
> + DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
> + if (ret < 0)
> + goto err_out;
> + } else
> + ret = DEFAULT_CONTEXT_ID;
>
> ctx->file_priv = file_priv;
> ctx->id = ret;
> @@ -294,7 +293,7 @@ i915_gem_create_context(struct drm_device *dev,
> if (IS_ERR(ctx))
> return ctx;
>
> - if (is_global_default_ctx) {
> + if (is_global_default_ctx && ctx->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
> @@ -342,7 +341,7 @@ i915_gem_create_context(struct drm_device *dev,
> return ctx;
>
> err_unpin:
> - if (is_global_default_ctx)
> + if (is_global_default_ctx && ctx->obj)
> i915_gem_object_ggtt_unpin(ctx->obj);
> err_destroy:
> i915_gem_context_unreference(ctx);
> @@ -352,32 +351,22 @@ err_destroy:
> void i915_gem_context_reset(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_ring_buffer *ring;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return;
> -
> /* Prevent the hardware from restoring the last context (which hung) on
> * the next switch */
> for (i = 0; i < I915_NUM_RINGS; i++) {
> - struct i915_hw_context *dctx;
> - if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> - continue;
> + struct intel_ring_buffer *ring = &dev_priv->ring[i];
> + struct i915_hw_context *dctx = ring->default_context;
>
> /* Do a fake switch to the default context */
> - ring = &dev_priv->ring[i];
> - dctx = ring->default_context;
> - if (WARN_ON(!dctx))
> + if (ring->last_context == dctx)
> continue;
>
> if (!ring->last_context)
> continue;
>
> - if (ring->last_context == dctx)
> - continue;
> -
> - if (i == RCS) {
> + if (dctx->obj && i == RCS) {
> WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
> get_context_alignment(dev), 0));
> /* Fake a finish/inactive */
> @@ -394,44 +383,35 @@ void i915_gem_context_reset(struct drm_device *dev)
> int i915_gem_context_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_ring_buffer *ring;
> + struct i915_hw_context *ctx;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return 0;
> -
> /* Init should only be called once per module load. Eventually the
> * restriction on the context_disabled check can be loosened. */
> if (WARN_ON(dev_priv->ring[RCS].default_context))
> return 0;
>
> - dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> -
> - if (dev_priv->hw_context_size > (1<<20)) {
> - DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
> - return -E2BIG;
> + if (HAS_HW_CONTEXTS(dev)) {
> + dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> + if (dev_priv->hw_context_size > (1<<20)) {
> + DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
> + dev_priv->hw_context_size);
> + dev_priv->hw_context_size = 0;
> + }
> }
>
> - dev_priv->ring[RCS].default_context =
> - i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> -
> - if (IS_ERR_OR_NULL(dev_priv->ring[RCS].default_context)) {
> - DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %ld\n",
> - PTR_ERR(dev_priv->ring[RCS].default_context));
> - return PTR_ERR(dev_priv->ring[RCS].default_context);
> + ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> + if (IS_ERR(ctx)) {
> + DRM_ERROR("Failed to create default global context (error %ld)\n",
> + PTR_ERR(ctx));
> + return PTR_ERR(ctx);
> }
>
> - for (i = RCS + 1; i < I915_NUM_RINGS; i++) {
> - if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> - continue;
> -
> - ring = &dev_priv->ring[i];
> + /* NB: RCS will hold a ref for all rings */
> + for (i = 0; i < I915_NUM_RINGS; i++)
> + dev_priv->ring[i].default_context = ctx;
>
> - /* NB: RCS will hold a ref for all rings */
> - ring->default_context = dev_priv->ring[RCS].default_context;
> - }
> -
> - DRM_DEBUG_DRIVER("HW context support initialized\n");
> + DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
> return 0;
> }
>
> @@ -441,33 +421,30 @@ void i915_gem_context_fini(struct drm_device *dev)
> struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return;
> -
> - /* 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. */
> - intel_gpu_reset(dev);
> -
> - /* When default context is created and switched to, base object refcount
> - * will be 2 (+1 from object creation and +1 from do_switch()).
> - * i915_gem_context_fini() will be called after gpu_idle() has switched
> - * to default context. So we need to unreference the base object once
> - * to offset the do_switch part, so that i915_gem_context_unreference()
> - * can then free the base object correctly. */
> - 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);
> - i915_gem_context_unreference(dctx);
> - dev_priv->ring[RCS].last_context = NULL;
> + if (dctx->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. */
> + intel_gpu_reset(dev);
> +
> + /* When default context is created and switched to, base object refcount
> + * will be 2 (+1 from object creation and +1 from do_switch()).
> + * i915_gem_context_fini() will be called after gpu_idle() has switched
> + * to default context. So we need to unreference the base object once
> + * to offset the do_switch part, so that i915_gem_context_unreference()
> + * can then free the base object correctly. */
> + 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);
> + i915_gem_context_unreference(dctx);
> + dev_priv->ring[RCS].last_context = NULL;
> + }
> }
>
> for (i = 0; i < I915_NUM_RINGS; i++) {
> struct intel_ring_buffer *ring = &dev_priv->ring[i];
> - if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> - continue;
>
> if (ring->last_context)
> i915_gem_context_unreference(ring->last_context);
> @@ -478,7 +455,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>
> i915_gem_object_ggtt_unpin(dctx->obj);
> i915_gem_context_unreference(dctx);
> - dev_priv->mm.aliasing_ppgtt = NULL;
> }
>
> int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> @@ -486,9 +462,6 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> struct intel_ring_buffer *ring;
> int ret, i;
>
> - if (!HAS_HW_CONTEXTS(dev_priv->dev))
> - return 0;
> -
> /* This is the only place the aliasing PPGTT gets enabled, which means
> * it has to happen before we bail on reset */
> if (dev_priv->mm.aliasing_ppgtt) {
> @@ -503,7 +476,7 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> BUG_ON(!dev_priv->ring[RCS].default_context);
>
> for_each_ring(ring, dev_priv, i) {
> - ret = do_switch(ring, ring->default_context);
> + ret = i915_switch_context(ring, ring->default_context);
> if (ret)
> return ret;
> }
> @@ -526,19 +499,6 @@ static int context_idr_cleanup(int id, void *p, void *data)
> int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> -
> - if (!HAS_HW_CONTEXTS(dev)) {
> - /* Cheat for hang stats */
> - file_priv->private_default_ctx =
> - kzalloc(sizeof(struct i915_hw_context), GFP_KERNEL);
> -
> - if (file_priv->private_default_ctx == NULL)
> - return -ENOMEM;
> -
> - file_priv->private_default_ctx->vm = &dev_priv->gtt.base;
> - return 0;
> - }
>
> idr_init(&file_priv->context_idr);
>
> @@ -559,14 +519,13 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
>
> - if (!HAS_HW_CONTEXTS(dev)) {
> - kfree(file_priv->private_default_ctx);
> + if (IS_ERR(file_priv->private_default_ctx))
> return;
> - }
I still don't get this. How can file_priv->private_default_ctx be
anything but a valid thing?
>
> idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
> - i915_gem_context_unreference(file_priv->private_default_ctx);
> idr_destroy(&file_priv->context_idr);
> +
> + i915_gem_context_unreference(file_priv->private_default_ctx);
> }
>
> struct i915_hw_context *
> @@ -574,9 +533,6 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> {
> struct i915_hw_context *ctx;
>
> - if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
> - return file_priv->private_default_ctx;
> -
> ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> if (!ctx)
> return ERR_PTR(-ENOENT);
> @@ -758,7 +714,6 @@ unpin_out:
> /**
> * i915_switch_context() - perform a GPU context switch.
> * @ring: ring for which we'll execute the context switch
> - * @file_priv: file_priv associated with the context, may be NULL
> * @to: the context to switch to
> *
> * The context life cycle is simple. The context refcount is incremented and
> @@ -767,18 +722,19 @@ unpin_out:
> * object while letting the normal object tracking destroy the backing BO.
> */
> int i915_switch_context(struct intel_ring_buffer *ring,
> - struct drm_file *file,
> struct i915_hw_context *to)
> {
> struct drm_i915_private *dev_priv = ring->dev->dev_private;
>
> WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>
> - BUG_ON(file && to == NULL);
> -
> - /* We have the fake context */
> - if (!HAS_HW_CONTEXTS(ring->dev)) {
> - ring->last_context = to;
> + if (to->obj == NULL) { /* We have the fake context */
> + if (to != ring->last_context) {
> + i915_gem_context_reference(to);
> + if (ring->last_context)
> + i915_gem_context_unreference(ring->last_context);
> + ring->last_context = to;
> + }
> return 0;
> }
>
> @@ -793,7 +749,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> struct i915_hw_context *ctx;
> int ret;
>
> - if (!HAS_HW_CONTEXTS(dev))
> + if (to_i915(dev)->hw_context_size == 0)
> return -ENODEV;
You didn't like the idea of USES_Hw_CONTEXTS, or hw_contexts_enabled()?
Just curious, I can live with this.
>
> ret = i915_mutex_lock_interruptible(dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 7447160155a3..2c9d9cbaf653 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1221,7 +1221,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> if (ret)
> goto err;
>
> - ret = i915_switch_context(ring, file, ctx);
> + ret = i915_switch_context(ring, ctx);
> if (ret)
> goto err;
>
Your ulterior motive is really the win IMO. I still can't spot anything
wrong, but I didn't look as hard this time around:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: Always use kref tracking for all contexts.
2014-04-05 19:18 ` Ben Widawsky
@ 2014-04-05 20:14 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2014-04-05 20:14 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, Ben Widawsky, Mika Kuoppala
On Sat, Apr 05, 2014 at 12:18:05PM -0700, Ben Widawsky wrote:
> On Fri, Apr 04, 2014 at 02:40:06PM +0100, Chris Wilson wrote:
> > @@ -559,14 +519,13 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> > {
> > struct drm_i915_file_private *file_priv = file->driver_priv;
> >
> > - if (!HAS_HW_CONTEXTS(dev)) {
> > - kfree(file_priv->private_default_ctx);
> > + if (IS_ERR(file_priv->private_default_ctx))
> > return;
> > - }
>
> I still don't get this. How can file_priv->private_default_ctx be
> anything but a valid thing?
I honestly didn't check to see if close was never called along an error
path, or if context open failing prevented the whole file open, so just
left in this guard in case we do set it to the error pointer during
open.
> > @@ -793,7 +749,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > struct i915_hw_context *ctx;
> > int ret;
> >
> > - if (!HAS_HW_CONTEXTS(dev))
> > + if (to_i915(dev)->hw_context_size == 0)
> > return -ENODEV;
>
> You didn't like the idea of USES_Hw_CONTEXTS, or hw_contexts_enabled()?
> Just curious, I can live with this.
It basically came down to this single instance where we wanted to use
hw_context_size as hw_contexts_enabled() so I wasn't too fussed. In
retrospect, it is exactly the sort of self-descriptive predicate I would
ask of others. Tsk, tsk, lazy me.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm/i915: Always use kref tracking for all contexts.
2014-04-04 13:40 ` Chris Wilson
2014-04-05 19:18 ` Ben Widawsky
@ 2014-04-09 8:07 ` Chris Wilson
2014-04-11 6:37 ` Ben Widawsky
1 sibling, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2014-04-09 8:07 UTC (permalink / raw)
To: intel-gfx; +Cc: Mika Kuoppala, Ben Widawsky
If we always initialize kref for the context, even if we are using fake
contexts for hangstats when there is no hw support, we can forgo the
dance to dereference the ctx->obj and inspect whether we are permitted
to use kref inside i915_gem_context_reference() and _unreference().
My ulterior motive here is to improve the debugging of a use-after-free
of ctx->obj. This patch avoids the dereference here and instead forces
the assertion checks associated with kref.
v2: Refactor the fake contexts to being even more like the real
contexts, so that there is much less duplicated and special case code.
v3: Tweaks.
v4: Tweaks, minor.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76671
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: lu hua <huax.lu@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 8 +-
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_gem_context.c | 234 ++++++++++++-----------------
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
4 files changed, 101 insertions(+), 145 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 533bd8f6a5b1..0557bd92b26b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2276,20 +2276,18 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
int i915_gem_context_enable(struct drm_i915_private *dev_priv);
void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
int i915_switch_context(struct intel_ring_buffer *ring,
- struct drm_file *file, struct i915_hw_context *to);
+ struct i915_hw_context *to);
struct i915_hw_context *
i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
void i915_gem_context_free(struct kref *ctx_ref);
static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
{
- if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
- kref_get(&ctx->ref);
+ kref_get(&ctx->ref);
}
static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
{
- if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
- kref_put(&ctx->ref, i915_gem_context_free);
+ kref_put(&ctx->ref, i915_gem_context_free);
}
static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index eb5cf077c626..4368437458fd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2819,7 +2819,7 @@ int i915_gpu_idle(struct drm_device *dev)
/* Flush everything onto the inactive list. */
for_each_ring(ring, dev_priv, i) {
- ret = i915_switch_context(ring, NULL, ring->default_context);
+ ret = i915_switch_context(ring, ring->default_context);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 30b355afb362..f77b4c126465 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -96,9 +96,6 @@
#define GEN6_CONTEXT_ALIGN (64<<10)
#define GEN7_CONTEXT_ALIGN 4096
-static int do_switch(struct intel_ring_buffer *ring,
- struct i915_hw_context *to);
-
static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
{
struct drm_device *dev = ppgtt->base.dev;
@@ -185,13 +182,15 @@ void i915_gem_context_free(struct kref *ctx_ref)
typeof(*ctx), ref);
struct i915_hw_ppgtt *ppgtt = NULL;
- /* We refcount even the aliasing PPGTT to keep the code symmetric */
- if (USES_PPGTT(ctx->obj->base.dev))
- ppgtt = ctx_to_ppgtt(ctx);
+ if (ctx->obj) {
+ /* We refcount even the aliasing PPGTT to keep the code symmetric */
+ if (USES_PPGTT(ctx->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);
+ /* 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);
+ }
if (ppgtt)
kref_put(&ppgtt->ref, ppgtt_release);
@@ -232,40 +231,40 @@ __create_hw_context(struct drm_device *dev,
return ERR_PTR(-ENOMEM);
kref_init(&ctx->ref);
- ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
- INIT_LIST_HEAD(&ctx->link);
- if (ctx->obj == NULL) {
- kfree(ctx);
- DRM_DEBUG_DRIVER("Context object allocated failed\n");
- return ERR_PTR(-ENOMEM);
- }
+ list_add_tail(&ctx->link, &dev_priv->context_list);
- /*
- * Try to make the context utilize L3 as well as LLC.
- *
- * On VLV we don't have L3 controls in the PTEs so we
- * shouldn't touch the cache level, especially as that
- * would make the object snooped which might have a
- * negative performance impact.
- */
- if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) {
- ret = i915_gem_object_set_cache_level(ctx->obj,
- I915_CACHE_L3_LLC);
- /* Failure shouldn't ever happen this early */
- if (WARN_ON(ret))
+ if (dev_priv->hw_context_size) {
+ ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
+ if (ctx->obj == NULL) {
+ ret = -ENOMEM;
goto err_out;
- }
+ }
- list_add_tail(&ctx->link, &dev_priv->context_list);
+ /*
+ * Try to make the context utilize L3 as well as LLC.
+ *
+ * On VLV we don't have L3 controls in the PTEs so we
+ * shouldn't touch the cache level, especially as that
+ * would make the object snooped which might have a
+ * negative performance impact.
+ */
+ if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) {
+ ret = i915_gem_object_set_cache_level(ctx->obj,
+ I915_CACHE_L3_LLC);
+ /* Failure shouldn't ever happen this early */
+ if (WARN_ON(ret))
+ goto err_out;
+ }
+ }
/* Default context will never have a file_priv */
- if (file_priv == NULL)
- return ctx;
-
- ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID, 0,
- GFP_KERNEL);
- if (ret < 0)
- goto err_out;
+ if (file_priv != NULL) {
+ ret = idr_alloc(&file_priv->context_idr, ctx,
+ DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
+ if (ret < 0)
+ goto err_out;
+ } else
+ ret = DEFAULT_CONTEXT_ID;
ctx->file_priv = file_priv;
ctx->id = ret;
@@ -302,7 +301,7 @@ i915_gem_create_context(struct drm_device *dev,
if (IS_ERR(ctx))
return ctx;
- if (is_global_default_ctx) {
+ if (is_global_default_ctx && ctx->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
@@ -350,7 +349,7 @@ i915_gem_create_context(struct drm_device *dev,
return ctx;
err_unpin:
- if (is_global_default_ctx)
+ if (is_global_default_ctx && ctx->obj)
i915_gem_object_ggtt_unpin(ctx->obj);
err_destroy:
i915_gem_context_unreference(ctx);
@@ -360,32 +359,22 @@ err_destroy:
void i915_gem_context_reset(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_ring_buffer *ring;
int i;
- if (!HAS_HW_CONTEXTS(dev))
- return;
-
/* Prevent the hardware from restoring the last context (which hung) on
* the next switch */
for (i = 0; i < I915_NUM_RINGS; i++) {
- struct i915_hw_context *dctx;
- if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
- continue;
+ struct intel_ring_buffer *ring = &dev_priv->ring[i];
+ struct i915_hw_context *dctx = ring->default_context;
/* Do a fake switch to the default context */
- ring = &dev_priv->ring[i];
- dctx = ring->default_context;
- if (WARN_ON(!dctx))
+ if (ring->last_context == dctx)
continue;
if (!ring->last_context)
continue;
- if (ring->last_context == dctx)
- continue;
-
- if (i == RCS) {
+ if (dctx->obj && i == RCS) {
WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
get_context_alignment(dev), 0));
/* Fake a finish/inactive */
@@ -402,44 +391,35 @@ void i915_gem_context_reset(struct drm_device *dev)
int i915_gem_context_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_ring_buffer *ring;
+ struct i915_hw_context *ctx;
int i;
- if (!HAS_HW_CONTEXTS(dev))
- return 0;
-
/* Init should only be called once per module load. Eventually the
* restriction on the context_disabled check can be loosened. */
if (WARN_ON(dev_priv->ring[RCS].default_context))
return 0;
- dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
-
- if (dev_priv->hw_context_size > (1<<20)) {
- DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
- return -E2BIG;
+ if (HAS_HW_CONTEXTS(dev)) {
+ dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
+ if (dev_priv->hw_context_size > (1<<20)) {
+ DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
+ dev_priv->hw_context_size);
+ dev_priv->hw_context_size = 0;
+ }
}
- dev_priv->ring[RCS].default_context =
- i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
-
- if (IS_ERR_OR_NULL(dev_priv->ring[RCS].default_context)) {
- DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %ld\n",
- PTR_ERR(dev_priv->ring[RCS].default_context));
- return PTR_ERR(dev_priv->ring[RCS].default_context);
+ ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
+ if (IS_ERR(ctx)) {
+ DRM_ERROR("Failed to create default global context (error %ld)\n",
+ PTR_ERR(ctx));
+ return PTR_ERR(ctx);
}
- for (i = RCS + 1; i < I915_NUM_RINGS; i++) {
- if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
- continue;
-
- ring = &dev_priv->ring[i];
-
- /* NB: RCS will hold a ref for all rings */
- ring->default_context = dev_priv->ring[RCS].default_context;
- }
+ /* NB: RCS will hold a ref for all rings */
+ for (i = 0; i < I915_NUM_RINGS; i++)
+ dev_priv->ring[i].default_context = ctx;
- DRM_DEBUG_DRIVER("HW context support initialized\n");
+ DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
return 0;
}
@@ -449,33 +429,30 @@ void i915_gem_context_fini(struct drm_device *dev)
struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
int i;
- if (!HAS_HW_CONTEXTS(dev))
- return;
-
- /* 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. */
- intel_gpu_reset(dev);
-
- /* When default context is created and switched to, base object refcount
- * will be 2 (+1 from object creation and +1 from do_switch()).
- * i915_gem_context_fini() will be called after gpu_idle() has switched
- * to default context. So we need to unreference the base object once
- * to offset the do_switch part, so that i915_gem_context_unreference()
- * can then free the base object correctly. */
- 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);
- i915_gem_context_unreference(dctx);
- dev_priv->ring[RCS].last_context = NULL;
+ if (dctx->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. */
+ intel_gpu_reset(dev);
+
+ /* When default context is created and switched to, base object refcount
+ * will be 2 (+1 from object creation and +1 from do_switch()).
+ * i915_gem_context_fini() will be called after gpu_idle() has switched
+ * to default context. So we need to unreference the base object once
+ * to offset the do_switch part, so that i915_gem_context_unreference()
+ * can then free the base object correctly. */
+ 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);
+ i915_gem_context_unreference(dctx);
+ dev_priv->ring[RCS].last_context = NULL;
+ }
}
for (i = 0; i < I915_NUM_RINGS; i++) {
struct intel_ring_buffer *ring = &dev_priv->ring[i];
- if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
- continue;
if (ring->last_context)
i915_gem_context_unreference(ring->last_context);
@@ -486,7 +463,6 @@ void i915_gem_context_fini(struct drm_device *dev)
i915_gem_object_ggtt_unpin(dctx->obj);
i915_gem_context_unreference(dctx);
- dev_priv->mm.aliasing_ppgtt = NULL;
}
int i915_gem_context_enable(struct drm_i915_private *dev_priv)
@@ -494,9 +470,6 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
struct intel_ring_buffer *ring;
int ret, i;
- if (!HAS_HW_CONTEXTS(dev_priv->dev))
- return 0;
-
/* This is the only place the aliasing PPGTT gets enabled, which means
* it has to happen before we bail on reset */
if (dev_priv->mm.aliasing_ppgtt) {
@@ -511,7 +484,7 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
BUG_ON(!dev_priv->ring[RCS].default_context);
for_each_ring(ring, dev_priv, i) {
- ret = do_switch(ring, ring->default_context);
+ ret = i915_switch_context(ring, ring->default_context);
if (ret)
return ret;
}
@@ -534,19 +507,6 @@ static int context_idr_cleanup(int id, void *p, void *data)
int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
{
struct drm_i915_file_private *file_priv = file->driver_priv;
- struct drm_i915_private *dev_priv = dev->dev_private;
-
- if (!HAS_HW_CONTEXTS(dev)) {
- /* Cheat for hang stats */
- file_priv->private_default_ctx =
- kzalloc(sizeof(struct i915_hw_context), GFP_KERNEL);
-
- if (file_priv->private_default_ctx == NULL)
- return -ENOMEM;
-
- file_priv->private_default_ctx->vm = &dev_priv->gtt.base;
- return 0;
- }
idr_init(&file_priv->context_idr);
@@ -567,14 +527,10 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
{
struct drm_i915_file_private *file_priv = file->driver_priv;
- if (!HAS_HW_CONTEXTS(dev)) {
- kfree(file_priv->private_default_ctx);
- return;
- }
-
idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
- i915_gem_context_unreference(file_priv->private_default_ctx);
idr_destroy(&file_priv->context_idr);
+
+ i915_gem_context_unreference(file_priv->private_default_ctx);
}
struct i915_hw_context *
@@ -582,9 +538,6 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
{
struct i915_hw_context *ctx;
- if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
- return file_priv->private_default_ctx;
-
ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
if (!ctx)
return ERR_PTR(-ENOENT);
@@ -766,7 +719,6 @@ unpin_out:
/**
* i915_switch_context() - perform a GPU context switch.
* @ring: ring for which we'll execute the context switch
- * @file_priv: file_priv associated with the context, may be NULL
* @to: the context to switch to
*
* The context life cycle is simple. The context refcount is incremented and
@@ -775,24 +727,30 @@ unpin_out:
* object while letting the normal object tracking destroy the backing BO.
*/
int i915_switch_context(struct intel_ring_buffer *ring,
- struct drm_file *file,
struct i915_hw_context *to)
{
struct drm_i915_private *dev_priv = ring->dev->dev_private;
WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
- BUG_ON(file && to == NULL);
-
- /* We have the fake context */
- if (!HAS_HW_CONTEXTS(ring->dev)) {
- ring->last_context = to;
+ if (to->obj == NULL) { /* We have the fake context */
+ if (to != ring->last_context) {
+ i915_gem_context_reference(to);
+ if (ring->last_context)
+ i915_gem_context_unreference(ring->last_context);
+ ring->last_context = to;
+ }
return 0;
}
return do_switch(ring, to);
}
+static bool hw_context_enabled(struct drm_device *dev)
+{
+ return to_i915(dev)->hw_context_size;
+}
+
int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{
@@ -801,7 +759,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
struct i915_hw_context *ctx;
int ret;
- if (!HAS_HW_CONTEXTS(dev))
+ if (!hw_context_enabled(dev))
return -ENODEV;
ret = i915_mutex_lock_interruptible(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 34914025e750..0ec8621eb4f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1222,7 +1222,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
if (ret)
goto err;
- ret = i915_switch_context(ring, file, ctx);
+ ret = i915_switch_context(ring, ctx);
if (ret)
goto err;
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: Always use kref tracking for all contexts.
2014-04-09 8:07 ` Chris Wilson
@ 2014-04-11 6:37 ` Ben Widawsky
0 siblings, 0 replies; 10+ messages in thread
From: Ben Widawsky @ 2014-04-11 6:37 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, Mika Kuoppala
On Wed, Apr 09, 2014 at 09:07:36AM +0100, Chris Wilson wrote:
> If we always initialize kref for the context, even if we are using fake
> contexts for hangstats when there is no hw support, we can forgo the
> dance to dereference the ctx->obj and inspect whether we are permitted
> to use kref inside i915_gem_context_reference() and _unreference().
>
> My ulterior motive here is to improve the debugging of a use-after-free
> of ctx->obj. This patch avoids the dereference here and instead forces
> the assertion checks associated with kref.
>
> v2: Refactor the fake contexts to being even more like the real
> contexts, so that there is much less duplicated and special case code.
>
> v3: Tweaks.
> v4: Tweaks, minor.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76671
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: lu hua <huax.lu@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
I couldn't spot any problems, but I got a bit lazier with each review
since v2.
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 8 +-
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_context.c | 234 ++++++++++++-----------------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> 4 files changed, 101 insertions(+), 145 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 533bd8f6a5b1..0557bd92b26b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2276,20 +2276,18 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> int i915_gem_context_enable(struct drm_i915_private *dev_priv);
> void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> int i915_switch_context(struct intel_ring_buffer *ring,
> - struct drm_file *file, struct i915_hw_context *to);
> + struct i915_hw_context *to);
> struct i915_hw_context *
> i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
> void i915_gem_context_free(struct kref *ctx_ref);
> static inline void i915_gem_context_reference(struct i915_hw_context *ctx)
> {
> - if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> - kref_get(&ctx->ref);
> + kref_get(&ctx->ref);
> }
>
> static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
> {
> - if (ctx->obj && HAS_HW_CONTEXTS(ctx->obj->base.dev))
> - kref_put(&ctx->ref, i915_gem_context_free);
> + kref_put(&ctx->ref, i915_gem_context_free);
> }
>
> static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index eb5cf077c626..4368437458fd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2819,7 +2819,7 @@ int i915_gpu_idle(struct drm_device *dev)
>
> /* Flush everything onto the inactive list. */
> for_each_ring(ring, dev_priv, i) {
> - ret = i915_switch_context(ring, NULL, ring->default_context);
> + ret = i915_switch_context(ring, ring->default_context);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 30b355afb362..f77b4c126465 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -96,9 +96,6 @@
> #define GEN6_CONTEXT_ALIGN (64<<10)
> #define GEN7_CONTEXT_ALIGN 4096
>
> -static int do_switch(struct intel_ring_buffer *ring,
> - struct i915_hw_context *to);
> -
> static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
> {
> struct drm_device *dev = ppgtt->base.dev;
> @@ -185,13 +182,15 @@ void i915_gem_context_free(struct kref *ctx_ref)
> typeof(*ctx), ref);
> struct i915_hw_ppgtt *ppgtt = NULL;
>
> - /* We refcount even the aliasing PPGTT to keep the code symmetric */
> - if (USES_PPGTT(ctx->obj->base.dev))
> - ppgtt = ctx_to_ppgtt(ctx);
> + if (ctx->obj) {
> + /* We refcount even the aliasing PPGTT to keep the code symmetric */
> + if (USES_PPGTT(ctx->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);
> + /* 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);
> + }
>
> if (ppgtt)
> kref_put(&ppgtt->ref, ppgtt_release);
> @@ -232,40 +231,40 @@ __create_hw_context(struct drm_device *dev,
> return ERR_PTR(-ENOMEM);
>
> kref_init(&ctx->ref);
> - ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> - INIT_LIST_HEAD(&ctx->link);
> - if (ctx->obj == NULL) {
> - kfree(ctx);
> - DRM_DEBUG_DRIVER("Context object allocated failed\n");
> - return ERR_PTR(-ENOMEM);
> - }
> + list_add_tail(&ctx->link, &dev_priv->context_list);
>
> - /*
> - * Try to make the context utilize L3 as well as LLC.
> - *
> - * On VLV we don't have L3 controls in the PTEs so we
> - * shouldn't touch the cache level, especially as that
> - * would make the object snooped which might have a
> - * negative performance impact.
> - */
> - if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) {
> - ret = i915_gem_object_set_cache_level(ctx->obj,
> - I915_CACHE_L3_LLC);
> - /* Failure shouldn't ever happen this early */
> - if (WARN_ON(ret))
> + if (dev_priv->hw_context_size) {
> + ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
> + if (ctx->obj == NULL) {
> + ret = -ENOMEM;
> goto err_out;
> - }
> + }
>
> - list_add_tail(&ctx->link, &dev_priv->context_list);
> + /*
> + * Try to make the context utilize L3 as well as LLC.
> + *
> + * On VLV we don't have L3 controls in the PTEs so we
> + * shouldn't touch the cache level, especially as that
> + * would make the object snooped which might have a
> + * negative performance impact.
> + */
> + if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) {
> + ret = i915_gem_object_set_cache_level(ctx->obj,
> + I915_CACHE_L3_LLC);
> + /* Failure shouldn't ever happen this early */
> + if (WARN_ON(ret))
> + goto err_out;
> + }
> + }
>
> /* Default context will never have a file_priv */
> - if (file_priv == NULL)
> - return ctx;
> -
> - ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID, 0,
> - GFP_KERNEL);
> - if (ret < 0)
> - goto err_out;
> + if (file_priv != NULL) {
> + ret = idr_alloc(&file_priv->context_idr, ctx,
> + DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
> + if (ret < 0)
> + goto err_out;
> + } else
> + ret = DEFAULT_CONTEXT_ID;
>
> ctx->file_priv = file_priv;
> ctx->id = ret;
> @@ -302,7 +301,7 @@ i915_gem_create_context(struct drm_device *dev,
> if (IS_ERR(ctx))
> return ctx;
>
> - if (is_global_default_ctx) {
> + if (is_global_default_ctx && ctx->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
> @@ -350,7 +349,7 @@ i915_gem_create_context(struct drm_device *dev,
> return ctx;
>
> err_unpin:
> - if (is_global_default_ctx)
> + if (is_global_default_ctx && ctx->obj)
> i915_gem_object_ggtt_unpin(ctx->obj);
> err_destroy:
> i915_gem_context_unreference(ctx);
> @@ -360,32 +359,22 @@ err_destroy:
> void i915_gem_context_reset(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_ring_buffer *ring;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return;
> -
> /* Prevent the hardware from restoring the last context (which hung) on
> * the next switch */
> for (i = 0; i < I915_NUM_RINGS; i++) {
> - struct i915_hw_context *dctx;
> - if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> - continue;
> + struct intel_ring_buffer *ring = &dev_priv->ring[i];
> + struct i915_hw_context *dctx = ring->default_context;
>
> /* Do a fake switch to the default context */
> - ring = &dev_priv->ring[i];
> - dctx = ring->default_context;
> - if (WARN_ON(!dctx))
> + if (ring->last_context == dctx)
> continue;
>
> if (!ring->last_context)
> continue;
>
> - if (ring->last_context == dctx)
> - continue;
> -
> - if (i == RCS) {
> + if (dctx->obj && i == RCS) {
> WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
> get_context_alignment(dev), 0));
> /* Fake a finish/inactive */
> @@ -402,44 +391,35 @@ void i915_gem_context_reset(struct drm_device *dev)
> int i915_gem_context_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_ring_buffer *ring;
> + struct i915_hw_context *ctx;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return 0;
> -
> /* Init should only be called once per module load. Eventually the
> * restriction on the context_disabled check can be loosened. */
> if (WARN_ON(dev_priv->ring[RCS].default_context))
> return 0;
>
> - dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> -
> - if (dev_priv->hw_context_size > (1<<20)) {
> - DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
> - return -E2BIG;
> + if (HAS_HW_CONTEXTS(dev)) {
> + dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> + if (dev_priv->hw_context_size > (1<<20)) {
> + DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
> + dev_priv->hw_context_size);
> + dev_priv->hw_context_size = 0;
> + }
> }
>
> - dev_priv->ring[RCS].default_context =
> - i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> -
> - if (IS_ERR_OR_NULL(dev_priv->ring[RCS].default_context)) {
> - DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %ld\n",
> - PTR_ERR(dev_priv->ring[RCS].default_context));
> - return PTR_ERR(dev_priv->ring[RCS].default_context);
> + ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> + if (IS_ERR(ctx)) {
> + DRM_ERROR("Failed to create default global context (error %ld)\n",
> + PTR_ERR(ctx));
> + return PTR_ERR(ctx);
> }
>
> - for (i = RCS + 1; i < I915_NUM_RINGS; i++) {
> - if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> - continue;
> -
> - ring = &dev_priv->ring[i];
> -
> - /* NB: RCS will hold a ref for all rings */
> - ring->default_context = dev_priv->ring[RCS].default_context;
> - }
> + /* NB: RCS will hold a ref for all rings */
> + for (i = 0; i < I915_NUM_RINGS; i++)
> + dev_priv->ring[i].default_context = ctx;
>
> - DRM_DEBUG_DRIVER("HW context support initialized\n");
> + DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
> return 0;
> }
>
> @@ -449,33 +429,30 @@ void i915_gem_context_fini(struct drm_device *dev)
> struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
> int i;
>
> - if (!HAS_HW_CONTEXTS(dev))
> - return;
> -
> - /* 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. */
> - intel_gpu_reset(dev);
> -
> - /* When default context is created and switched to, base object refcount
> - * will be 2 (+1 from object creation and +1 from do_switch()).
> - * i915_gem_context_fini() will be called after gpu_idle() has switched
> - * to default context. So we need to unreference the base object once
> - * to offset the do_switch part, so that i915_gem_context_unreference()
> - * can then free the base object correctly. */
> - 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);
> - i915_gem_context_unreference(dctx);
> - dev_priv->ring[RCS].last_context = NULL;
> + if (dctx->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. */
> + intel_gpu_reset(dev);
> +
> + /* When default context is created and switched to, base object refcount
> + * will be 2 (+1 from object creation and +1 from do_switch()).
> + * i915_gem_context_fini() will be called after gpu_idle() has switched
> + * to default context. So we need to unreference the base object once
> + * to offset the do_switch part, so that i915_gem_context_unreference()
> + * can then free the base object correctly. */
> + 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);
> + i915_gem_context_unreference(dctx);
> + dev_priv->ring[RCS].last_context = NULL;
> + }
> }
>
> for (i = 0; i < I915_NUM_RINGS; i++) {
> struct intel_ring_buffer *ring = &dev_priv->ring[i];
> - if (!(INTEL_INFO(dev)->ring_mask & (1<<i)))
> - continue;
>
> if (ring->last_context)
> i915_gem_context_unreference(ring->last_context);
> @@ -486,7 +463,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>
> i915_gem_object_ggtt_unpin(dctx->obj);
> i915_gem_context_unreference(dctx);
> - dev_priv->mm.aliasing_ppgtt = NULL;
> }
>
> int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> @@ -494,9 +470,6 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> struct intel_ring_buffer *ring;
> int ret, i;
>
> - if (!HAS_HW_CONTEXTS(dev_priv->dev))
> - return 0;
> -
> /* This is the only place the aliasing PPGTT gets enabled, which means
> * it has to happen before we bail on reset */
> if (dev_priv->mm.aliasing_ppgtt) {
> @@ -511,7 +484,7 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
> BUG_ON(!dev_priv->ring[RCS].default_context);
>
> for_each_ring(ring, dev_priv, i) {
> - ret = do_switch(ring, ring->default_context);
> + ret = i915_switch_context(ring, ring->default_context);
> if (ret)
> return ret;
> }
> @@ -534,19 +507,6 @@ static int context_idr_cleanup(int id, void *p, void *data)
> int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> -
> - if (!HAS_HW_CONTEXTS(dev)) {
> - /* Cheat for hang stats */
> - file_priv->private_default_ctx =
> - kzalloc(sizeof(struct i915_hw_context), GFP_KERNEL);
> -
> - if (file_priv->private_default_ctx == NULL)
> - return -ENOMEM;
> -
> - file_priv->private_default_ctx->vm = &dev_priv->gtt.base;
> - return 0;
> - }
>
> idr_init(&file_priv->context_idr);
>
> @@ -567,14 +527,10 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
>
> - if (!HAS_HW_CONTEXTS(dev)) {
> - kfree(file_priv->private_default_ctx);
> - return;
> - }
> -
> idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
> - i915_gem_context_unreference(file_priv->private_default_ctx);
> idr_destroy(&file_priv->context_idr);
> +
> + i915_gem_context_unreference(file_priv->private_default_ctx);
> }
>
> struct i915_hw_context *
> @@ -582,9 +538,6 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> {
> struct i915_hw_context *ctx;
>
> - if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
> - return file_priv->private_default_ctx;
> -
> ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> if (!ctx)
> return ERR_PTR(-ENOENT);
> @@ -766,7 +719,6 @@ unpin_out:
> /**
> * i915_switch_context() - perform a GPU context switch.
> * @ring: ring for which we'll execute the context switch
> - * @file_priv: file_priv associated with the context, may be NULL
> * @to: the context to switch to
> *
> * The context life cycle is simple. The context refcount is incremented and
> @@ -775,24 +727,30 @@ unpin_out:
> * object while letting the normal object tracking destroy the backing BO.
> */
> int i915_switch_context(struct intel_ring_buffer *ring,
> - struct drm_file *file,
> struct i915_hw_context *to)
> {
> struct drm_i915_private *dev_priv = ring->dev->dev_private;
>
> WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>
> - BUG_ON(file && to == NULL);
> -
> - /* We have the fake context */
> - if (!HAS_HW_CONTEXTS(ring->dev)) {
> - ring->last_context = to;
> + if (to->obj == NULL) { /* We have the fake context */
> + if (to != ring->last_context) {
> + i915_gem_context_reference(to);
> + if (ring->last_context)
> + i915_gem_context_unreference(ring->last_context);
> + ring->last_context = to;
> + }
> return 0;
> }
>
> return do_switch(ring, to);
> }
>
> +static bool hw_context_enabled(struct drm_device *dev)
> +{
> + return to_i915(dev)->hw_context_size;
> +}
> +
> int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file)
> {
> @@ -801,7 +759,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> struct i915_hw_context *ctx;
> int ret;
>
> - if (!HAS_HW_CONTEXTS(dev))
> + if (!hw_context_enabled(dev))
> return -ENODEV;
>
> ret = i915_mutex_lock_interruptible(dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 34914025e750..0ec8621eb4f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1222,7 +1222,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> if (ret)
> goto err;
>
> - ret = i915_switch_context(ring, file, ctx);
> + ret = i915_switch_context(ring, ctx);
> if (ret)
> goto err;
>
> --
> 1.9.1
>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 10+ messages in thread