On 12/01/16 23:17, yu.dai@xxxxxxxxx wrote:
From: Alex Dai <yu.dai@xxxxxxxxx>
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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
