On Tue, Apr 02, 2013 at 03:45:42PM -0700, Ben Widawsky wrote:
> From: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>
> In preparation to do analysis of which context was
> guilty of gpu hung, store kreffed context pointer
> into request struct.
>
> This allows us to inspect contexts when gpu is reset
> even if those contexts would already be released
> by userspace.
>
> 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 Chris)
>
> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
Now I remember why my version of reference counting was so much more
complicated. In my case, I want to keep the last context around instead
of the last context object. To do this we can't do a kref_put until
we've switched to the next context (similar to how we manage the context
object). I want to do this since the context stores the PPGTT which will
currently be in use. I need to switch PDEs at context switch time.
So the below isn't really useful to me, I think, and I believe I need a
more complex refcounting mechanism as I described on IRC earlier today.
Daniel, Chris, thoughts?
> ---
> drivers/gpu/drm/i915/i915_drv.h | 10 ++++++++--
> drivers/gpu/drm/i915/i915_gem.c | 16 +++++++++++++++-
> drivers/gpu/drm/i915/i915_gem_context.c | 17 +++++++++++++----
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 ++++---
> 4 files changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5b0c699..b88b67d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -437,6 +437,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;
> @@ -1240,6 +1241,9 @@ struct drm_i915_gem_request {
> /** Postion in the ringbuffer of the end of the request */
> u32 tail;
>
> + /** Context related to this request */
> + struct i915_hw_context *ctx;
> +
> /** Time at which this request was emitted, in jiffies. */
> unsigned long emitted_jiffies;
>
> @@ -1630,9 +1634,10 @@ int __must_check i915_gpu_idle(struct drm_device *dev);
> int __must_check i915_gem_idle(struct drm_device *dev);
> int i915_do_add_request(struct intel_ring_buffer *ring,
> u32 *seqno,
> - struct drm_file *file);
> + struct drm_file *file,
> + struct i915_hw_context *ctx);
> #define i915_add_request(ring, seqno) \
> - i915_do_add_request(ring, seqno, NULL)
> + i915_do_add_request(ring, seqno, NULL, NULL)
> int __must_check i915_wait_seqno(struct intel_ring_buffer *ring,
> uint32_t seqno);
> int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
> @@ -1676,6 +1681,7 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> struct i915_hw_context * __must_check
> i915_switch_context(struct intel_ring_buffer *ring,
> struct drm_file *file, int to_id);
> +void i915_gem_context_free(struct kref *ctx_ref);
> 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.c b/drivers/gpu/drm/i915/i915_gem.c
> index fbbe7d9..e55c4a8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1997,7 +1997,8 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
> int
> i915_do_add_request(struct intel_ring_buffer *ring,
> u32 *out_seqno,
> - struct drm_file *file)
> + struct drm_file *file,
> + struct i915_hw_context *ctx)
> {
> drm_i915_private_t *dev_priv = ring->dev->dev_private;
> struct drm_i915_gem_request *request;
> @@ -2037,6 +2038,11 @@ i915_do_add_request(struct intel_ring_buffer *ring,
> request->seqno = intel_ring_get_seqno(ring);
> request->ring = ring;
> request->tail = request_ring_position;
> + request->ctx = ctx;
> +
> + if (request->ctx)
> + kref_get(&request->ctx->ref);
> +
> request->emitted_jiffies = jiffies;
> was_empty = list_empty(&ring->request_list);
> list_add_tail(&request->list, &ring->request_list);
> @@ -2101,6 +2107,10 @@ static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
>
> list_del(&request->list);
> i915_gem_request_remove_from_client(request);
> +
> + if (request->ctx)
> + kref_put(&request->ctx->ref, i915_gem_context_free);
> +
> kfree(request);
> }
>
> @@ -2195,6 +2205,10 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>
> list_del(&request->list);
> i915_gem_request_remove_from_client(request);
> +
> + if (request->ctx)
> + kref_put(&request->ctx->ref, i915_gem_context_free);
> +
> kfree(request);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ddf26a6..19feeb6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -126,11 +126,18 @@ static int get_context_size(struct drm_device *dev)
>
> static void do_destroy(struct i915_hw_context *ctx)
> {
> + drm_gem_object_unreference(&ctx->obj->base);
> + kfree(ctx);
> +}
> +
> +void i915_gem_context_free(struct kref *ctx_ref)
> +{
> + struct i915_hw_context *ctx = container_of(ctx_ref,
> + typeof(*ctx), ref);
> if (ctx->file_priv)
> idr_remove(&ctx->file_priv->context_idr, ctx->id);
>
> - drm_gem_object_unreference(&ctx->obj->base);
> - kfree(ctx);
> + do_destroy(ctx);
> }
>
> static struct i915_hw_context *
> @@ -145,6 +152,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);
> @@ -275,7 +283,8 @@ static int context_idr_cleanup(int id, void *p, void *data)
>
> BUG_ON(id == DEFAULT_CONTEXT_ID);
>
> - do_destroy(ctx);
> + ctx->file_priv = NULL;
> + kref_put(&ctx->ref, i915_gem_context_free);
>
> return 0;
> }
> @@ -511,7 +520,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> return -ENOENT;
> }
>
> - do_destroy(ctx);
> + kref_put(&ctx->ref, i915_gem_context_free);
>
> mutex_unlock(&dev->struct_mutex);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d87b94b..7f58b2e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -794,13 +794,14 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
> static void
> i915_gem_execbuffer_retire_commands(struct drm_device *dev,
> struct drm_file *file,
> - struct intel_ring_buffer *ring)
> + struct intel_ring_buffer *ring,
> + struct i915_hw_context *ctx)
> {
> /* Unconditionally force add_request to emit a full flush. */
> ring->gpu_caches_dirty = true;
>
> /* Add a breadcrumb for the completion of the batch buffer */
> - (void)i915_do_add_request(ring, NULL, file);
> + (void)i915_do_add_request(ring, NULL, file, ctx);
> }
>
> static int
> @@ -1076,7 +1077,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags);
>
> i915_gem_execbuffer_move_to_active(&eb->objects, ring);
> - i915_gem_execbuffer_retire_commands(dev, file, ring);
> + i915_gem_execbuffer_retire_commands(dev, file, ring, ctx);
>
> err:
> eb_destroy(eb);
> --
> 1.8.2
>
--
Ben Widawsky, Intel Open Source Technology Center
