From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 02/13] drm/i915: fix invalid reference handling of the default ctx obj Date: Fri, 13 Jul 2012 08:25:49 -0700 Message-ID: <20120713082549.5f3f7d18@bwidawsk.net> References: <1342185256-16024-1-git-send-email-chris@chris-wilson.co.uk> <1342185256-16024-3-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 cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 4DB44A1144 for ; Fri, 13 Jul 2012 08:25:57 -0700 (PDT) In-Reply-To: <1342185256-16024-3-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: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 13 Jul 2012 14:14:05 +0100 Chris Wilson wrote: > Otherwise we end up trying to unpin a freed object and BUG. > Just a quick excuse: originally do_switch was just a very low level helper, and everything called i915_switch_context (including default setup), so the bug likely didn't exist in older revisions of the series. Since you're moving around so much code, maybe we could just move do_switch() a bit higher in the file so we don't have to do the forward declaration crap anymore? I am a bit confused why I didn't hit this in my testing, but thanks for tracking it down. I must say though, really like this patch, and the cleanups it does in addition to fixing the bug is way understated in the commit message. Still, a couple of comments below. > Signed-off-by: Chris Wilson Reviewed-by: 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; The second BUG_ON I think is still fairly useful (though it would be nicer to have a context_id in the object so we could check at the time of unpin). > > 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); I haven't really looked hard, but we do end up here quite early in init, reset, unfreeze. Hopefully this is always safe to do. > /* 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, -- Ben Widawsky, Intel Open Source Technology Center