| Message ID | 1367317834-11294-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 30, 2013 at 01:30:33PM +0300, Mika Kuoppala wrote: > Enabling PPGTT and also the need to track which context was guilty of > gpu hang (arb robustness enabling) have put pressure for struct i915_hw_context > to be more than just a placeholder for hw context state. > > In order to track object lifetime properly in a multi peer usage, add reference > counting for i915_hw_context. > > v2: track i915_hw_context pointers instead of using ctx_ids > (from Chris Wilson) > > v3 (Ben): Get rid of do_release() and handle refcounting more compactly. > (recommended by Chis) > > v4: kref_* put inside static inlines (Daniel Vetter) > remove code duplication on freeing context (Chris Wilson) > > v5: idr_remove and ctx->file_priv = NULL in destroy ioctl (Chris) > This actually will cause a problem if one destroys a context and later > refers to the idea of the context (multiple contexts may have the same > id, but only 1 will exist in the idr). > > v6: Strip out the request related stuff. Reworded commit message. > Got rid of do_destroy and introduced i915_gem_context_release_handle, > suggested by Chris Wilson. > > v7: idr_remove can't be called inside idr_for_each (Chris Wilson) > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v2) > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v3) > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v4) > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v5) > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v7) I can't spot anything wrong, thought I'd argue this commit message belongs with the actual reference counting to come later since a lot of the history is with bugs found there. This patch is simply introducing context refcounting interfaces and replacing explicit destruction with unreferencing (and in all cases there should be no functional impact). So bring on the real request reference counting now! Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_drv.h | 12 ++++++++++++ > drivers/gpu/drm/i915/i915_gem_context.c | 28 ++++++++++++++-------------- > 2 files changed, 26 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 14156f2..7f83be4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -452,6 +452,7 @@ struct i915_hw_ppgtt { > /* This must match up with the value previously used for execbuf2.rsvd1. */ > #define DEFAULT_CONTEXT_ID 0 > struct i915_hw_context { > + struct kref ref; > int id; > bool is_initialized; > struct drm_i915_file_private *file_priv; > @@ -1701,6 +1702,17 @@ void i915_gem_context_fini(struct drm_device *dev); > 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, int to_id); > +void i915_gem_context_free(struct kref *ctx_ref); > +static inline void i915_gem_context_reference(struct i915_hw_context *ctx) > +{ > + kref_get(&ctx->ref); > +} > + > +static inline void i915_gem_context_unreference(struct i915_hw_context *ctx) > +{ > + kref_put(&ctx->ref, i915_gem_context_free); > +} > + > int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, > struct drm_file *file); > int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index a1e8ecb..9e8c685 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -124,10 +124,10 @@ static int get_context_size(struct drm_device *dev) > return ret; > } > > -static void do_destroy(struct i915_hw_context *ctx) > +void i915_gem_context_free(struct kref *ctx_ref) > { > - if (ctx->file_priv) > - idr_remove(&ctx->file_priv->context_idr, ctx->id); > + struct i915_hw_context *ctx = container_of(ctx_ref, > + typeof(*ctx), ref); > > drm_gem_object_unreference(&ctx->obj->base); > kfree(ctx); > @@ -145,6 +145,7 @@ create_hw_context(struct drm_device *dev, > if (ctx == NULL) > return ERR_PTR(-ENOMEM); > > + kref_init(&ctx->ref); > ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size); > if (ctx->obj == NULL) { > kfree(ctx); > @@ -169,18 +170,18 @@ create_hw_context(struct drm_device *dev, > if (file_priv == NULL) > return ctx; > > - ctx->file_priv = file_priv; > - > ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID + 1, 0, > GFP_KERNEL); > if (ret < 0) > goto err_out; > + > + ctx->file_priv = file_priv; > ctx->id = ret; > > return ctx; > > err_out: > - do_destroy(ctx); > + i915_gem_context_unreference(ctx); > return ERR_PTR(ret); > } > > @@ -226,7 +227,7 @@ static int create_default_context(struct drm_i915_private *dev_priv) > err_unpin: > i915_gem_object_unpin(ctx->obj); > err_destroy: > - do_destroy(ctx); > + i915_gem_context_unreference(ctx); > return ret; > } > > @@ -262,6 +263,7 @@ void i915_gem_context_init(struct drm_device *dev) > void i915_gem_context_fini(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context; > > if (dev_priv->hw_contexts_disabled) > return; > @@ -271,9 +273,8 @@ void i915_gem_context_fini(struct drm_device *dev) > * other code, leading to spurious errors. */ > intel_gpu_reset(dev); > > - i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj); > - > - do_destroy(dev_priv->ring[RCS].default_context); > + i915_gem_object_unpin(dctx->obj); > + i915_gem_context_unreference(dctx); > } > > static int context_idr_cleanup(int id, void *p, void *data) > @@ -282,8 +283,7 @@ static int context_idr_cleanup(int id, void *p, void *data) > > BUG_ON(id == DEFAULT_CONTEXT_ID); > > - do_destroy(ctx); > - > + i915_gem_context_unreference(ctx); > return 0; > } > > @@ -512,8 +512,8 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, > return -ENOENT; > } > > - do_destroy(ctx); > - > + idr_remove(&ctx->file_priv->context_idr, ctx->id); > + i915_gem_context_unreference(ctx); > mutex_unlock(&dev->struct_mutex); > > DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id); > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Apr 30, 2013 at 11:40:16AM -0700, Ben Widawsky wrote: > On Tue, Apr 30, 2013 at 01:30:33PM +0300, Mika Kuoppala wrote: > > Enabling PPGTT and also the need to track which context was guilty of > > gpu hang (arb robustness enabling) have put pressure for struct i915_hw_context > > to be more than just a placeholder for hw context state. > > > > In order to track object lifetime properly in a multi peer usage, add reference > > counting for i915_hw_context. > > > > v2: track i915_hw_context pointers instead of using ctx_ids > > (from Chris Wilson) > > > > v3 (Ben): Get rid of do_release() and handle refcounting more compactly. > > (recommended by Chis) > > > > v4: kref_* put inside static inlines (Daniel Vetter) > > remove code duplication on freeing context (Chris Wilson) > > > > v5: idr_remove and ctx->file_priv = NULL in destroy ioctl (Chris) > > This actually will cause a problem if one destroys a context and later > > refers to the idea of the context (multiple contexts may have the same > > id, but only 1 will exist in the idr). > > > > v6: Strip out the request related stuff. Reworded commit message. > > Got rid of do_destroy and introduced i915_gem_context_release_handle, > > suggested by Chris Wilson. > > > > v7: idr_remove can't be called inside idr_for_each (Chris Wilson) > > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v2) > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v3) > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v4) > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v5) > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> (v7) > > I can't spot anything wrong, thought I'd argue this commit message > belongs with the actual reference counting to come later since a lot of > the history is with bugs found there. > > This patch is simply introducing context refcounting interfaces and > replacing explicit destruction with unreferencing (and in all cases > there should be no functional impact). > > So bring on the real request reference counting now! > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 14156f2..7f83be4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -452,6 +452,7 @@ struct i915_hw_ppgtt { /* This must match up with the value previously used for execbuf2.rsvd1. */ #define DEFAULT_CONTEXT_ID 0 struct i915_hw_context { + struct kref ref; int id; bool is_initialized; struct drm_i915_file_private *file_priv; @@ -1701,6 +1702,17 @@ void i915_gem_context_fini(struct drm_device *dev); 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, int to_id); +void i915_gem_context_free(struct kref *ctx_ref); +static inline void i915_gem_context_reference(struct i915_hw_context *ctx) +{ + kref_get(&ctx->ref); +} + +static inline void i915_gem_context_unreference(struct i915_hw_context *ctx) +{ + kref_put(&ctx->ref, i915_gem_context_free); +} + int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, struct drm_file *file); int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index a1e8ecb..9e8c685 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -124,10 +124,10 @@ static int get_context_size(struct drm_device *dev) return ret; } -static void do_destroy(struct i915_hw_context *ctx) +void i915_gem_context_free(struct kref *ctx_ref) { - if (ctx->file_priv) - idr_remove(&ctx->file_priv->context_idr, ctx->id); + struct i915_hw_context *ctx = container_of(ctx_ref, + typeof(*ctx), ref); drm_gem_object_unreference(&ctx->obj->base); kfree(ctx); @@ -145,6 +145,7 @@ create_hw_context(struct drm_device *dev, if (ctx == NULL) return ERR_PTR(-ENOMEM); + kref_init(&ctx->ref); ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size); if (ctx->obj == NULL) { kfree(ctx); @@ -169,18 +170,18 @@ create_hw_context(struct drm_device *dev, if (file_priv == NULL) return ctx; - ctx->file_priv = file_priv; - ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID + 1, 0, GFP_KERNEL); if (ret < 0) goto err_out; + + ctx->file_priv = file_priv; ctx->id = ret; return ctx; err_out: - do_destroy(ctx); + i915_gem_context_unreference(ctx); return ERR_PTR(ret); } @@ -226,7 +227,7 @@ static int create_default_context(struct drm_i915_private *dev_priv) err_unpin: i915_gem_object_unpin(ctx->obj); err_destroy: - do_destroy(ctx); + i915_gem_context_unreference(ctx); return ret; } @@ -262,6 +263,7 @@ void i915_gem_context_init(struct drm_device *dev) void i915_gem_context_fini(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context; if (dev_priv->hw_contexts_disabled) return; @@ -271,9 +273,8 @@ void i915_gem_context_fini(struct drm_device *dev) * other code, leading to spurious errors. */ intel_gpu_reset(dev); - i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj); - - do_destroy(dev_priv->ring[RCS].default_context); + i915_gem_object_unpin(dctx->obj); + i915_gem_context_unreference(dctx); } static int context_idr_cleanup(int id, void *p, void *data) @@ -282,8 +283,7 @@ static int context_idr_cleanup(int id, void *p, void *data) BUG_ON(id == DEFAULT_CONTEXT_ID); - do_destroy(ctx); - + i915_gem_context_unreference(ctx); return 0; } @@ -512,8 +512,8 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, return -ENOENT; } - do_destroy(ctx); - + idr_remove(&ctx->file_priv->context_idr, ctx->id); + i915_gem_context_unreference(ctx); mutex_unlock(&dev->struct_mutex); DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);