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

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

[deepak.s]

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);
 	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

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

[Daniel Vetter]

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

-- 
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: [PATCH] drm/i915: Clean-up idr table if context create fails.

[shuang.he]

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 '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Deepak S]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

Cleanup idr table if any error happens after __create_hw_context() in
i915_gem_create_context()

v2: add a new err_idr (Daniel)

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f3e84c4..9b425a3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -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);
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
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
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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

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

[Chris Wilson]

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

-- 
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: [PATCH] drm/i915: Clean-up idr table if context create fails.

[Deepak S]

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


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx