drm/i915: Clean-up idr table if context create fails.
Subsystems (2)
PATCH 1/1 drm/i915: Clean-up idr table if context create fails.
From: Deepak S <[email protected]>
Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context()
--- drivers/gpu/drm/i915/i915_gem_context.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/gpu/drm/i915/i915_gem_context.c +2 -0
@@ -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); }
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 15 lines · On Mon, Mar 30, 2015 at 08:03:58PM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
Cleanup idr table if any error happens after
__create_hw_context()ini915_gem_create_context()Signed-off-by Deepak S <[email protected]>--- drivers/gpu/drm/i915/i915_gem_context.c | 2 ++ 1 file changed, 2 insertions(+)drivers/gpu/drm/i915/i915_gem_context.c +0
-0
drivers/gpu/drm/i915/i915_gem_context.c err_unpin: · line 291
Quoted 5 lines
drivers/gpu/drm/i915/i915_gem_context.c +2
-0@@ -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
Quoted 10 lines
drivers/gpu/drm/i915/i915_gem_context.c +0
-0i915_gem_context_unreference (ctx);return ERR_PTR (ret); }Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 24 lines · Daniel Vetter
Quoted 15 lines · On Mon, Mar 30, 2015 at 08:03:58PM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
Cleanup idr table if any error happens after
__create_hw_context()ini915_gem_create_context()Signed-off-by Deepak S <[email protected]>--- drivers/gpu/drm/i915/i915_gem_context.c | 2 ++ 1 file changed, 2 insertions(+)drivers/gpu/drm/i915/i915_gem_context.c +0
-0drivers/gpu/drm/i915/i915_gem_context.c err_unpin: · line 291
Quoted 5 lines
drivers/gpu/drm/i915/i915_gem_context.c +2
-0@@ -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 toidr_removeunconditional.
Quoted 1 line
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
Quoted 10 lines
Quoted 10 lines
drivers/gpu/drm/i915/i915_gem_context.c +0
-0i915_gem_context_unreference (ctx);return ERR_PTR (ret); }Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v2 1/1 drm/i915: Clean-up idr table if context create fails.
From: Deepak S <[email protected]>
Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context()
v2: add a new err_idr (Daniel)
--- drivers/gpu/drm/i915/i915_gem_context.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_gem_context.c +4 -2
@@ -262,7 +262,7 @@ i915_gem_create_context(struct drm_device *dev, get_context_alignment(dev), 0);if (ret) {DRM_DEBUG_DRIVER ("Couldn't pin %d\n" , ret); - goto err_destroy; + goto err_idr; } }
@@ -286,7 +286,9 @@ i915_gem_create_context(struct drm_device *dev, 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: +err_idr: +if (ctx->file_priv) +idr_remove (&ctx->file_priv->context_idr, ctx->user_handle);i915_gem_context_unreference (ctx);return ERR_PTR (ret); }
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Task id: 6122 -------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 272/272 272/272 ILK 302/302 302/302 SNB 303/303 303/303 IVB 338/338 338/338 BYT -1 287/287 286/287 HSW 361/361 361/361 BDW 308/308 308/308
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(14) FAIL(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 27 lines · Deepak S
Quoted 24 lines · Daniel Vetter
Quoted 15 lines · On Mon, Mar 30, 2015 at 08:03:58PM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
Cleanup idr table if any error happens after
__create_hw_context()ini915_gem_create_context()Signed-off-by Deepak S <[email protected]>--- drivers/gpu/drm/i915/i915_gem_context.c | 2 ++ 1 file changed, 2 insertions(+)drivers/gpu/drm/i915/i915_gem_context.c +0
-0drivers/gpu/drm/i915/i915_gem_context.c err_unpin: · line 291
Quoted 5 lines
drivers/gpu/drm/i915/i915_gem_context.c +2
-0@@ -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 toidr_removeunconditional.
Quoted 5 lines
Quoted 1 line
Thanks, Daniel
Thanks Daniel for review.
I do not think we can have a unconditional idr remove since for global ctxi915_gem_create_contextcalled 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
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 34 lines · Daniel Vetter
Quoted 27 lines · Deepak S
Quoted 24 lines · Daniel Vetter
Quoted 15 lines · On Mon, Mar 30, 2015 at 08:03:58PM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
Cleanup idr table if any error happens after
__create_hw_context()ini915_gem_create_context()Signed-off-by Deepak S <[email protected]>--- drivers/gpu/drm/i915/i915_gem_context.c | 2 ++ 1 file changed, 2 insertions(+)drivers/gpu/drm/i915/i915_gem_context.c +0
-0drivers/gpu/drm/i915/i915_gem_context.c err_unpin: · line 291
Quoted 5 lines
drivers/gpu/drm/i915/i915_gem_context.c +2
-0@@ -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 toidr_removeunconditional.Quoted 5 lines
Quoted 1 line
Thanks, Daniel
Thanks Daniel for review.
I do not think we can have a unconditional idr remove since for global ctxi915_gem_create_contextcalled with file_priv=NULL?
Quoted 5 lines
Hm right, the entire control-flow in there is a bit funny. I think a much cleaner solution would be to drop the
file_privefromcreate_contextand add a newi915_gem_context_create_userwhich wrapscreate_contextand 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
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 32 lines · Chris Wilson
Quoted 31 lines · Daniel Vetter
Quoted 30 lines · Deepak S
Quoted 24 lines · Daniel Vetter
Quoted 15 lines · On Mon, Mar 30, 2015 at 08:03:58PM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
Cleanup idr table if any error happens after
__create_hw_context()ini915_gem_create_context()Signed-off-by Deepak S <[email protected]>--- drivers/gpu/drm/i915/i915_gem_context.c | 2 ++ 1 file changed, 2 insertions(+)drivers/gpu/drm/i915/i915_gem_context.c +0
-0drivers/gpu/drm/i915/i915_gem_context.c err_unpin: · line 291
Quoted 5 lines
drivers/gpu/drm/i915/i915_gem_context.c +2
-0@@ -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 toidr_removeunconditional.Quoted 1 line
Thanks, Daniel
Thanks Daniel for review.
I do not think we can have a unconditional idr remove since for global ctxi915_gem_create_contextcalled with file_priv=NULL?
Quoted 8 lines
Quoted 5 lines
Hm right, the entire control-flow in there is a bit funny. I think a much cleaner solution would be to drop the
file_privefromcreate_contextand add a newi915_gem_context_create_userwhich wrapscreate_contextand 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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 Test drm-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 [email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx