[PATCH] drm/i915: Free resources correctly if we cannot map status page during ctx create

[PATCH] drm/i915: Free resources correctly if we cannot map status page during ctx create

[Arun Siluvery]

We are not freeing memory allocated for ringbuf and ctx if we fail
to map status page so release all resources correctly.

Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index f3efdbd..a84d24b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1777,8 +1777,10 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 		ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(ctx_obj);
 		ring->status_page.page_addr =
 				kmap(sg_page(ctx_obj->pages->sgl));
-		if (ring->status_page.page_addr == NULL)
-			return -ENOMEM;
+		if (ring->status_page.page_addr == NULL) {
+			ret = -ENOMEM;
+			goto error;
+		}
 		ring->status_page.obj = ctx_obj;
 	}
 
-- 
2.1.2

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

Re: [PATCH] drm/i915: Free resources correctly if we cannot map status page during ctx create

[Daniel, Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 1893 bytes --]

> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf

> Of Arun Siluvery

> Sent: Monday, November 17, 2014 3:48 PM

> To: intel-gfx@lists.freedesktop.org

> Subject: [Intel-gfx] [PATCH] drm/i915: Free resources correctly if we cannot

> map status page during ctx create

>

> We are not freeing memory allocated for ringbuf and ctx if we fail to map

> status page so release all resources correctly.

>

> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com<mailto:arun.siluvery@linux.intel.com>>

> ---

>  drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--

>  1 file changed, 4 insertions(+), 2 deletions(-)

>

> diff --git a/drivers/gpu/drm/i915/intel_lrc.c

> b/drivers/gpu/drm/i915/intel_lrc.c

> index f3efdbd..a84d24b 100644

> --- a/drivers/gpu/drm/i915/intel_lrc.c

> +++ b/drivers/gpu/drm/i915/intel_lrc.c

> @@ -1777,8 +1777,10 @@ int intel_lr_context_deferred_create(struct

> intel_context *ctx,

>                             ring->status_page.gfx_addr =

> i915_gem_obj_ggtt_offset(ctx_obj);

>                             ring->status_page.page_addr =

>                                                             kmap(sg_page(ctx_obj->pages->sgl));

> -                           if (ring->status_page.page_addr == NULL)

> -                                           return -ENOMEM;

> +                          if (ring->status_page.page_addr == NULL) {

> +                                          ret = -ENOMEM;

> +                                          goto error;

> +                          }

>                             ring->status_page.obj = ctx_obj;

>             }





Hi Arun,



I think your tree is out of date.  See this patch:

http://patchwork.freedesktop.org/patch/35828/



Cheers,

Thomas.



[-- Attachment #1.2: Type: text/html, Size: 7636 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Free resources correctly if we cannot map status page during ctx create

[Siluvery, Arun]

On 17/11/2014 15:54, Daniel, Thomas wrote:
>> -----Original Message-----
>
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
>
>> Of Arun Siluvery
>
>> Sent: Monday, November 17, 2014 3:48 PM
>
>> To: intel-gfx@lists.freedesktop.org
>
>> Subject: [Intel-gfx] [PATCH] drm/i915: Free resources correctly if we cannot
>
>> map status page during ctx create
>
>>
>
>> We are not freeing memory allocated for ringbuf and ctx if we fail to map
>
>> status page so release all resources correctly.
>
>>
>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com<mailto:arun.siluvery@linux.intel.com>>
>
>> ---
>
>>   drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
>
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>
>>
>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
>
>> b/drivers/gpu/drm/i915/intel_lrc.c
>
>> index f3efdbd..a84d24b 100644
>
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>
>> @@ -1777,8 +1777,10 @@ int intel_lr_context_deferred_create(struct
>
>> intel_context *ctx,
>
>>                              ring->status_page.gfx_addr =
>
>> i915_gem_obj_ggtt_offset(ctx_obj);
>
>>                              ring->status_page.page_addr =
>
>>                                                              kmap(sg_page(ctx_obj->pages->sgl));
>
>> -                           if (ring->status_page.page_addr == NULL)
>
>> -                                           return -ENOMEM;
>
>> +                          if (ring->status_page.page_addr == NULL) {
>
>> +                                          ret = -ENOMEM;
>
>> +                                          goto error;
>
>> +                          }
>
>>                              ring->status_page.obj = ctx_obj;
>
>>              }
>
>
>
>
>
> Hi Arun,
>
>
>
> I think your tree is out of date.  See this patch:
>
> http://patchwork.freedesktop.org/patch/35828/
>
>
>
> Cheers,
>
> Thomas.
>
>
You are right, I don't have latest changes.
This patch can be ignored.

regards
Arun

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

Re: [PATCH] drm/i915: Free resources correctly if we cannot map status page during ctx create

[Daniel Vetter]

On Mon, Nov 17, 2014 at 03:48:27PM +0000, Arun Siluvery wrote:
> We are not freeing memory allocated for ringbuf and ctx if we fail
> to map status page so release all resources correctly.
> 
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index f3efdbd..a84d24b 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1777,8 +1777,10 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
>  		ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(ctx_obj);
>  		ring->status_page.page_addr =
>  				kmap(sg_page(ctx_obj->pages->sgl));
> -		if (ring->status_page.page_addr == NULL)
> -			return -ENOMEM;
> +		if (ring->status_page.page_addr == NULL) {
> +			ret = -ENOMEM;
> +			goto error;
> +		}

Since this popped up: Do we have an automated igt testcase to exercise
this corner-case?

We do have it for legacy (gen6/7) contexts, including exercising the
shrinker logic to get rid of misplaced ctx objects to avoid ggtt
fragmentation hassles ...
-Daniel

>  		ring->status_page.obj = ctx_obj;
>  	}
>  
> -- 
> 2.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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: Free resources correctly if we cannot map status page during ctx create

