Re: [PATCH] drm/i915/guc: Add onion teardown to the GuC setup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 16/02/17 06:18, Oscar Mateo wrote:
Starting with intel_guc_loader, down to intel_guc_submission
and finally to intel_guc_log.

Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  94 +++++----
 drivers/gpu/drm/i915/intel_guc_loader.c    |  19 +-
 drivers/gpu/drm/i915/intel_guc_log.c       | 309 +++++++++++++++--------------
 drivers/gpu/drm/i915/intel_uc.h            |   5 +-
 4 files changed, 229 insertions(+), 198 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index f7d9897..4147674 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -609,8 +609,14 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
 	return vma;
 }

-static void guc_client_free(struct i915_guc_client *client)
+static void guc_client_free(struct i915_guc_client **p_client)
 {
+	struct i915_guc_client *client;
+
+	client = fetch_and_zero(p_client);
+	if (!client)
+		return;
+

This works now, but it might become risky if we have multiple clients in the future because we might do something like the following:

	for_each_guc_client(client, guc->client_list, ...)
		guc_client_free(&client);

 	/*
 	 * XXX: wait for any outstanding submissions before freeing memory.
 	 * Be sure to drop any locks
@@ -818,7 +824,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;
@@ -835,14 +841,11 @@ static void guc_addon_create(struct intel_guc *guc)
 			sizeof(struct guc_mmio_reg_state) +
 			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;

-	vma = guc->ads_vma;
-	if (!vma) {

I believe the if was here to avoid re-allocating the vma if this function was called after a GPU reset. I agree that the check should be outside this function (and it already is), but we might want to still add here something like:

	if (WARN_ON(guc->ads_vma))
		return 0;

-		vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
-		if (IS_ERR(vma))
-			return;
+	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);

-		guc->ads_vma = vma;
-	}
+	guc->ads_vma = vma;

 	page = i915_vma_first_page(vma);
 	ads = kmap(page);
@@ -885,6 +888,13 @@ static void guc_addon_create(struct intel_guc *guc)
 			sizeof(struct guc_mmio_reg_state);

 	kunmap(page);
+
+	return 0;
+}
+
+static void guc_addon_destroy(struct intel_guc *guc)
+{
+	i915_vma_unpin_and_release(&guc->ads_vma);
 }

 /*
@@ -899,6 +909,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;
@@ -919,15 +930,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,
@@ -935,14 +954,33 @@ 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_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);

-err:
-	i915_guc_submission_fini(dev_priv);
-	return -ENOMEM;
+	return ret;
+}
+
+void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
+{
+	struct intel_guc *guc = &dev_priv->guc;
+

if I'm not missing anything, this function is called from intel_guc_fini(), which can in turn be called from the onion unwinding of i915_load_modeset_init() before intel_guc_init() is ever called, so we need to either fix that unwinding or add some kind of guard here.

+	guc_client_free(&guc->execbuf_client);
+	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)
@@ -1004,26 +1042,6 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 	intel_execlists_enable_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
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 22f882d..0b48ad8 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -489,7 +489,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)

 	err = i915_guc_submission_init(dev_priv);
 	if (err)
-		goto fail;
+		goto err_guc;

 	/*
 	 * WaEnableuKernelHeaderValidFix:skl,bxt
@@ -504,7 +504,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 		 */
 		err = guc_hw_reset(dev_priv);
 		if (err)
-			goto fail;
+			goto err_submission;

 		intel_huc_load(dev_priv);
 		err = guc_ucode_xfer(dev_priv);
@@ -512,7 +512,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 			break;

 		if (--retries == 0)
-			goto fail;
+			goto err_submission;

 		DRM_INFO("GuC fw load failed: %d; will reset and "
 			 "retry %d more time(s)\n", err, retries);
@@ -528,7 +528,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)

 		err = i915_guc_submission_enable(dev_priv);
 		if (err)
-			goto fail;
+			goto err_interrupts;
 		guc_interrupts_capture(dev_priv);
 	}

@@ -539,15 +539,16 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)

 	return 0;

+err_interrupts:
+	gen9_disable_guc_interrupts(dev_priv);
+err_submission:
+	i915_guc_submission_fini(dev_priv);
+err_guc:
+	i915_ggtt_disable_guc(dev_priv);
 fail:
 	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
 		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;

