Re: [PATCH] drm/i915: Clean-up idr table if context create fails.

From: Daniel Vetter <daniel@ffwll.ch>
To: Deepak S <deepak.s@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Clean-up idr table if context create fails.
Date: Tue, 7 Apr 2015 10:20:15 +0200	[thread overview]
Message-ID: <20150407082015.GR6354@phenom.ffwll.local> (raw)
In-Reply-To: <551D41EA.20307@linux.intel.com>

On Thu, Apr 02, 2015 at 06:49:38PM +0530, Deepak S wrote:
> 
> 
> On Monday 30 March 2015 09:13 PM, Daniel Vetter wrote:
> >On Mon, Mar 30, 2015 at 08:03:58PM +0530, deepak.s@linux.intel.com wrote:
> >>From: Deepak S <deepak.s@linux.intel.com>
> >>
> >>Cleanup idr table if any error happens after __create_hw_context() in
> >>i915_gem_create_context()
> >>
> >>Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_gem_context.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> >>index f3e84c4..69bebe5 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_context.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem_context.c
> >>@@ -287,6 +287,8 @@ err_unpin:
> >>  	if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state)
> >>  		i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
> >>  err_destroy:
> >>+	if (ctx->file_priv)
> >>+		idr_remove(&ctx->file_priv->context_idr, ctx->user_handle);
> >The common approach is to add a new err_idr: label at the op of the unwind
> >code and make the call to idr_remove unconditional.
> >
> >Thanks, Daniel
> 
> Thanks Daniel for review.
> I do not think we can have a unconditional idr remove since for global ctx
> i915_gem_create_context called with file_priv=NULL?

Hm right, the entire control-flow in there is a bit funny. I think a much
cleaner solution would be to drop the file_prive from create_context and
add a new i915_gem_context_create_user which wraps create_context and the
idr allocation. Doing the cleanup, conditionally, in a different function
than where we do the allocation is a bit too brittle imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx