drm/i915/guc: Add onion teardown to the GuC setup

Starting with intel_guc_loader, down to intel_guc_submission
and finally to intel_guc_log.

v2:
  - Null execbuf client outside guc_client_free (Daniele)
  - Assert if things try to get allocated twice (Daniele/Joonas)
  - Null guc->log.buf_addr when destroyed (Daniele)
  - Newline between returning success and error labels (Joonas)
  - Remove some unnecessary comments (Joonas)
  - Keep guc_log_create_extras naming convention (Joonas)
  - Helper function guc_log_has_extras (Joonas)
  - No need for separate relay_channel create/destroy. It's just another extra.
  - No need to nullify guc->log.flush_wq when destroyed (Joonas)
  - Hoist the check for has_extras out of guc_log_create_extras (Joonas)
  - Try to do i915_guc_log_register/unregister calls (kind of) symmetric (Daniele)
  - Make sure initel_guc_fini is not called before init is ever called (Daniele)

v3:
  - Remove unnecessary parenthesis (Joonas)
  - Check for logs enabled on debugfs registration
  - Rebase on top of Tvrtko's "Fix request re-submission after reset"

v4:
  - Rebased
  - Comment around enabling/disabling interrupts inside GuC logging (Joonas)
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

drivers/gpu/drm/i915/i915_drv.c

@@ -549,6 +549,8 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
 static void i915_gem_fini(struct drm_i915_private *dev_priv)
 {
 	mutex_lock(&dev_priv->drm.struct_mutex);
+	if (i915.enable_guc_loading)
+		intel_uc_fini_hw(dev_priv);
 	i915_gem_cleanup_engines(dev_priv);
 	i915_gem_context_fini(dev_priv);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
@@ -609,7 +611,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	ret = i915_gem_init(dev_priv);
 	if (ret)
-		goto cleanup_irq;
+		goto cleanup_uc;
 
 	intel_modeset_gem_init(dev);
 
@@ -631,9 +633,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (i915_gem_suspend(dev_priv))
 		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 	i915_gem_fini(dev_priv);
+cleanup_uc:
+	intel_uc_fini_fw(dev_priv);
 cleanup_irq:
-	intel_guc_fini(dev_priv);
-	intel_huc_fini(dev_priv);
 	drm_irq_uninstall(dev);
 	intel_teardown_gmbus(dev_priv);
 cleanup_csr:
@@ -1369,9 +1371,8 @@ void i915_driver_unload(struct drm_device *dev)
 	/* Flush any outstanding unpin_work. */
 	drain_workqueue(dev_priv->wq);
 
-	intel_guc_fini(dev_priv);
-	intel_huc_fini(dev_priv);
 	i915_gem_fini(dev_priv);
+	intel_uc_fini_fw(dev_priv);
 	intel_fbc_cleanup_cfb(dev_priv);
 
 	intel_power_domains_fini(dev_priv);

drivers/gpu/drm/i915/i915_gem.c

@@ -4608,10 +4608,12 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 
 	intel_mocs_init_l3cc_table(dev_priv);
 
-	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_uc_init_hw(dev_priv);
-	if (ret)
-		goto out;
+	if (i915.enable_guc_loading) {
+		/* We can't enable contexts until all firmware is loaded */
+		ret = intel_uc_init_hw(dev_priv);
+		if (ret)
+			goto out;
+	}
 
 out:
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);

drivers/gpu/drm/i915/i915_guc_submission.c

@@ -911,7 +911,6 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
 	ida_simple_remove(&guc->ctx_ids, client->ctx_index);
 err_client:
 	kfree(client);
-
 	return ERR_PTR(ret);
 }
 
@@ -937,7 +936,7 @@ static void guc_policies_init(struct guc_policies *policies)
 	policies->is_valid = 1;
 }
 
-static void guc_addon_create(struct intel_guc *guc)
+static int guc_addon_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct i915_vma *vma;
@@ -953,14 +952,13 @@ static void guc_addon_create(struct intel_guc *guc)
 	enum intel_engine_id id;
 	u32 base;
 