-	guc_interrupts_release(dev_priv);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-	i915_ggtt_disable_guc(dev_priv);
-
 	/*
 	 * We've failed to load the firmware :(
 	 *
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 5c0f9a4..c1365e6 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -66,7 +66,6 @@ static int guc_log_control(struct intel_guc *guc, u32 control_val)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }

-
 /*
  * Sub buffer switch callback. Called whenever relay has to switch to a new
  * sub buffer, relay stays on the same sub buffer if 0 is returned.
@@ -139,12 +138,7 @@ static int remove_buf_file_callback(struct dentry *dentry)
 	.remove_buf_file = remove_buf_file_callback,
 };

-static void guc_log_remove_relay_file(struct intel_guc *guc)
-{
-	relay_close(guc->log.relay_chan);
-}
-
-static int guc_log_create_relay_channel(struct intel_guc *guc)
+static int guc_log_relay_channel_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct rchan *guc_log_relay_chan;
@@ -172,12 +166,21 @@ static int guc_log_create_relay_channel(struct intel_guc *guc)
 	return 0;
 }

-static int guc_log_create_relay_file(struct intel_guc *guc)
+static void guc_log_relay_channel_destroy(struct intel_guc *guc)
+{
+	relay_close(guc->log.relay_chan);
+	guc->log.relay_chan = NULL;
+}
+
+static int guc_log_relay_file_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct dentry *log_dir;
 	int ret;

+	if (i915.guc_log_level < 0)
+		return 0; /* nothing to do */
+
 	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
 	log_dir = dev_priv->drm.primary->debugfs_root;

@@ -198,7 +201,10 @@ static int guc_log_create_relay_file(struct intel_guc *guc)
 	}

 	ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
-	if (ret) {
+	if (ret == -EEXIST) {
+		/* Ignore "file already exists" */
+	}
+	else if (ret < 0) {
 		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
 		return ret;
 	}
@@ -371,31 +377,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	}
 }

-static void guc_log_cleanup(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
-	lockdep_assert_held(&dev_priv->drm.struct_mutex);
-
-	/* First disable the flush interrupt */
-	gen9_disable_guc_interrupts(dev_priv);
-
-	if (guc->log.flush_wq)
-		destroy_workqueue(guc->log.flush_wq);
-
-	guc->log.flush_wq = NULL;
-
-	if (guc->log.relay_chan)
-		guc_log_remove_relay_file(guc);
-
-	guc->log.relay_chan = NULL;
-
-	if (guc->log.buf_addr)
-		i915_gem_object_unpin_map(guc->log.vma->obj);
-
-	guc->log.buf_addr = NULL;
-}
-
 static void capture_logs_work(struct work_struct *work)
 {
 	struct intel_guc *guc =
@@ -404,120 +385,84 @@ static void capture_logs_work(struct work_struct *work)
 	guc_log_capture_logs(guc);
 }

-static int guc_log_create_extras(struct intel_guc *guc)
+static int guc_log_extras_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	void *vaddr;
-	int ret;
+	int ret = 0;

 	lockdep_assert_held(&dev_priv->drm.struct_mutex);

-	/* Nothing to do */
 	if (i915.guc_log_level < 0)
-		return 0;
+		return 0; /* nothing to do */

-	if (!guc->log.buf_addr) {
-		/* Create a WC (Uncached for read) vmalloc mapping of log
-		 * buffer pages, so that we can directly get the data
-		 * (up-to-date) from memory.
-		 */
-		vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
-		if (IS_ERR(vaddr)) {
-			ret = PTR_ERR(vaddr);
-			DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
-			return ret;
-		}
+	if (guc->log.buf_addr)
+		return 0; /* already allocated */

-		guc->log.buf_addr = vaddr;
+	/* Create a WC (Uncached for read) vmalloc mapping of log
+	 * buffer pages, so that we can directly get the data
+	 * (up-to-date) from memory.
+	 */
+	vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
+	if (IS_ERR(vaddr)) {
+		DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
+		return PTR_ERR(vaddr);
 	}

-	if (!guc->log.relay_chan) {
-		/* Create a relay channel, so that we have buffers for storing
-		 * the GuC firmware logs, the channel will be linked with a file
-		 * later on when debugfs is registered.
-		 */
-		ret = guc_log_create_relay_channel(guc);
-		if (ret)
-			return ret;
-	}
+	guc->log.buf_addr = vaddr;

-	if (!guc->log.flush_wq) {
-		INIT_WORK(&guc->log.flush_work, capture_logs_work);
-
-		 /*
-		 * GuC log buffer flush work item has to do register access to
-		 * send the ack to GuC and this work item, if not synced before
-		 * suspend, can potentially get executed after the GFX device is
-		 * suspended.
-		 * By marking the WQ as freezable, we don't have to bother about
-		 * flushing of this work item from the suspend hooks, the pending
-		 * work item if any will be either executed before the suspend
-		 * or scheduled later on resume. This way the handling of work
-		 * item can be kept same between system suspend & rpm suspend.
-		 */
-		guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
-							    WQ_HIGHPRI | WQ_FREEZABLE);
-		if (guc->log.flush_wq == NULL) {
-			DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
-			return -ENOMEM;
-		}
+	/* Create a relay channel, so that we have buffers for storing
+	 * the GuC firmware logs, the channel will be linked with a file
+	 * later on when debugfs is registered.
+	 */
+	ret = guc_log_relay_channel_create(guc);
+	if (ret < 0)
+		goto err_vaddr;
+
+	INIT_WORK(&guc->log.flush_work, capture_logs_work);
+
+	 /*
+	 * GuC log buffer flush work item has to do register access to
+	 * send the ack to GuC and this work item, if not synced before
+	 * suspend, can potentially get executed after the GFX device is
+	 * suspended.
+	 * By marking the WQ as freezable, we don't have to bother about
+	 * flushing of this work item from the suspend hooks, the pending
+	 * work item if any will be either executed before the suspend
+	 * or scheduled later on resume. This way the handling of work
+	 * item can be kept same between system suspend & rpm suspend.
+	 */
+	guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
+						    WQ_HIGHPRI | WQ_FREEZABLE);
+	if (guc->log.flush_wq == NULL) {
+		DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
+		ret = -ENOMEM;
+		goto err_relaychan;
 	}

 	return 0;
+err_relaychan:
+	guc_log_relay_channel_destroy(guc);
+err_vaddr:
+	i915_gem_object_unpin_map(guc->log.vma->obj);
+

guc->log.buf_addr = NULL here would be good because you use it to check if the extras have been created.

+	return ret;
 }

-void intel_guc_log_create(struct intel_guc *guc)
+static void guc_log_extras_destroy(struct intel_guc *guc)
 {
-	struct i915_vma *vma;
-	unsigned long offset;
-	uint32_t size, flags;
-
-	if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
-		i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
-
-	/* The first page is to save log buffer state. Allocate one
-	 * extra page for others in case for overlap */
-	size = (1 + GUC_LOG_DPC_PAGES + 1 +
-		GUC_LOG_ISR_PAGES + 1 +
-		GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
-
-	vma = guc->log.vma;
-	if (!vma) {
-		/* We require SSE 4.1 for fast reads from the GuC log buffer and
-		 * it should be present on the chipsets supporting GuC based
-		 * submisssions.
-		 */
-		if (WARN_ON(!i915_has_memcpy_from_wc())) {
-			/* logging will not be enabled */
-			i915.guc_log_level = -1;
-			return;
-		}
-
-		vma = intel_guc_allocate_vma(guc, size);
-		if (IS_ERR(vma)) {
-			/* logging will be off */
-			i915.guc_log_level = -1;
-			return;
-		}
-
-		guc->log.vma = vma;
-
-		if (guc_log_create_extras(guc)) {
-			guc_log_cleanup(guc);
-			i915_vma_unpin_and_release(&guc->log.vma);
-			i915.guc_log_level = -1;
-			return;
-		}
-	}
-
-	/* each allocated unit is a page */
-	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
-		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
-		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
-		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
+	/*
+	 * It's possible that extras were never allocated because guc_log_level
+	 * was < 0 at the time
+	 **/
+	if (!guc->log.buf_addr)
+		return;

-	offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
-	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+	destroy_workqueue(guc->log.flush_wq);
+	guc->log.flush_wq = NULL;
+	guc_log_relay_channel_destroy(guc);
+	i915_gem_object_unpin_map(guc->log.vma->obj);
+	guc->log.buf_addr = NULL;
 }

 static int guc_log_late_setup(struct intel_guc *guc)
@@ -527,26 +472,25 @@ static int guc_log_late_setup(struct intel_guc *guc)

 	lockdep_assert_held(&dev_priv->drm.struct_mutex);

-	if (i915.guc_log_level < 0)
-		return -EINVAL;
-
 	/* If log_level was set as -1 at boot time, then setup needed to
 	 * handle log buffer flush interrupts would not have been done yet,
 	 * so do that now.
 	 */
-	ret = guc_log_create_extras(guc);
+	ret = guc_log_extras_create(guc);
 	if (ret)
 		goto err;

-	ret = guc_log_create_relay_file(guc);
+	ret = guc_log_relay_file_create(guc);
 	if (ret)
-		goto err;
+		goto err_extras;

 	return 0;
+err_extras:
+	guc_log_extras_destroy(guc);
 err:
-	guc_log_cleanup(guc);
 	/* logging will remain off */
 	i915.guc_log_level = -1;
+
 	return ret;
 }

