On 06/01/16 20:53, [email protected] wrote:
From: Alex Dai <[email protected]>
During driver unloading, the guc_client created for command submission
needs to be released to avoid memory leak.
Signed-off-by: Alex Dai <[email protected]>
---
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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
