diff mbox

[v5,2/2] drm/i915: Make GuC log part of the uC error state

Message ID 20171025104112.34476-2-michal.wajdeczko@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michal Wajdeczko Oct. 25, 2017, 10:41 a.m. UTC 
We keep details of GuC and HuC in separate error state struct.
Make GuC log part of it to group all related data together.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c | 18 +++---------------
 2 files changed, 4 insertions(+), 16 deletions(-)

Comments

Chris Wilson Oct. 25, 2017, 11:11 a.m. UTC | #1 
Quoting Michal Wajdeczko (2017-10-25 11:41:12)
> We keep details of GuC and HuC in separate error state struct.
> Make GuC log part of it to group all related data together.

The only significant change would appear to placement in error-state; it
now comes at the end. Maybe that's ok; what do we feel is the relevance
of the guc log vs the dumping of user buffers? Do we want to read the
log first, or are we likely to skip to the userstate and then jump back?

At least the consideration is worthy of comment in the changelog.
-Chris
Michal Wajdeczko Oct. 26, 2017, 4:46 p.m. UTC | #2 
On Wed, 25 Oct 2017 13:11:25 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-10-25 11:41:12)
>> We keep details of GuC and HuC in separate error state struct.
>> Make GuC log part of it to group all related data together.
>
> The only significant change would appear to placement in error-state; it
> now comes at the end. Maybe that's ok; what do we feel is the relevance
> of the guc log vs the dumping of user buffers? Do we want to read the
> log first, or are we likely to skip to the userstate and then jump back?

I would expect data in error-state to be grouped always per "source" to
easy track what is available from it. Also ordering of these sources can
be based on the order driver is using/creating them (ie. pciid, modparams,
device flags, uc, engines, buffers... - which is different than today's)

>
> At least the consideration is worthy of comment in the changelog.

Ok, I'll add small note.

Michal
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a17a68f..41f8b7b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -914,6 +914,7 @@  struct i915_gpu_state {
 	struct i915_error_uc {
 		struct intel_uc_fw guc_fw;
 		struct intel_uc_fw huc_fw;
+		struct drm_i915_error_object *guc_log;
 	} uc;
 
 	/* Generic register state */
@@ -939,7 +940,6 @@  struct i915_gpu_state {
 	struct intel_overlay_error_state *overlay;
 	struct intel_display_error_state *display;
 	struct drm_i915_error_object *semaphore;
-	struct drm_i915_error_object *guc_log;
 
 	struct drm_i915_error_engine {
 		int engine_id;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 238fb54..c2d3e8e 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -618,6 +618,7 @@  static void err_print_uc(struct drm_i915_error_state_buf *m,
 
 	intel_uc_fw_dump(&error_uc->guc_fw, &p);
 	intel_uc_fw_dump(&error_uc->huc_fw, &p);
+	print_error_obj(m, NULL, "GuC log buffer", error_uc->guc_log);
 }
 
 int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
@@ -795,8 +796,6 @@  int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 
 	print_error_obj(m, NULL, "Semaphores", error->semaphore);
 
-	print_error_obj(m, NULL, "GuC log buffer", error->guc_log);
-
 	if (error->overlay)
 		intel_overlay_print_error_state(m, error->overlay);
 
@@ -868,6 +867,7 @@  static void cleanup_uc_state(const struct i915_error_uc *error_uc)
 {
 	kfree(error_uc->guc_fw.path);
 	kfree(error_uc->huc_fw.path);
+	i915_error_object_free(error_uc->guc_log);
 }
 
 void __i915_gpu_state_free(struct kref *error_ref)
@@ -896,7 +896,6 @@  void __i915_gpu_state_free(struct kref *error_ref)
 	}
 
 	i915_error_object_free(error->semaphore);
-	i915_error_object_free(error->guc_log);
 
 	for (i = 0; i < ARRAY_SIZE(error->active_bo); i++)
 		kfree(error->active_bo[i]);
@@ -1618,17 +1617,7 @@  static void capture_uc_state(struct i915_gpu_state *error)
 	 */
 	error_uc->guc_fw.path = kstrdup(i915->guc.fw.path, GFP_ATOMIC);
 	error_uc->huc_fw.path = kstrdup(i915->huc.fw.path, GFP_ATOMIC);
-}
-
-static void i915_gem_capture_guc_log_buffer(struct drm_i915_private *dev_priv,
-					    struct i915_gpu_state *error)
-{
-	/* Capturing log buf contents won't be useful if logging was disabled */
-	if (!dev_priv->guc.log.vma || (i915_modparams.guc_log_level < 0))
-		return;
-
-	error->guc_log = i915_error_object_create(dev_priv,
-						  dev_priv->guc.log.vma);
+	error_uc->guc_log = i915_error_object_create(i915, i915->guc.log.vma);
 }
 
 /* Capture all registers which don't fit into another category. */
@@ -1779,7 +1768,6 @@  static int capture(void *data)
 	i915_gem_record_rings(error->i915, error);
 	i915_capture_active_buffers(error->i915, error);
 	i915_capture_pinned_buffers(error->i915, error);
-	i915_gem_capture_guc_log_buffer(error->i915, error);
 
 	error->overlay = intel_overlay_capture_error_state(error->i915);
 	error->display = intel_display_capture_error_state(error->i915);