From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 02/13] drm/i915: fix invalid reference handling of the default ctx obj Date: Fri, 13 Jul 2012 17:37:14 +0200 Message-ID: <20120713153714.GE5721@phenom.ffwll.local> 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 mail-wg0-f41.google.com (mail-wg0-f41.google.com [74.125.82.41]) by gabe.freedesktop.org (Postfix) with ESMTP id 361ECA1156 for ; Fri, 13 Jul 2012 08:37:19 -0700 (PDT) Received: by wgbds1 with SMTP id ds1so625856wgb.0 for ; Fri, 13 Jul 2012 08:37:18 -0700 (PDT) Content-Disposition: inline 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, Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Fri, Jul 13, 2012 at 02:14:05PM +0100, Chris Wilson wrote: > Otherwise we end up trying to unpin a freed object and BUG. > > Signed-off-by: Chris Wilson > Cc: Ben Widawsky Afact this patch contains quite some code refactoring that does not relate directly to the fix (or if it does, I fail to see the direct relevance). So I think this either needs an explanation in the commit message or be put into a separate patch (I agree though for actual code cleanups). For the fix itself I seem to be a bit dense again - the only thing I see is that you move the refcount handling into do_switch. Afacs we do the ref-handling in both cases only when do_switch is successful, and also right at the end of do_switch (or right afterwards). So can you please enlighten your clueless maintainer a bit an explain how things blow up? Yours, Daniel > --- > 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 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48