Re: [Intel-gfx] [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@intel.com wrote:
> From: Alex Dai 
>
> 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 
>
> 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: [Intel-gfx] [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@intel.com wrote:
> From: Alex Dai 
>
> 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 
>
> 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: [Intel-gfx] [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@intel.com wrote:

From: Alex Dai 

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 

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

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

[yu . dai]

From: Alex Dai 

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 

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