-	vma = guc->ads_vma;
-	if (!vma) {
-		vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
-		if (IS_ERR(vma))
-			return;
+	GEM_BUG_ON(guc->ads_vma);
 
-		guc->ads_vma = vma;
-	}
+	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
+	guc->ads_vma = vma;
 
 	page = i915_vma_first_page(vma);
 	blob = kmap(page);
@@ -997,6 +995,13 @@ static void guc_addon_create(struct intel_guc *guc)
 	blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
 
 	kunmap(page);
+
+	return 0;
+}
+
+static void guc_addon_destroy(struct intel_guc *guc)
+{
+	i915_vma_unpin_and_release(&guc->ads_vma);
 }
 
 /*
@@ -1011,6 +1016,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 	struct i915_vma *vma;
 	void *vaddr;
+	int ret;
 
 	if (!HAS_GUC_SCHED(dev_priv))
 		return 0;
@@ -1020,10 +1026,10 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	i915_guc_submission_disable(dev_priv);
 
 	if (!i915.enable_guc_submission)
-		return 0; /* not enabled  */
+		return 0;
 
 	if (guc->ctx_pool)
-		return 0; /* already allocated */
+		return 0;
 
 	vma = intel_guc_allocate_vma(guc, gemsize);
 	if (IS_ERR(vma))
@@ -1031,15 +1037,23 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 	guc->ctx_pool = vma;
 
-	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
-	if (IS_ERR(vaddr))
-		goto err;
+	vaddr = i915_gem_object_pin_map(guc->ctx_pool->obj, I915_MAP_WB);
+	if (IS_ERR(vaddr)) {
+		ret = PTR_ERR(vaddr);
+		goto err_vma;
+	}
 
 	guc->ctx_pool_vaddr = vaddr;
 
+	ret = intel_guc_log_create(guc);
+	if (ret < 0)
+		goto err_vaddr;
+
+	ret = guc_addon_create(guc);
+	if (ret < 0)
+		goto err_log;
+
 	ida_init(&guc->ctx_ids);
-	intel_guc_log_create(guc);
-	guc_addon_create(guc);
 
 	guc->execbuf_client = guc_client_alloc(dev_priv,
 					       INTEL_INFO(dev_priv)->ring_mask,
@@ -1047,14 +1061,37 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 					       dev_priv->kernel_context);
 	if (IS_ERR(guc->execbuf_client)) {
 		DRM_ERROR("Failed to create GuC client for execbuf!\n");
-		goto err;
+		ret = PTR_ERR(guc->execbuf_client);
+		goto err_ads;
 	}
 
 	return 0;
 
-err:
-	i915_guc_submission_fini(dev_priv);
-	return -ENOMEM;
+err_ads:
+	guc_addon_destroy(guc);
+err_log:
+	intel_guc_log_destroy(guc);
+err_vaddr:
+	i915_gem_object_unpin_map(guc->ctx_pool->obj);
+err_vma:
+	i915_vma_unpin_and_release(&guc->ctx_pool);
+	return ret;
+}
+
+void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
+{
+	struct intel_guc *guc = &dev_priv->guc;
+
+	if (!i915.enable_guc_submission)
+		return 0;
+
+	guc_client_free(guc->execbuf_client);
+	guc->execbuf_client = NULL;
+	ida_destroy(&guc->ctx_ids);
+	guc_addon_destroy(guc);
+	intel_guc_log_destroy(guc);
+	i915_gem_object_unpin_map(guc->ctx_pool->obj);
+	i915_vma_unpin_and_release(&guc->ctx_pool);
 }
 
 static void guc_reset_wq(struct i915_guc_client *client)
