From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: [PATCH 02/13] drm/i915: fix invalid reference handling of the default ctx obj Date: Fri, 13 Jul 2012 14:14:05 +0100 Message-ID: <1342185256-16024-3-git-send-email-chris@chris-wilson.co.uk> References: <1342185256-16024-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (smtp.fireflyinternet.com [109.228.6.236]) by gabe.freedesktop.org (Postfix) with ESMTP id 9C1839E9DB for ; Fri, 13 Jul 2012 06:14:56 -0700 (PDT) In-Reply-To: <1342185256-16024-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: intel-gfx@lists.freedesktop.org Cc: Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org Otherwise we end up trying to unpin a freed object and BUG. Signed-off-by: Chris Wilson Cc: Ben Widawsky --- drivers/gpu/drm/i915/i915_gem_context.c | 82 +++++++++++-------------------- 1 file changed, 30 insertions(+), 52 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 90857f8..b0a855f 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -97,8 +97,7 @@ static struct i915_hw_context * i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id); -static int do_switch(struct drm_i915_gem_object *from_obj, - struct i915_hw_context *to, u32 seqno); +static int do_switch(struct i915_hw_context *to); static int get_context_size(struct drm_device *dev) { @@ -220,26 +219,24 @@ static int create_default_context(struct drm_i915_private *dev_priv) */ dev_priv->ring[RCS].default_context = ctx; ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false); - if (ret) { - do_destroy(ctx); - return ret; - } + if (ret) + goto err_destroy; ret = i915_gem_object_set_to_gtt_domain(ctx->obj, true); - if (ret) { - i915_gem_object_unpin(ctx->obj); - do_destroy(ctx); - return ret; - } + if (ret) + goto err_unpin; - ret = do_switch(NULL, ctx, 0); - if (ret) { - i915_gem_object_unpin(ctx->obj); - do_destroy(ctx); - } else { - DRM_DEBUG_DRIVER("Default HW context loaded\n"); - } + ret = do_switch(ctx); + if (ret) + goto err_unpin; + DRM_DEBUG_DRIVER("Default HW context loaded\n"); + return 0; + +err_unpin: + i915_gem_object_unpin(ctx->obj); +err_destroy: + do_destroy(ctx); return ret; } @@ -366,16 +363,14 @@ mi_set_context(struct intel_ring_buffer *ring, return ret; } -static int do_switch(struct drm_i915_gem_object *from_obj, - struct i915_hw_context *to, - u32 seqno) +static int do_switch(struct i915_hw_context *to) { - struct intel_ring_buffer *ring = NULL; + struct drm_i915_gem_object *from_obj = to->ring->last_context_obj; u32 hw_flags = 0; int ret; - BUG_ON(to == NULL); - BUG_ON(from_obj != NULL && from_obj->pin_count == 0); + if (from_obj == to->obj) + return 0; ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false); if (ret) @@ -389,8 +384,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj, else if (WARN_ON_ONCE(from_obj == to->obj)) /* not yet expected */ hw_flags |= MI_FORCE_RESTORE; - ring = to->ring; - ret = mi_set_context(ring, to, hw_flags); + ret = mi_set_context(to->ring, to, hw_flags); if (ret) { i915_gem_object_unpin(to->obj); return ret; @@ -403,6 +397,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj, * MI_SET_CONTEXT instead of when the next seqno has completed. */ if (from_obj != NULL) { + u32 seqno = i915_gem_next_request_seqno(to->ring); /* As long as MI_SET_CONTEXT is serializing, ie. it flushes the * whole damn pipeline, we don't need to explicitly mark the * object dirty. The only exception is that the context must be @@ -412,13 +407,15 @@ static int do_switch(struct drm_i915_gem_object *from_obj, */ from_obj->base.write_domain = I915_GEM_DOMAIN_INSTRUCTION; from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; - i915_gem_object_move_to_active(from_obj, ring, seqno); + i915_gem_object_move_to_active(from_obj, to->ring, seqno); from_obj->dirty = 1; BUG_ON(from_obj->ring != to->ring); i915_gem_object_unpin(from_obj); + drm_gem_object_unreference(&from_obj->base); } - ring->last_context_obj = to->obj; + drm_gem_object_reference(&to->obj->base); + to->ring->last_context_obj = to->obj; to->is_initialized = true; return 0; @@ -442,10 +439,7 @@ int i915_switch_context(struct intel_ring_buffer *ring, int to_id) { struct drm_i915_private *dev_priv = ring->dev->dev_private; - struct drm_i915_file_private *file_priv = NULL; struct i915_hw_context *to; - struct drm_i915_gem_object *from_obj = ring->last_context_obj; - int ret; if (dev_priv->hw_contexts_disabled) return 0; @@ -453,34 +447,18 @@ int i915_switch_context(struct intel_ring_buffer *ring, if (ring != &dev_priv->ring[RCS]) return 0; - if (file) - file_priv = file->driver_priv; - if (to_id == DEFAULT_CONTEXT_ID) { to = ring->default_context; } else { - to = i915_gem_context_get(file_priv, to_id); + if (file == NULL) + return -EINVAL; + + to = i915_gem_context_get(file->driver_priv, to_id); if (to == NULL) return -ENOENT; } - if (from_obj == to->obj) - return 0; - - ret = do_switch(from_obj, to, i915_gem_next_request_seqno(to->ring)); - if (ret) - return ret; - - /* Just to make the code a little cleaner we take the object reference - * after the switch was successful. It would be more intuitive to ref - * the 'to' object before the switch but we know the refcount must be >0 - * if context_get() succeeded, and we hold struct mutex. So it's safe to - * do this here/now - */ - drm_gem_object_reference(&to->obj->base); - if (from_obj != NULL) - drm_gem_object_unreference(&from_obj->base); - return ret; + return do_switch(to); } int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, -- 1.7.10.4