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 deepa...@linux.intel.com Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() Signed-off-by: Deepak S deepa...@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
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 deepa...@linux.intel.com Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() Signed-off-by: Deepak S deepa...@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. 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 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 deepa...@linux.intel.com Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() Signed-off-by: Deepak S deepa...@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. 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 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 deepa...@linux.intel.com Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() Signed-off-by: Deepak S deepa...@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? - 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.
On Mon, Mar 30, 2015 at 08:03:58PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@linux.intel.com Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() Signed-off-by: Deepak S deepa...@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 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
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
