diff mbox

[v6,13/14] drm/i915: Enable GuC firmware log

Message ID 1430345615-5576-14-git-send-email-yu.dai@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

yu.dai@intel.com April 29, 2015, 10:13 p.m. UTC
From: Alex Dai <yu.dai@intel.com>

Allocate a gem obj to hold GuC log data. Also a debugfs interface
(i915_guc_log_dump) is provided to print out the log content.

Issue: VIZ-4884
Signed-off-by: Alex Dai <yu.dai@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 41 +++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/i915_params.c      |  5 +++
 drivers/gpu/drm/i915/intel_guc.h        |  1 +
 drivers/gpu/drm/i915/intel_guc_loader.c | 64 ++++++++++++++++++++++++++++++++-
 5 files changed, 104 insertions(+), 8 deletions(-)

Comments

Dave Gordon May 1, 2015, 5:48 p.m. UTC | #1
On 29/04/15 23:13, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
> 
> Allocate a gem obj to hold GuC log data. Also a debugfs interface
> (i915_guc_log_dump) is provided to print out the log content.
> 
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     | 41 +++++++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/i915_params.c      |  5 +++
>  drivers/gpu/drm/i915/intel_guc.h        |  1 +
>  drivers/gpu/drm/i915/intel_guc_loader.c | 64 ++++++++++++++++++++++++++++++++-
>  5 files changed, 104 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f12bbee..f47714c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2332,14 +2332,14 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
>  
>  	tmp = I915_READ(GUC_STATUS);
>  
> -	seq_puts(m, "\nResponse from GuC:\n");
> +	seq_printf(m, "\nGuC status 0x%08x:\n", tmp);
>  	seq_printf(m, "\tBootrom status = 0x%x\n",
>  		(tmp & GS_BOOTROM_MASK) >> GS_BOOTROM_SHIFT);
>  	seq_printf(m, "\tuKernel status = 0x%x\n",
>  		(tmp & GS_UKERNEL_MASK) >> GS_UKERNEL_SHIFT);
>  	seq_printf(m, "\tMIA Core status = 0x%x\n",
>  		(tmp & GS_MIA_MASK) >> GS_MIA_SHIFT);
> -	seq_puts(m, "Scratch registers value:\n");
> +	seq_puts(m, "\nScratch registers value:\n");
>  	for (i = 0; i < 16; i++)
>  		seq_printf(m, "\t%2d: \t0x%x\n", i, I915_READ(SOFT_SCRATCH(i)));
>  
> @@ -2352,13 +2352,11 @@ static int i915_guc_info(struct seq_file *m, void *data)
>  	struct drm_device *dev = node->minor->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_guc guc;
> -	struct i915_guc_client client;
> +	struct i915_guc_client client = { 0 };

This line gives a warning, because the preceding patch [12/14],
added a new first member which is not a scalar, so can't be initialised
with a zero.

> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 892f974..f8065cf 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -31,6 +31,7 @@
>  #define GUC_WQ_SIZE	(PAGE_SIZE * 2)
>  
>  struct i915_guc_client {
> +	spinlock_t wq_lock;
>  	struct drm_i915_gem_object *client_obj;
>  	u32 priority;
>  	off_t doorbell_offset;

GCC allows '{}' as a set-everything-to-default initialiser, but kernel
coding standards may not. Or, we could put in an explicit member name, i.e.

+	struct i915_guc_client client = { .client_obj = 0 };

But in any case I was thinking of reordering the i915_guc_client
structure members for better packing, and/or better logical grouping
and/or ordering. So the client_obj member may well end up at the top,
and then the struct initialisation will work as-is :)

.Dave.
Dave Gordon May 5, 2015, 12:45 p.m. UTC | #2
On 29/04/15 23:13, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
> 
> Allocate a gem obj to hold GuC log data. Also a debugfs interface
> (i915_guc_log_dump) is provided to print out the log content.
> 
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     | 41 +++++++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/i915_params.c      |  5 +++
>  drivers/gpu/drm/i915/intel_guc.h        |  1 +
>  drivers/gpu/drm/i915/intel_guc_loader.c | 64 ++++++++++++++++++++++++++++++++-
>  5 files changed, 104 insertions(+), 8 deletions(-)

[snip]

> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 9ad2e27..95e4eb7 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -54,6 +54,7 @@ struct i915_params i915 __read_mostly = {
>  	.verbose_state_checks = 1,
>  	.nuclear_pageflip = 0,
>  	.enable_guc_scheduling = false,
> +	.guc_log_level = 0,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -188,3 +189,7 @@ MODULE_PARM_DESC(nuclear_pageflip,
>  
>  module_param_named(enable_guc_scheduling, i915.enable_guc_scheduling, bool, 0400);
>  MODULE_PARM_DESC(enable_guc_scheduling, "Enable GuC scheduling (default:false)");
> +
> +module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
> +MODULE_PARM_DESC(guc_log_level,
> +	"GuC firmware logging level (0:disabled, 1~4:enabled)");

See below ...

> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index eb25cfb..a61d1df 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -51,6 +51,12 @@
>   * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
>   * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
>   *
> + * Firmware log:
> + * Firmware log is enabled by setting i915.guc_log_level to non-negative level.

This comment doesn't agree with the one above ...

> +static void create_guc_log(struct intel_guc *guc, u32 *params)
> +{
> +	struct drm_i915_private *dev_priv =
> +			container_of(guc, struct drm_i915_private, guc);
> +	struct drm_i915_gem_object *obj;
> +	u32 flags, size;
> +
> +	/* 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;
> +
> +	if (!guc->log_obj) {
> +		obj = intel_guc_allocate_gem_obj(dev_priv->dev, size);
> +		if (!obj) {
> +			/* logging will be off */
> +			*(params + GUC_CTL_LOG_PARAMS) = 0;
> +			i915.guc_log_level = 0;
> +			return;
> +		}
> +
> +		guc->log_obj = obj;
> +	}
> +	else
> +		obj = guc->log_obj;
> +
> +	/* 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);
> +
> +	size = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT; /* in pages */
> +	flags |= size << GUC_LOG_BUF_ADDR_SHIFT;
> +
> +	*(params + GUC_CTL_LOG_PARAMS) = flags;
> +
> +	i915.guc_log_level--;

This code modifies the global parameter, which will result in
(a) weird results if this code can be executed more than once
(b) confusion is the user reads it back and finds that it's
    not the same as given on the command line!

> +	if (i915.guc_log_level > GUC_LOG_VERBOSITY_ULTRA)
> +		i915.guc_log_level = GUC_LOG_VERBOSITY_ULTRA;
> +
> +	*(params + GUC_CTL_DEBUG) |= i915.guc_log_level;

These lines only happen to work because the VERBOSITY_SHIFT is 0.

Given the opportunities for confusion here, I think it would be better
to redefine the module parameter as signed, with -1 => disabled, and 0-3
for the levels of verbosity. This will mean the values used for the
module parameter will be the same as described in the GuC firmware
specification (i.e. verbosity levels of 0-3).

Then, the validation code above can legitimately limit the range to
(negative => off, 0-3 => level, 4+ => clipped to 3), and that will not
surprise the user too much when they read it back (it will be as they
set it, unless it was too large, in which case it shows what the maximum
useful value is).

With the parameter defined this way, we can add #defines for min and max
verbosity (0 and 3, without incorporating the shift left), and then
apply the shift only when setting the host-to-GuC-interface parameter,
rather than worrying about it when dealing with the user parameter.

>  static void set_guc_init_params(struct drm_i915_private *dev_priv)
>  {
>  	u32 params[GUC_CTL_MAX_DWORDS];
> @@ -227,7 +282,9 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
>  	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
>  			GUC_CTL_VCS2_ENABLED;
>  
> -	/* XXX: Set up log buffer */
> +	/* Set up log buffer */
> +	if (i915.guc_log_level > 0)
> +		create_guc_log(&dev_priv->guc, params);

I would rather this was done during initial setup i.e. in the same place
as allocating the context pool and the execbuf client, rather than in
the middle of setting up the parameters. We can stash the values that
would have been passed back through params[] in the struct guc, so this
code can then just copy then to the parameter block.

.Dave.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f12bbee..f47714c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2332,14 +2332,14 @@  static int i915_guc_load_status_info(struct seq_file *m, void *data)
 
 	tmp = I915_READ(GUC_STATUS);
 
-	seq_puts(m, "\nResponse from GuC:\n");
+	seq_printf(m, "\nGuC status 0x%08x:\n", tmp);
 	seq_printf(m, "\tBootrom status = 0x%x\n",
 		(tmp & GS_BOOTROM_MASK) >> GS_BOOTROM_SHIFT);
 	seq_printf(m, "\tuKernel status = 0x%x\n",
 		(tmp & GS_UKERNEL_MASK) >> GS_UKERNEL_SHIFT);
 	seq_printf(m, "\tMIA Core status = 0x%x\n",
 		(tmp & GS_MIA_MASK) >> GS_MIA_SHIFT);
-	seq_puts(m, "Scratch registers value:\n");
+	seq_puts(m, "\nScratch registers value:\n");
 	for (i = 0; i < 16; i++)
 		seq_printf(m, "\t%2d: \t0x%x\n", i, I915_READ(SOFT_SCRATCH(i)));
 
@@ -2352,13 +2352,11 @@  static int i915_guc_info(struct seq_file *m, void *data)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc guc;
-	struct i915_guc_client client;
+	struct i915_guc_client client = { 0 };
 
-	if (!i915.enable_guc_scheduling)
+	if (!HAS_GUC_SCHED(dev_priv->dev))
 		return 0;
 
-	memset(&client, 0, sizeof(struct i915_guc_client));
-
 	/* Take a local copy of the GuC data, so we can dump it at leisure */
 	spin_lock(&dev_priv->guc.host2guc_lock);
 	guc = dev_priv->guc;
@@ -2376,7 +2374,7 @@  static int i915_guc_info(struct seq_file *m, void *data)
 	seq_printf(m, "GuC action failure count: %u\n", guc.action_fail);
 	seq_printf(m, "GuC last action error code: %d\n", guc.action_err);
 
-	seq_printf(m, "GuC execbuf client @ %p:\n", guc.execbuf_client);
+	seq_printf(m, "\nGuC execbuf client @ %p:\n", guc.execbuf_client);
 	seq_printf(m, "\tTotal submissions: %llu\n", client.submissions);
 	seq_printf(m, "\tFailed to queue: %u\n", client.q_fail);
 	seq_printf(m, "\tFailed doorbell: %u\n", client.b_fail);
@@ -2387,6 +2385,34 @@  static int i915_guc_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_guc_log_dump(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *log_obj = dev_priv->guc.log_obj;
+	u32 *log;
+	int i = 0, pg;
+
+	if (!log_obj)
+		return 0;
+
+	for (pg = 0; pg < log_obj->base.size / PAGE_SIZE; pg++) {
+		log = kmap_atomic(i915_gem_object_get_page(log_obj, pg));
+
+		for (i = 0; i < PAGE_SIZE / sizeof(u32); i += 4)
+			seq_printf(m, "0x%08x 0x%08x 0x%08x 0x%08x\n",
+				   *(log + i), *(log + i + 1),
+				   *(log + i + 2), *(log + i + 3));
+
+		kunmap_atomic(log);
+	}
+
+	seq_putc(m, '\n');
+
+	return 0;
+}
+
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -4855,6 +4881,7 @@  static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_gem_batch_pool", i915_gem_batch_pool_info, 0},
 	{"i915_guc_info", i915_guc_info, 0},
 	{"i915_guc_load_status", i915_guc_load_status_info, 0},
+	{"i915_guc_log_dump", i915_guc_log_dump, 0},
 	{"i915_frequency_info", i915_frequency_info, 0},
 	{"i915_hangcheck_info", i915_hangcheck_info, 0},
 	{"i915_drpc_info", i915_drpc_info, 0},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0d60119..aa3d81b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2505,6 +2505,7 @@  struct i915_params {
 	bool disable_display;
 	bool disable_vtd_wa;
 	bool enable_guc_scheduling;
+	unsigned int guc_log_level;
 	int use_mmio_flip;
 	int mmio_debug;
 	bool verbose_state_checks;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 9ad2e27..95e4eb7 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -54,6 +54,7 @@  struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.enable_guc_scheduling = false,
+	.guc_log_level = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -188,3 +189,7 @@  MODULE_PARM_DESC(nuclear_pageflip,
 
 module_param_named(enable_guc_scheduling, i915.enable_guc_scheduling, bool, 0400);
 MODULE_PARM_DESC(enable_guc_scheduling, "Enable GuC scheduling (default:false)");
+
+module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
+MODULE_PARM_DESC(guc_log_level,
+	"GuC firmware logging level (0:disabled, 1~4:enabled)");
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index b096d1a..3673511 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -66,6 +66,7 @@  struct intel_guc {
 	spinlock_t host2guc_lock;
 
 	struct drm_i915_gem_object *ctx_pool_obj;
+	struct drm_i915_gem_object *log_obj;
 
 	struct i915_guc_client *execbuf_client;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index eb25cfb..a61d1df 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -51,6 +51,12 @@ 
  * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
  * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
  *
+ * Firmware log:
+ * Firmware log is enabled by setting i915.guc_log_level to non-negative level.
+ * Log data is printed out via reading debugfs i915_guc_log_dump. Reading from
+ * i915_guc_load_status will print out firmware loading status and scratch
+ * registers value.
+ *
  */
 
 #define I915_SKL_GUC_UCODE "i915/skl_guc_ver1.bin"
@@ -58,6 +64,10 @@  MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
 #define I915_BXT_GUC_UCODE "i915/bxt_guc_ver1.bin"
 MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
 
+#define GUC_LOG_DPC_PAGES	3
+#define GUC_LOG_ISR_PAGES	3
+#define GUC_LOG_CRASH_PAGES	1
+
 /**
  * intel_guc_allocate_gem_obj() - Allocate gem object for GuC usage
  * @dev:	drm device
@@ -204,6 +214,51 @@  static u32 get_core_family(struct drm_device *dev)
 	}
 }
 
+static void create_guc_log(struct intel_guc *guc, u32 *params)
+{
+	struct drm_i915_private *dev_priv =
+			container_of(guc, struct drm_i915_private, guc);
+	struct drm_i915_gem_object *obj;
+	u32 flags, size;
+
+	/* 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;
+
+	if (!guc->log_obj) {
+		obj = intel_guc_allocate_gem_obj(dev_priv->dev, size);
+		if (!obj) {
+			/* logging will be off */
+			*(params + GUC_CTL_LOG_PARAMS) = 0;
+			i915.guc_log_level = 0;
+			return;
+		}
+
+		guc->log_obj = obj;
+	}
+	else
+		obj = guc->log_obj;
+
+	/* 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);
+
+	size = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT; /* in pages */
+	flags |= size << GUC_LOG_BUF_ADDR_SHIFT;
+
+	*(params + GUC_CTL_LOG_PARAMS) = flags;
+
+	i915.guc_log_level--;
+	if (i915.guc_log_level > GUC_LOG_VERBOSITY_ULTRA)
+		i915.guc_log_level = GUC_LOG_VERBOSITY_ULTRA;
+
+	*(params + GUC_CTL_DEBUG) |= i915.guc_log_level;
+}
+
 static void set_guc_init_params(struct drm_i915_private *dev_priv)
 {
 	u32 params[GUC_CTL_MAX_DWORDS];
@@ -227,7 +282,9 @@  static void set_guc_init_params(struct drm_i915_private *dev_priv)
 	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
 			GUC_CTL_VCS2_ENABLED;
 
-	/* XXX: Set up log buffer */
+	/* Set up log buffer */
+	if (i915.guc_log_level > 0)
+		create_guc_log(&dev_priv->guc, params);
 
 	/* If GuC scheduling is enabled, setup params here. */
 	if (i915.enable_guc_scheduling) {
@@ -476,6 +533,11 @@  void intel_guc_ucode_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
 
+	if (dev_priv->guc.log_obj) {
+		intel_guc_release_gem_obj(dev_priv->guc.log_obj);
+		dev_priv->guc.log_obj = NULL;
+	}
+
 	guc_scheduler_fini(dev);
 
 	intel_uc_fw_fini(guc_fw);