| Message ID | 1427726038-19718-1-git-send-email-deepak.s@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > 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
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@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 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 '*'
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? - 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
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
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, 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. 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
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, 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. > 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
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); }