@@ -1199,26 +1236,6 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 	intel_engines_reset_default_submission(dev_priv);
 }
 
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
-{
-	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_guc_client *client;
-
-	client = fetch_and_zero(&guc->execbuf_client);
-	if (client && !IS_ERR(client))
-		guc_client_free(client);
-
-	i915_vma_unpin_and_release(&guc->ads_vma);
-	i915_vma_unpin_and_release(&guc->log.vma);
-
-	if (guc->ctx_pool_vaddr) {
-		ida_destroy(&guc->ctx_ids);
-		i915_gem_object_unpin_map(guc->ctx_pool->obj);
-	}
-
-	i915_vma_unpin_and_release(&guc->ctx_pool);
-}
-
 /**
  * intel_guc_suspend() - notify GuC entering suspend state
  * @dev_priv:	i915 device private

drivers/gpu/drm/i915/intel_guc_loader.c

@@ -430,24 +430,3 @@ int intel_guc_select_fw(struct intel_guc *guc)
 
 	return 0;
 }
-
-/**
- * intel_guc_fini() - clean up all allocated resources
- * @dev_priv:	i915 device private
- */
-void intel_guc_fini(struct drm_i915_private *dev_priv)
-{
-	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
-	struct drm_i915_gem_object *obj;
-
-	mutex_lock(&dev_priv->drm.struct_mutex);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
-	obj = fetch_and_zero(&guc_fw->obj);
-	if (obj)
-		i915_gem_object_put(obj);
-
-	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
-}

drivers/gpu/drm/i915/intel_uc.c

@@ -95,24 +95,41 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 		intel_uc_prepare_fw(dev_priv, &dev_priv->guc.fw);
 }
 
+void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
+{
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+	struct drm_i915_gem_object *obj;
+
+	obj = fetch_and_zero(&guc_fw->obj);
+	if (obj)
+		i915_gem_object_put(obj);
+
+	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
+
+	obj = fetch_and_zero(&huc_fw->obj);
+	if (obj)
+		i915_gem_object_put(obj);
+
+	huc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
+}
+
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	int ret, attempts;
 
-	/* GuC not enabled, nothing to do */
-	if (!i915.enable_guc_loading)
-		return 0;
-
 	gen9_reset_guc_interrupts(dev_priv);
 
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
 
-	if (i915.enable_guc_submission) {
-		ret = i915_guc_submission_init(dev_priv);
-		if (ret)
-			goto err;
-	}
+	/*
+	 * This is stuff we need to have available at fw load time
+	 * if we are planning to enable submission later
+	 */
+	ret = i915_guc_submission_init(dev_priv);
+	if (ret)
+		goto err_guc;
 
 	/* WaEnableuKernelHeaderValidFix:skl */
 	/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
@@ -150,7 +167,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 		ret = i915_guc_submission_enable(dev_priv);
 		if (ret)
-			goto err_submission;
+			goto err_interrupts;
 	}
 
 	return 0;
@@ -164,11 +181,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 * nonfatal error (i.e. it doesn't prevent driver load, but
 	 * marks the GPU as wedged until reset).
 	 */
+err_interrupts:
+	gen9_disable_guc_interrupts(dev_priv);
 err_submission:
-	if (i915.enable_guc_submission)
-		i915_guc_submission_fini(dev_priv);
-
-err:
+	i915_guc_submission_fini(dev_priv);
+err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
 	DRM_ERROR("GuC init failed\n");
@@ -185,6 +202,16 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
+{
+	if (i915.enable_guc_submission) {
+		i915_guc_submission_disable(dev_priv);
+		gen9_disable_guc_interrupts(dev_priv);
+	}
+	i915_guc_submission_fini(dev_priv);
+	i915_ggtt_disable_guc(dev_priv);
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command

drivers/gpu/drm/i915/intel_uc.h

@@ -187,7 +187,9 @@ struct intel_huc {
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
+void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
 int intel_uc_init_hw(struct drm_i915_private *dev_priv);
+void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
 void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
 			 struct intel_uc_fw *uc_fw);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
@@ -196,7 +198,6 @@ int intel_guc_sample_forcewake(struct intel_guc *guc);
 /* intel_guc_loader.c */
 int intel_guc_select_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct intel_guc *guc);
-void intel_guc_fini(struct drm_i915_private *dev_priv);
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
@@ -212,10 +213,11 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 
 /* intel_guc_log.c */
-void intel_guc_log_create(struct intel_guc *guc);
+int intel_guc_log_create(struct intel_guc *guc);
+void intel_guc_log_destroy(struct intel_guc *guc);
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
 void i915_guc_log_register(struct drm_i915_private *dev_priv);
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
-int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
 
 static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 {