@@ -586,6 +530,68 @@ static void guc_flush_logs(struct intel_guc *guc)
 	guc_log_capture_logs(guc);
 }

+int intel_guc_log_create(struct intel_guc *guc)
+{
+	struct i915_vma *vma;
+	unsigned long offset;
+	uint32_t size, flags;
+	int ret;
+

If we expect this to not be called if the log already exists, for safety we could add something like:

	if (WARN_ON(guc->log.vma))
		return 0;

To make sure we don't mess this up if we keep the object alive across resets.

+	if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
+		i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
+
+	/* The first page is to save log buffer state. Allocate one
+	 * extra page for others in case for overlap */
+	size = (1 + GUC_LOG_DPC_PAGES + 1 +
+		GUC_LOG_ISR_PAGES + 1 +
+		GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
+
+	/* We require SSE 4.1 for fast reads from the GuC log buffer and
+	 * it should be present on the chipsets supporting GuC based
+	 * submisssions.
+	 */
+	if (WARN_ON(!i915_has_memcpy_from_wc())) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	vma = intel_guc_allocate_vma(guc, size);
+	if (IS_ERR(vma)) {
+		ret = PTR_ERR(vma);
+		goto err;
+	}
+
+	guc->log.vma = vma;
+
+	ret = guc_log_extras_create(guc);
+	if (ret < 0)
+		goto err_vma;
+
+	/* each allocated unit is a page */
+	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
+		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
+		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
+		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
+
+	offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
+	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+
+	return 0;
+err_vma:
+	i915_vma_unpin_and_release(&guc->log.vma);
+err:
+	/* logging will be off */
+	i915.guc_log_level = -1;
+
+	return ret;
+}
+
+void intel_guc_log_destroy(struct intel_guc *guc)
+{
+	guc_log_extras_destroy(guc);
+	i915_vma_unpin_and_release(&guc->log.vma);
+}
+
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -609,17 +615,24 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 		return ret;
 	}

-	i915.guc_log_level = log_param.verbosity;
+	if (log_param.logging_enabled)
+	{
+		i915.guc_log_level = log_param.verbosity;

-	/* If log_level was set as -1 at boot time, then the relay channel file
-	 * wouldn't have been created by now and interrupts also would not have
-	 * been enabled.
-	 */
-	if (!dev_priv->guc.log.relay_chan) {
+		/* If log_level was set as -1 at boot time, then the relay channel file
+		 * wouldn't have been created by now and interrupts also would not have
+		 * been enabled. Try again now, just in case.
+		 */
 		ret = guc_log_late_setup(guc);
-		if (!ret)
-			gen9_enable_guc_interrupts(dev_priv);
-	} else if (!log_param.logging_enabled) {
+		if (ret < 0) {
+			DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
+			return ret;
+		}
+
+		gen9_enable_guc_interrupts(dev_priv);
+	}
+	else
+	{
 		/* Once logging is disabled, GuC won't generate logs & send an
 		 * interrupt. But there could be some data in the log buffer
 		 * which is yet to be captured. So request GuC to update the log
@@ -629,9 +642,6 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)

 		/* As logging is disabled, update log level to reflect that */
 		i915.guc_log_level = -1;
-	} else {
-		/* In case interrupts were disabled, enable them now */
-		gen9_enable_guc_interrupts(dev_priv);
 	}

 	return ret;
@@ -653,6 +663,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 		return;

 	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_log_cleanup(&dev_priv->guc);
+	gen9_disable_guc_interrupts(dev_priv);
+	intel_guc_log_destroy(&dev_priv->guc);

OCD nitpick: i915_guc_log_unregister is not simmetric with i915_guc_log_register after this change, because intel_guc_log_destroy() is destroying the vma, which was not created by guc_log_late_setup().

Thanks,
Daniele

 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 511b96b..f34448b 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -217,10 +217,11 @@ void intel_uc_fw_fetch(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)
 {

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux