Re: [Intel-gfx] [PATCH] drm/i915: Clean-up idr table if context create fails.
On Tuesday 07 April 2015 02:02 PM, Chris Wilson wrote: On Tue, Apr 07, 2015 at 10:20:15AM +0200, Daniel Vetter wrote: 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, deepa...@linux.intel.com wrote: From: Deepak S Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() Signed-off-by: Deepak S --- 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. I suggested that it look like: http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem_context.c#n179 -Chris Thanks Chris and Daniel, I will submit cleaned up patches ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Clean-up idr table if context create fails.
On Tue, Apr 07, 2015 at 10:20:15AM +0200, Daniel Vetter wrote: > 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, deepa...@linux.intel.com wrote: > > >>From: Deepak S > > >> > > >>Cleanup idr table if any error happens after __create_hw_context() in > > >>i915_gem_create_context() > > >> > > >>Signed-off-by: Deepak S > > >>--- > > >> 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. I suggested that it look like: http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem_context.c#n179 -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Clean-up idr table if context create fails.
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, deepa...@linux.intel.com wrote: > >>From: Deepak S > >> > >>Cleanup idr table if any error happens after __create_hw_context() in > >>i915_gem_create_context() > >> > >>Signed-off-by: Deepak S > >>--- > >> 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
Re: [Intel-gfx] [PATCH] drm/i915: Clean-up idr table if context create fails.
On Monday 30 March 2015 09:13 PM, Daniel Vetter wrote: On Mon, Mar 30, 2015 at 08:03:58PM +0530, deepa...@linux.intel.com wrote: From: Deepak S Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() Signed-off-by: Deepak S --- 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? - Thanks, Deepak i915_gem_context_unreference(ctx); return ERR_PTR(ret); } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Clean-up idr table if context create fails.
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6093 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -3 270/270 267/270 ILK 303/303 303/303 SNB 304/304 304/304 IVB 337/337 337/337 BYT 287/287 287/287 HSW 361/361 361/361 BDW 309/309 309/309 -Detailed- Platform Testdrm-intel-nightly Series Applied PNV igt@gem_userptr_blits@coherency-sync CRASH(4)PASS(2) CRASH(2) *PNV igt@gem_pwrite_pread@snooped-pwrite-blt-cpu_mmap-correctness PASS(2) CRASH(1)PASS(1) PNV igt@gem_tiled_pread_pwrite FAIL(1)PASS(3) FAIL(2) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Clean-up idr table if context create fails.
On Mon, Mar 30, 2015 at 08:03:58PM +0530, deepa...@linux.intel.com wrote: > From: Deepak S > > Cleanup idr table if any error happens after __create_hw_context() in > i915_gem_create_context() > > Signed-off-by: Deepak S > --- > 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 > i915_gem_context_unreference(ctx); > return ERR_PTR(ret); > } > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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
[Intel-gfx] [PATCH] drm/i915: Clean-up idr table if context create fails.
From: Deepak S Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() Signed-off-by: Deepak S --- 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); i915_gem_context_unreference(ctx); return ERR_PTR(ret); } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
