[PATCH] drm/i915: Fix locking around GuC firmware load

[PATCH] drm/i915: Fix locking around GuC firmware load

[Daniel Stone]

The GuC firmware load requires struct_mutex to create a GEM object,
but this collides badly with request_firmware. Move struct_mutex
locking down into the loader itself, so we don't hold it across the
entire load process, including request_firmware.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/i915/i915_dma.c         | 7 +------
 drivers/gpu/drm/i915/intel_guc_loader.c | 6 ++++--
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index eddcbfd..b4c340d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -408,10 +408,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	 * working irqs for e.g. gmbus and dp aux transfers. */
 	intel_modeset_init(dev);
 
-	/* intel_guc_ucode_init() needs the mutex to allocate GEM objects */
-	mutex_lock(&dev->struct_mutex);
 	intel_guc_ucode_init(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	ret = i915_gem_init(dev);
 	if (ret)
@@ -454,9 +451,7 @@ cleanup_gem:
 	i915_gem_context_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
 cleanup_irq:
-	mutex_lock(&dev->struct_mutex);
 	intel_guc_ucode_fini(dev);
-	mutex_unlock(&dev->struct_mutex);
 	drm_irq_uninstall(dev);
 cleanup_gem_stolen:
 	i915_gem_cleanup_stolen(dev);
@@ -1191,8 +1186,8 @@ int i915_driver_unload(struct drm_device *dev)
 	/* Flush any outstanding unpin_work. */
 	flush_workqueue(dev_priv->wq);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_guc_ucode_fini(dev);
+	mutex_lock(&dev->struct_mutex);
 	i915_gem_cleanup_ringbuffer(dev);
 	i915_gem_context_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index ac31696..e56d19d 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -504,7 +504,9 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
 			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
 
+	mutex_lock(&dev->struct_mutex);
 	obj = i915_gem_object_create_from_data(dev, fw->data, fw->size);
+	mutex_unlock(&dev->struct_mutex);
 	if (IS_ERR_OR_NULL(obj)) {
 		err = obj ? PTR_ERR(obj) : -ENOMEM;
 		goto fail;
@@ -540,8 +542,6 @@ fail:
  * @dev:	drm device
  *
  * Called early during driver load, but after GEM is initialised.
- * The device struct_mutex must be held by the caller, as we're
- * going to allocate a GEM object to hold the firmware image.
  *
  * The firmware will be transferred to the GuC's memory later,
  * when intel_guc_ucode_load() is called.
@@ -598,9 +598,11 @@ void intel_guc_ucode_fini(struct drm_device *dev)
 	direct_interrupts_to_host(dev_priv);
 	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;
 }
-- 
2.5.0

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

Re: [PATCH] drm/i915: Fix locking around GuC firmware load

[Dave Airlie]

On 4 November 2015 at 07:42, Daniel Stone <daniels@collabora.com> wrote:
> The GuC firmware load requires struct_mutex to create a GEM object,
> but this collides badly with request_firmware. Move struct_mutex
> locking down into the loader itself, so we don't hold it across the
> entire load process, including request_firmware.
>
> Signed-off-by: Daniel Stone <daniels@collabora.com>

This avoids the lockdep spew on my machine.

Reviewed-by: Dave AIrlie <airlied@redhat.com>

I'm going to put this into -next, granted I've still got the other WARN spew
but lockdep working is a good start.

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

Re: [PATCH] drm/i915: Fix locking around GuC firmware load

[Yu Dai]

On 11/03/2015 07:22 PM, Dave Airlie wrote:
> On 4 November 2015 at 07:42, Daniel Stone <daniels@collabora.com> wrote:
> > The GuC firmware load requires struct_mutex to create a GEM object,
> > but this collides badly with request_firmware. Move struct_mutex
> > locking down into the loader itself, so we don't hold it across the
> > entire load process, including request_firmware.
> >
> > Signed-off-by: Daniel Stone <daniels@collabora.com>
>
> This avoids the lockdep spew on my machine.
>
> Reviewed-by: Dave AIrlie <airlied@redhat.com>
>
> I'm going to put this into -next, granted I've still got the other WARN spew
> but lockdep working is a good start.
>
>

Looks good to me. Thanks for the fix.

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

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