[Chris Wilson]

On Mon, Nov 17, 2014 at 08:04:09PM +0100, Daniel Vetter wrote:
> On Mon, Nov 17, 2014 at 03:48:27PM +0000, Arun Siluvery wrote:
> > We are not freeing memory allocated for ringbuf and ctx if we fail
> > to map status page so release all resources correctly.
> > 
> > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index f3efdbd..a84d24b 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1777,8 +1777,10 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
> >  		ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(ctx_obj);
> >  		ring->status_page.page_addr =
> >  				kmap(sg_page(ctx_obj->pages->sgl));
> > -		if (ring->status_page.page_addr == NULL)
> > -			return -ENOMEM;
> > +		if (ring->status_page.page_addr == NULL) {
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> 
> Since this popped up: Do we have an automated igt testcase to exercise
> this corner-case?

How? kmap() never returns NULL.
-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: Free resources correctly if we cannot map status page during ctx create

[Daniel Vetter]

On Tue, Nov 18, 2014 at 07:44:40AM +0000, Chris Wilson wrote:
> On Mon, Nov 17, 2014 at 08:04:09PM +0100, Daniel Vetter wrote:
> > On Mon, Nov 17, 2014 at 03:48:27PM +0000, Arun Siluvery wrote:
> > > We are not freeing memory allocated for ringbuf and ctx if we fail
> > > to map status page so release all resources correctly.
> > > 
> > > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > > index f3efdbd..a84d24b 100644
> > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > @@ -1777,8 +1777,10 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
> > >  		ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(ctx_obj);
> > >  		ring->status_page.page_addr =
> > >  				kmap(sg_page(ctx_obj->pages->sgl));
> > > -		if (ring->status_page.page_addr == NULL)
> > > -			return -ENOMEM;
> > > +		if (ring->status_page.page_addr == NULL) {
> > > +			ret = -ENOMEM;
> > > +			goto error;
> > > +		}
> > 
> > Since this popped up: Do we have an automated igt testcase to exercise
> > this corner-case?
> 
> How? kmap() never returns NULL.

Oh dear.

/me hangs head in shame

I'll do a patch to correct this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Drop return value from lrc_setup_hardware_status_page

[Daniel Vetter]

kmap never fails.

Spotted-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b7c4c9ab9012..3cf15c4da0e8 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1706,7 +1706,7 @@ static uint32_t get_lr_context_size(struct intel_engine_cs *ring)
 	return ret;
 }
 
-static int lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
+static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
 		struct drm_i915_gem_object *default_ctx_obj)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
@@ -1716,15 +1716,11 @@ static int lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
 	ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(default_ctx_obj);
 	ring->status_page.page_addr =
 			kmap(sg_page(default_ctx_obj->pages->sgl));
-	if (ring->status_page.page_addr == NULL)
-		return -ENOMEM;
 	ring->status_page.obj = default_ctx_obj;
 
 	I915_WRITE(RING_HWS_PGA(ring->mmio_base),
 			(u32)ring->status_page.gfx_addr);
 	POSTING_READ(RING_HWS_PGA(ring->mmio_base));
-
-	return 0;
 }
 
 /**
@@ -1811,13 +1807,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 	ctx->engine[ring->id].ringbuf = ringbuf;
 	ctx->engine[ring->id].state = ctx_obj;
 
-	if (ctx == ring->default_context) {
-		ret = lrc_setup_hardware_status_page(ring, ctx_obj);
-		if (ret) {
-			DRM_ERROR("Failed to setup hardware status page\n");
-			goto error;
-		}
-	}
+	if (ctx == ring->default_context)
+		lrc_setup_hardware_status_page(ring, ctx_obj);
 
 	if (ring->id == RCS && !ctx->rcs_initialized) {
 		if (ring->init_context) {
-- 
2.1.1

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

Re: [PATCH] drm/i915: Drop return value from

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=290/290->290/290
PNV: pass/total=362/362->362/362
ILK: pass/total=381/381->374/381
IVB: pass/total=522/559->522/559
SNB: pass/total=444/444->444/444
HSW: pass/total=595/595->594/595
BDW: pass/total=436/436->436/436
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
ILK: Intel_gpu_tools, igt_kms_flip_bcs-wf_vblank-vs-modeset-interruptible, PASS(4, M37M26) -> DMESG_WARN(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_flip_flip-vs-rmfb-interruptible, PASS(4, M37M26) -> DMESG_WARN(2, M26)PASS(2, M26)
ILK: Intel_gpu_tools, igt_kms_flip_plain-flip, PASS(4, M37M26) -> DMESG_WARN(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_flip_rcs-wf_vblank-vs-dpms-interruptible, PASS(4, M37M26) -> DMESG_WARN(3, M26)PASS(1, M26)
ILK: Intel_gpu_tools, igt_kms_flip_vblank-vs-hang-interruptible, PASS(4, M37M26) -> DMESG_WARN(3, M26)PASS(1, M26)
ILK: Intel_gpu_tools, igt_kms_flip_wf_vblank-ts-check, PASS(4, M37M26) -> DMESG_WARN(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_flip_wf_vblank-vs-dpms, DMESG_WARN(1, M26)PASS(3, M6M26) -> DMESG_WARN(2, M26)PASS(2, M26)
HSW: Intel_gpu_tools, igt_gem_reset_stats_close-pending-render, PASS(3, M19M40) -> NO_RESULT(1, M40)PASS(3, M40)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx