[PATCH] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[PATCH] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[yu.dai]

From: Alex Dai <yu.dai@intel.com>

During driver unloading, the guc_client created for command submission
needs to be released to avoid memory leak.

Signed-off-by: Alex Dai <yu.dai@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 9c24424..8ce4f32 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -995,6 +995,9 @@ void i915_guc_submission_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc *guc = &dev_priv->guc;
 
+	if (i915.enable_guc_submission)
+		i915_guc_submission_disable(dev);
+
 	gem_release_guc_obj(dev_priv->guc.ads_obj);
 	guc->ads_obj = NULL;
 
-- 
2.5.0

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

✓ success: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 532a438d16e609a4b8f161c0a18b34f24001ed8f drm-intel-nightly: 2016y-01m-06d-15h-38m-17s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-vebox:
                skip       -> PASS       (bdw-nuci7)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c:
                pass       -> SKIP       (bdw-nuci7)

bdw-nuci7        total:132  pass:1    dwarn:0   dfail:0   fail:0   skip:131
bsw-nuc-2        total:135  pass:115  dwarn:0   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:121  dwarn:1   dfail:0   fail:0   skip:13 
skl-i5k-2        total:135  pass:125  dwarn:2   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:125  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:123  dwarn:0   dfail:0   fail:0   skip:12 

Results at /archive/results/CI_IGT_test/Patchwork_1107/

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

Re: [PATCH] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[Dave Gordon]

On 06/01/16 20:53, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
>
> During driver unloading, the guc_client created for command submission
> needs to be released to avoid memory leak.
>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 9c24424..8ce4f32 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -995,6 +995,9 @@ void i915_guc_submission_fini(struct drm_device *dev)
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc *guc = &dev_priv->guc;
>
> +	if (i915.enable_guc_submission)
> +		i915_guc_submission_disable(dev);
> +
>   	gem_release_guc_obj(dev_priv->guc.ads_obj);
>   	guc->ads_obj = NULL;

This looks like the right thing to do, but the wrong place to do it.

i915_guc_submission_{init,enable,disable,fini} are the top-level 
functions exported from this source file and called (only) from 
intel_guc_loader.c

Therefore, the code in intel_guc_ucode_fini() should call 
submission_disable() before submission_fini(), like this:

/**
  * intel_guc_ucode_fini() - clean up all allocated resources
  * @dev:        drm device
  */
void intel_guc_ucode_fini(struct drm_device *dev)
{
         struct drm_i915_private *dev_priv = dev->dev_private;
         struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;

         direct_interrupts_to_host(dev_priv);
+	i915_guc_submission_disable(dev);
	i915_guc_submission_fini(dev);

         mutex_lock(&dev->struct_mutex);
         if (guc_fw->guc_fw_obj)
                 drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
         guc_fw->guc_fw_obj = NULL;
         mutex_unlock(&dev->struct_mutex);

         guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
}

There's no need for it to be conditional, as disable (and fini) are 
idempotent; if a thing hasn't been allocated, or has already been 
deallocated, then these functions will just do nothing.

HOWEVER,

while reviewing this I've noticed that the locking is all screwed up; 
basically "bf248ca drm/i915: Fix locking around GuC firmware load"
removed locking round the calls into i915_guc_loader.c and added it back 
in a few places, but not enough.

It would probably have been better to have left the locking in the 
caller, and hence round the entirety of the calls to _init, _load, 
_fini, and then explicitly DROP the mutex only for the duration of the 
request_firmware call.

It would have been better still not to insist on synchronous firmware 
load in the first place; the original generic (and asynchronous) loader 
didn't require struct_mutex or any other locking around the 
request_firmware() call, so we wouldn't now have to fix it (again).

At present, in intel_guc_loader.c, intel_guc_ucode_load() is called with 
the struct_mutex already held by the caller, but _init() and _fini() are 
called with it NOT held.

All exported functions in i915_guc_submission.c expect it to be held 
when they're called.

On that basis, what we need now is:

guc_fw_fetch() needs to take & release the mutex round the unreference 
in the fail: path (like the code in _fini above).

intel_guc_ucode_fini() needs to extend the scope of the lock to enclose 
all calls to _submission_ functions. So the above becomes:

/**
* intel_guc_ucode_fini() - clean up all allocated resources
* @dev: drm device
*/
void intel_guc_ucode_fini(struct drm_device *dev)
{
	struct drm_i915_private *dev_priv = dev->dev_private;
	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;

	mutex_lock(&dev->struct_mutex);
	direct_interrupts_to_host(dev_priv);
	i915_guc_submission_disable(dev);
	i915_guc_submission_fini(dev);

	if (guc_fw->guc_fw_obj)
		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
	guc_fw->guc_fw_obj = NULL;
	mutex_unlock(&dev->struct_mutex);

	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
}

FINALLY,

intel_guc_ucode_load() should probably call i915_guc_submission_fini() 
in the failure path (after submission_disable()) as it called 
submission_init() earlier. Not critical, as it will get called from 
ucode_fini() anyway, but it improves symmetry.

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

Re: [PATCH] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[Yu Dai]

On 01/12/2016 04:11 AM, Dave Gordon wrote:
> On 06/01/16 20:53, yu.dai@intel.com wrote:
> > From: Alex Dai <yu.dai@intel.com>
> >
> > During driver unloading, the guc_client created for command submission
> > needs to be released to avoid memory leak.
> >
> > Signed-off-by: Alex Dai <yu.dai@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 9c24424..8ce4f32 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -995,6 +995,9 @@ void i915_guc_submission_fini(struct drm_device *dev)
> >   	struct drm_i915_private *dev_priv = dev->dev_private;
> >   	struct intel_guc *guc = &dev_priv->guc;
> >
> > +	if (i915.enable_guc_submission)
> > +		i915_guc_submission_disable(dev);
> > +
> >   	gem_release_guc_obj(dev_priv->guc.ads_obj);
> >   	guc->ads_obj = NULL;
>
> This looks like the right thing to do, but the wrong place to do it.
>
> i915_guc_submission_{init,enable,disable,fini} are the top-level
> functions exported from this source file and called (only) from
> intel_guc_loader.c
>
> Therefore, the code in intel_guc_ucode_fini() should call
> submission_disable() before submission_fini(), like this:
>
> /**
>    * intel_guc_ucode_fini() - clean up all allocated resources
>    * @dev:        drm device
>    */
> void intel_guc_ucode_fini(struct drm_device *dev)
> {
>           struct drm_i915_private *dev_priv = dev->dev_private;
>           struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>
>           direct_interrupts_to_host(dev_priv);
> +	i915_guc_submission_disable(dev);
> 	i915_guc_submission_fini(dev);
>
>           mutex_lock(&dev->struct_mutex);
>           if (guc_fw->guc_fw_obj)
>                   drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
>           guc_fw->guc_fw_obj = NULL;
>           mutex_unlock(&dev->struct_mutex);
>
>           guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> }
>
> There's no need for it to be conditional, as disable (and fini) are
> idempotent; if a thing hasn't been allocated, or has already been
> deallocated, then these functions will just do nothing.

I agree. We will keep the symmetry here 
i915_guc_submission_init(_enable, _disable and _fini).
> HOWEVER,
>
> while reviewing this I've noticed that the locking is all screwed up;
> basically "bf248ca drm/i915: Fix locking around GuC firmware load"
> removed locking round the calls into i915_guc_loader.c and added it back
> in a few places, but not enough.
>
> It would probably have been better to have left the locking in the
> caller, and hence round the entirety of the calls to _init, _load,
> _fini, and then explicitly DROP the mutex only for the duration of the
> request_firmware call.
>
> It would have been better still not to insist on synchronous firmware
> load in the first place; the original generic (and asynchronous) loader
> didn't require struct_mutex or any other locking around the
> request_firmware() call, so we wouldn't now have to fix it (again).
>
> At present, in intel_guc_loader.c, intel_guc_ucode_load() is called with
> the struct_mutex already held by the caller, but _init() and _fini() are
> called with it NOT held.
>
> All exported functions in i915_guc_submission.c expect it to be held
> when they're called.
>
> On that basis, what we need now is:
>
> guc_fw_fetch() needs to take & release the mutex round the unreference
> in the fail: path (like the code in _fini above).

I prefer the current approach that only takes lock for necessary 
critical session.
> intel_guc_ucode_fini() needs to extend the scope of the lock to enclose
> all calls to _submission_ functions. So the above becomes:
>
> /**
> * intel_guc_ucode_fini() - clean up all allocated resources
> * @dev: drm device
> */
> void intel_guc_ucode_fini(struct drm_device *dev)
> {
> 	struct drm_i915_private *dev_priv = dev->dev_private;
> 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>
> 	mutex_lock(&dev->struct_mutex);
> 	direct_interrupts_to_host(dev_priv);
> 	i915_guc_submission_disable(dev);
> 	i915_guc_submission_fini(dev);
>
> 	if (guc_fw->guc_fw_obj)
> 		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
> 	guc_fw->guc_fw_obj = NULL;
> 	mutex_unlock(&dev->struct_mutex);
>
> 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> }

This is done by patch https://patchwork.freedesktop.org/patch/68708/. 
Please review this one.
> FINALLY,
>
> intel_guc_ucode_load() should probably call i915_guc_submission_fini()
> in the failure path (after submission_disable()) as it called
> submission_init() earlier. Not critical, as it will get called from
> ucode_fini() anyway, but it improves symmetry.
>
>

We don't have ucode_unload(). The ucode_fini() is actually doing the 
unload job. Because ucode_fini() needs to acquire the lock but 
ucode_load() expects that lock is held by caller, calling ucode_fini() 
inside ucode_load() is not good. I don't think it is worth to wrap up a 
ucode_unload() call which only includes two lines 
(direct_interrupts_to_host and i915_guc_submission_disable).

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

[PATCH v1] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[yu.dai]

From: Alex Dai <yu.dai@intel.com>

During driver unloading, the guc_client created for command submission
needs to be released to avoid memory leak.

The struct_mutex needs to be held before tearing down GuC.

v1: Move i915_guc_submission_disable out of i915_guc_submission_fini and
    take struct_mutex lock before release GuC client. (Dave Gordon)

Signed-off-by: Alex Dai <yu.dai@intel.com>

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index d20788f..70fa8f5 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -631,10 +631,11 @@ void intel_guc_ucode_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 
+	mutex_lock(&dev->struct_mutex);
 	direct_interrupts_to_host(dev_priv);
+	i915_guc_submission_disable(dev);
 	i915_guc_submission_fini(dev);
 
-	mutex_lock(&dev->struct_mutex);
 	if (guc_fw->guc_fw_obj)
 		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
 	guc_fw->guc_fw_obj = NULL;
-- 
2.5.0

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

✗ failure: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 06d0112e293dfdea7f796d4085f755898850947b drm-intel-nightly: 2016y-01m-12d-21h-16m-40s UTC integration manifest

Test gem_basic:
        Subgroup create-close:
                pass       -> DMESG-WARN (skl-i5k-2)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> DMESG-FAIL (skl-i5k-2)
Test gem_ctx_param_basic:
        Subgroup basic:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup invalid-param-set:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup non-root-set-no-zeromap:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup root-set-no-zeromap-disabled:
                pass       -> DMESG-WARN (skl-i5k-2)
Test gem_mmap:
        Subgroup basic:
                pass       -> DMESG-WARN (skl-i5k-2)
Test gem_mmap_gtt:
        Subgroup basic-read:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup basic-write:
                pass       -> DMESG-WARN (skl-i5k-2)
Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (bdw-ultra)
Test kms_addfb_basic:
        Subgroup addfb25-modifier-no-flag:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup addfb25-x-tiled-mismatch:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup addfb25-yf-tiled:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup bad-pitch-1024:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup bad-pitch-63:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup bad-pitch-999:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup clobberred-modifier:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup too-high:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup too-wide:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup unused-offsets:
                pass       -> DMESG-WARN (skl-i5k-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (skl-i7k-2)
                dmesg-warn -> PASS       (ilk-hp8440p)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (ilk-hp8440p)
        Subgroup basic-plain-flip:
                pass       -> DMESG-FAIL (skl-i5k-2)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> DMESG-FAIL (skl-i5k-2)
        Subgroup read-crc-pipe-a-frame-sequence:
                fail       -> PASS       (snb-x220t)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-FAIL (skl-i5k-2)
Test prime_self_import:
        Subgroup basic-with_two_bos:
                pass       -> DMESG-WARN (skl-i5k-2)

bdw-nuci7        total:138  pass:129  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:138  pass:132  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:141  pass:115  dwarn:2   dfail:0   fail:0   skip:24 
byt-nuc          total:141  pass:123  dwarn:3   dfail:0   fail:0   skip:15 
hsw-brixbox      total:141  pass:134  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:141  pass:137  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:141  pass:100  dwarn:4   dfail:0   fail:0   skip:37 
skl-i5k-2        total:141  pass:108  dwarn:20  dfail:4   fail:0   skip:8  
skl-i7k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:141  pass:122  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:141  pass:122  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1158/

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

Re: [PATCH v1] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[Dave Gordon]

On 12/01/16 23:17, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
>
> During driver unloading, the guc_client created for command submission
> needs to be released to avoid memory leak.
>
> The struct_mutex needs to be held before tearing down GuC.
>
> v1: Move i915_guc_submission_disable out of i915_guc_submission_fini and
>      take struct_mutex lock before release GuC client. (Dave Gordon)

You don't seem to have implemented all the points I mentioned? I think 
you want:

drivers/gpu/drm/i915/intel_guc_loader.c:
@@ -445,6 +445,7 @@ int intel_guc_ucode_load(struct drm_device *dev)

         direct_interrupts_to_host(dev_priv);
         i915_guc_submission_disable(dev);
+       i915_guc_submission_fini(dev);

Optional, but cleaner. We called i915_guc_submission_init() earlier in 
this function, so we should call i915_guc_submission_fini() in the 
failure path. That way, we either succeed, or leave the system state 
unchanged, NOT leaving extra objects allocated.

         return err;
  }

@@ -561,10 +562,12 @@ static void guc_fw_fetch(struct drm_device *dev, 
struct intel_guc_fw *guc_fw)
  	DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
  		  guc_fw->guc_fw_path, err);

+	mutex_lock(&dev->struct_mutex);
  	obj = guc_fw->guc_fw_obj;
  	if (obj)
  		drm_gem_object_unreference(&obj->base);
  	guc_fw->guc_fw_obj = NULL;
+	mutex_unlock(&dev->struct_mutex);

This is the locking that needs to be added to the failure path.
This is required *in addition to* the locking reorganisation below.

> Signed-off-by: Alex Dai <yu.dai@intel.com>
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index d20788f..70fa8f5 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -631,10 +631,11 @@ void intel_guc_ucode_fini(struct drm_device *dev)
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>
> +	mutex_lock(&dev->struct_mutex);
>   	direct_interrupts_to_host(dev_priv);
> +	i915_guc_submission_disable(dev);
>   	i915_guc_submission_fini(dev);
>
> -	mutex_lock(&dev->struct_mutex);
>   	if (guc_fw->guc_fw_obj)
>   		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
>   	guc_fw->guc_fw_obj = NULL;

This bit is fine, but incomplete without the other changes above.

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

Re: [PATCH v1] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[Yu Dai]

On 01/13/2016 10:15 AM, Dave Gordon wrote:
> On 12/01/16 23:17, yu.dai@intel.com wrote:
> > From: Alex Dai <yu.dai@intel.com>
> >
> > During driver unloading, the guc_client created for command submission
> > needs to be released to avoid memory leak.
> >
> > The struct_mutex needs to be held before tearing down GuC.
> >
> > v1: Move i915_guc_submission_disable out of i915_guc_submission_fini and
> >      take struct_mutex lock before release GuC client. (Dave Gordon)
>
> You don't seem to have implemented all the points I mentioned? I think
> you want:
>
> drivers/gpu/drm/i915/intel_guc_loader.c:
> @@ -445,6 +445,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
>
>           direct_interrupts_to_host(dev_priv);
>           i915_guc_submission_disable(dev);
> +       i915_guc_submission_fini(dev);
>
> Optional, but cleaner. We called i915_guc_submission_init() earlier in
> this function, so we should call i915_guc_submission_fini() in the
> failure path. That way, we either succeed, or leave the system state
> unchanged, NOT leaving extra objects allocated.
>
>           return err;
>    }

I don't want this because struct_mutex is held by caller already while 
the fini() will acquire it too.
> @@ -561,10 +562,12 @@ static void guc_fw_fetch(struct drm_device *dev,
> struct intel_guc_fw *guc_fw)
>    	DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
>    		  guc_fw->guc_fw_path, err);
>
> +	mutex_lock(&dev->struct_mutex);
>    	obj = guc_fw->guc_fw_obj;
>    	if (obj)
>    		drm_gem_object_unreference(&obj->base);
>    	guc_fw->guc_fw_obj = NULL;
> +	mutex_unlock(&dev->struct_mutex);
>
> This is the locking that needs to be added to the failure path.
> This is required *in addition to* the locking reorganisation below.

I missed this part.
> > Signed-off-by: Alex Dai <yu.dai@intel.com>
> >
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index d20788f..70fa8f5 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -631,10 +631,11 @@ void intel_guc_ucode_fini(struct drm_device *dev)
> >   	struct drm_i915_private *dev_priv = dev->dev_private;
> >   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >
> > +	mutex_lock(&dev->struct_mutex);
> >   	direct_interrupts_to_host(dev_priv);
> > +	i915_guc_submission_disable(dev);
> >   	i915_guc_submission_fini(dev);
> >
> > -	mutex_lock(&dev->struct_mutex);
> >   	if (guc_fw->guc_fw_obj)
> >   		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
> >   	guc_fw->guc_fw_obj = NULL;
>
> This bit is fine, but incomplete without the other changes above.
>
> .Dave.

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

Re: [PATCH v1] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[Dave Gordon]

On 13/01/16 18:17, Yu Dai wrote:
>
> On 01/13/2016 10:15 AM, Dave Gordon wrote:
>> On 12/01/16 23:17, yu.dai@intel.com wrote:
>> > From: Alex Dai <yu.dai@intel.com>
>> >
>> > During driver unloading, the guc_client created for command submission
>> > needs to be released to avoid memory leak.
>> >
>> > The struct_mutex needs to be held before tearing down GuC.
>> >
>> > v1: Move i915_guc_submission_disable out of i915_guc_submission_fini
>> and
>> >      take struct_mutex lock before release GuC client. (Dave Gordon)
>>
>> You don't seem to have implemented all the points I mentioned? I think
>> you want:
>>
>> drivers/gpu/drm/i915/intel_guc_loader.c:
>> @@ -445,6 +445,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
>>
>>           direct_interrupts_to_host(dev_priv);
>>           i915_guc_submission_disable(dev);
>> +       i915_guc_submission_fini(dev);
>>
>> Optional, but cleaner. We called i915_guc_submission_init() earlier in
>> this function, so we should call i915_guc_submission_fini() in the
>> failure path. That way, we either succeed, or leave the system state
>> unchanged, NOT leaving extra objects allocated.
>>
>>           return err;
>>    }
>
> I don't want this because struct_mutex is held by caller already while
> the fini() will acquire it too.

Yes it is and no it won't. That's guc_*submission*_fini() I want to call 
(which requires the mutex held), not intel_guc_*ucode*_fini() (which, as 
you say, acquires it).

.Dave.

>> @@ -561,10 +562,12 @@ static void guc_fw_fetch(struct drm_device *dev,
>> struct intel_guc_fw *guc_fw)
>>        DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
>>              guc_fw->guc_fw_path, err);
>>
>> +    mutex_lock(&dev->struct_mutex);
>>        obj = guc_fw->guc_fw_obj;
>>        if (obj)
>>            drm_gem_object_unreference(&obj->base);
>>        guc_fw->guc_fw_obj = NULL;
>> +    mutex_unlock(&dev->struct_mutex);
>>
>> This is the locking that needs to be added to the failure path.
>> This is required *in addition to* the locking reorganisation below.
>
> I missed this part.
>> > Signed-off-by: Alex Dai <yu.dai@intel.com>
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> > index d20788f..70fa8f5 100644
>> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> > @@ -631,10 +631,11 @@ void intel_guc_ucode_fini(struct drm_device *dev)
>> >       struct drm_i915_private *dev_priv = dev->dev_private;
>> >       struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>> >
>> > +    mutex_lock(&dev->struct_mutex);
>> >       direct_interrupts_to_host(dev_priv);
>> > +    i915_guc_submission_disable(dev);
>> >       i915_guc_submission_fini(dev);
>> >
>> > -    mutex_lock(&dev->struct_mutex);
>> >       if (guc_fw->guc_fw_obj)
>> >           drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
>> >       guc_fw->guc_fw_obj = NULL;
>>
>> This bit is fine, but incomplete without the other changes above.
>>
>> .Dave.
>

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

[PATCH v2] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[yu.dai]

From: Alex Dai <yu.dai@intel.com>

During driver unloading, the guc_client created for command submission
needs to be released to avoid memory leak.

The struct_mutex needs to be held before tearing down GuC.

v1: Move i915_guc_submission_disable out of i915_guc_submission_fini and
    take struct_mutex lock before release GuC client. (Dave Gordon)
v2: Add the locking for failure case in guc_fw_fetch. (Dave Gordon)
    Add i915_guc_submission_fini for failure case in intel_guc_ucode_load.

Signed-off-by: Alex Dai <yu.dai@intel.com>

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index d20788f..3accd91 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -445,6 +445,7 @@ fail:
 
 	direct_interrupts_to_host(dev_priv);
 	i915_guc_submission_disable(dev);
+	i915_guc_submission_fini(dev);
 
 	return err;
 }
@@ -561,10 +562,12 @@ fail:
 	DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
 		  guc_fw->guc_fw_path, err);
 
+	mutex_lock(&dev->struct_mutex);
 	obj = guc_fw->guc_fw_obj;
 	if (obj)
 		drm_gem_object_unreference(&obj->base);
 	guc_fw->guc_fw_obj = NULL;
+	mutex_unlock(&dev->struct_mutex);
 
 	release_firmware(fw);		/* OK even if fw is NULL */
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
@@ -631,10 +634,11 @@ void intel_guc_ucode_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 
+	mutex_lock(&dev->struct_mutex);
 	direct_interrupts_to_host(dev_priv);
+	i915_guc_submission_disable(dev);
 	i915_guc_submission_fini(dev);
 
-	mutex_lock(&dev->struct_mutex);
 	if (guc_fw->guc_fw_obj)
 		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
 	guc_fw->guc_fw_obj = NULL;
-- 
2.5.0

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

Re: [PATCH v2] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[Dave Gordon]

On 13/01/16 19:01, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
>
> During driver unloading, the guc_client created for command submission
> needs to be released to avoid memory leak.
>
> The struct_mutex needs to be held before tearing down GuC.
>
> v1: Move i915_guc_submission_disable out of i915_guc_submission_fini and
>      take struct_mutex lock before release GuC client. (Dave Gordon)
> v2: Add the locking for failure case in guc_fw_fetch. (Dave Gordon)
>      Add i915_guc_submission_fini for failure case in intel_guc_ucode_load.
>
> Signed-off-by: Alex Dai <yu.dai@intel.com>

LGTM.

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index d20788f..3accd91 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -445,6 +445,7 @@ fail:
>
>   	direct_interrupts_to_host(dev_priv);
>   	i915_guc_submission_disable(dev);
> +	i915_guc_submission_fini(dev);
>
>   	return err;
>   }
> @@ -561,10 +562,12 @@ fail:
>   	DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
>   		  guc_fw->guc_fw_path, err);
>
> +	mutex_lock(&dev->struct_mutex);
>   	obj = guc_fw->guc_fw_obj;
>   	if (obj)
>   		drm_gem_object_unreference(&obj->base);
>   	guc_fw->guc_fw_obj = NULL;
> +	mutex_unlock(&dev->struct_mutex);
>
>   	release_firmware(fw);		/* OK even if fw is NULL */
>   	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
> @@ -631,10 +634,11 @@ void intel_guc_ucode_fini(struct drm_device *dev)
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>
> +	mutex_lock(&dev->struct_mutex);
>   	direct_interrupts_to_host(dev_priv);
> +	i915_guc_submission_disable(dev);
>   	i915_guc_submission_fini(dev);
>
> -	mutex_lock(&dev->struct_mutex);
>   	if (guc_fw->guc_fw_obj)
>   		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
>   	guc_fw->guc_fw_obj = NULL;
>

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

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 058740f8fced6851aeda34f366f5330322cd585f drm-intel-nightly: 2016y-01m-13d-17h-07m-44s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (bdw-nuci7)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (ivb-t430s)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (skl-i7k-2)

bdw-nuci7        total:138  pass:129  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:138  pass:132  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:141  pass:115  dwarn:2   dfail:0   fail:0   skip:24 
hsw-brixbox      total:141  pass:134  dwarn:0   dfail:0   fail:0   skip:7  
ilk-hp8440p      total:141  pass:100  dwarn:4   dfail:0   fail:0   skip:37 
ivb-t430s        total:135  pass:121  dwarn:3   dfail:4   fail:0   skip:7  
skl-i5k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8  
skl-i7k-2        total:141  pass:130  dwarn:3   dfail:0   fail:0   skip:8  
snb-dellxps      total:141  pass:122  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:141  pass:122  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1179/

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

Re: [PATCH v2] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[Tvrtko Ursulin]

On 13/01/16 19:11, Dave Gordon wrote:
> On 13/01/16 19:01, yu.dai@intel.com wrote:
>> From: Alex Dai <yu.dai@intel.com>
>>
>> During driver unloading, the guc_client created for command submission
>> needs to be released to avoid memory leak.
>>
>> The struct_mutex needs to be held before tearing down GuC.
>>
>> v1: Move i915_guc_submission_disable out of i915_guc_submission_fini and
>>      take struct_mutex lock before release GuC client. (Dave Gordon)
>> v2: Add the locking for failure case in guc_fw_fetch. (Dave Gordon)
>>      Add i915_guc_submission_fini for failure case in
>> intel_guc_ucode_load.
>>
>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>
> LGTM.
>
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

Patch merged, thanks!

Regards,

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

Re: [PATCH v2] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[Daniel Vetter]

On Mon, Jan 18, 2016 at 10:01:24AM +0000, Tvrtko Ursulin wrote:
> 
> On 13/01/16 19:11, Dave Gordon wrote:
> >On 13/01/16 19:01, yu.dai@intel.com wrote:
> >>From: Alex Dai <yu.dai@intel.com>
> >>
> >>During driver unloading, the guc_client created for command submission
> >>needs to be released to avoid memory leak.
> >>
> >>The struct_mutex needs to be held before tearing down GuC.
> >>
> >>v1: Move i915_guc_submission_disable out of i915_guc_submission_fini and
> >>     take struct_mutex lock before release GuC client. (Dave Gordon)
> >>v2: Add the locking for failure case in guc_fw_fetch. (Dave Gordon)
> >>     Add i915_guc_submission_fini for failure case in
> >>intel_guc_ucode_load.
> >>
> >>Signed-off-by: Alex Dai <yu.dai@intel.com>
> >
> >LGTM.
> >
> >Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> 
> Patch merged, thanks!

CI resulted in warnings for this patch. Per our latest discussion in
Jesse's meeting please reply to the CI mail with your analysis of why
these are all preexisting failures and with links to bugzilla. Otherwise
this patch can't go in.

I'll paste you the link to the internally wiki with the BKM.
-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: ✗ warning: Fi.CI.BAT

[Tvrtko Ursulin]

On 14/01/16 09:49, Patchwork wrote:
> == Summary ==
>
> Built on 058740f8fced6851aeda34f366f5330322cd585f drm-intel-nightly: 2016y-01m-13d-17h-07m-44s UTC integration manifest
>
> Test gem_storedw_loop:
>          Subgroup basic-render:
>                  dmesg-warn -> PASS       (bdw-nuci7)
> Test kms_force_connector_basic:
>          Subgroup force-connector-state:
>                  pass       -> SKIP       (ivb-t430s)

Unrelated, apparently new so filed:
  https://bugs.freedesktop.org/show_bug.cgi?id=93769

> Test pm_rpm:
>          Subgroup basic-pci-d3-state:
>                  pass       -> DMESG-WARN (skl-i7k-2)

Known unrelated issue:
  https://bugs.freedesktop.org/show_bug.cgi?id=93768

Please do this type of analysis in the future every time CI result is 
not a success, or I am not merging things to avoid getting spanked by 
the maintainer. :)

Regards,

Tvrtko

>
> bdw-nuci7        total:138  pass:129  dwarn:0   dfail:0   fail:0   skip:9
> bdw-ultra        total:138  pass:132  dwarn:0   dfail:0   fail:0   skip:6
> bsw-nuc-2        total:141  pass:115  dwarn:2   dfail:0   fail:0   skip:24
> hsw-brixbox      total:141  pass:134  dwarn:0   dfail:0   fail:0   skip:7
> ilk-hp8440p      total:141  pass:100  dwarn:4   dfail:0   fail:0   skip:37
> ivb-t430s        total:135  pass:121  dwarn:3   dfail:4   fail:0   skip:7
> skl-i5k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8
> skl-i7k-2        total:141  pass:130  dwarn:3   dfail:0   fail:0   skip:8
> snb-dellxps      total:141  pass:122  dwarn:5   dfail:0   fail:0   skip:14
> snb-x220t        total:141  pass:122  dwarn:5   dfail:0   fail:1   skip:13
>
> Results at /archive/results/CI_IGT_test/Patchwork_1179/
>
> _______________________________________________
> 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