[v5] drm/i915/guc: capture GuC logs if FW fails to load
diff mbox

Message ID 1494971573-11634-1-git-send-email-daniele.ceraolospurio@intel.com
State New
Headers show

Commit Message

Daniele Ceraolo Spurio May 16, 2017, 9:52 p.m. UTC
We're currently deleting the GuC logs if the FW fails to load, but those
are still useful to understand why the loading failed. Keeping the
object around allows us to access them after driver load is completed.

v2: keep the object around instead of using kernel memory (chris)
    don't store the logs in the gpu_error struct (Chris)
    add a check on guc_log_level to avoid snapshotting empty logs

v3: use separate debugfs for error log (Chris)

v4: rebased

v5: clean up obj selection, move err_load inside guc_log, move err_load
    cleanup, rename functions (Michal)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 38 +++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_guc_log.c | 17 ++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.c      | 14 +++++++++++--
 drivers/gpu/drm/i915/intel_uc.h      |  5 +++++
 4 files changed, 58 insertions(+), 16 deletions(-)

Comments

Michal Wajdeczko May 18, 2017, 5:08 p.m. UTC | #1
On Tue, May 16, 2017 at 02:52:53PM -0700, Daniele Ceraolo Spurio wrote:
> We're currently deleting the GuC logs if the FW fails to load, but those
> are still useful to understand why the loading failed. Keeping the
> object around allows us to access them after driver load is completed.
> 
> v2: keep the object around instead of using kernel memory (chris)
>     don't store the logs in the gpu_error struct (Chris)
>     add a check on guc_log_level to avoid snapshotting empty logs
> 
> v3: use separate debugfs for error log (Chris)
> 
> v4: rebased
> 
> v5: clean up obj selection, move err_load inside guc_log, move err_load
>     cleanup, rename functions (Michal)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 38 +++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_guc_log.c | 17 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.c      | 14 +++++++++++--
>  drivers/gpu/drm/i915/intel_uc.h      |  5 +++++
>  4 files changed, 58 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index bd9abef..740395c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2607,27 +2607,36 @@ static int i915_guc_stage_pool(struct seq_file *m, void *data)
>  
>  static int i915_guc_log_dump(struct seq_file *m, void *data)
>  {
> -	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> -	struct drm_i915_gem_object *obj;
> -	int i = 0, pg;
> -
> -	if (!dev_priv->guc.log.vma)
> -		return 0;
> +	struct drm_info_node *node = m->private;
> +	struct drm_i915_private *dev_priv = node_to_i915(node);
> +	bool dump_load_err = !!node->info_ent->data;
> +	struct drm_i915_gem_object *obj = NULL;
> +	u32 *log;
> +	int i = 0;
>  
> -	obj = dev_priv->guc.log.vma->obj;
> -	for (pg = 0; pg < obj->base.size / PAGE_SIZE; pg++) {
> -		u32 *log = kmap_atomic(i915_gem_object_get_page(obj, pg));
> +	if (dump_load_err)
> +		obj = dev_priv->guc.log.load_err;
> +	else if (dev_priv->guc.log.vma)
> +		obj = dev_priv->guc.log.vma->obj;
>  
> -		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));
> +	if (!obj)
> +		return 0;
>  
> -		kunmap_atomic(log);
> +	log = i915_gem_object_pin_map(obj, I915_MAP_WC);
> +	if (IS_ERR(log)) {
> +		seq_puts(m, "Failed to pin object\n");

Hmm, quite unusual to report internal message. Maybe:

	DRM_DEBUG("Failed to pin object\n");
	seq_puts(m, "(log data unaccessible)\n");


> +		return PTR_ERR(log);
>  	}
>  
> +	for (i = 0; i < obj->base.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));
> +
>  	seq_putc(m, '\n');
>  
> +	i915_gem_object_unpin_map(obj);
> +
>  	return 0;
>  }
>  
> @@ -4811,6 +4820,7 @@ static int i915_hpd_storm_ctl_open(struct inode *inode, struct file *file)
>  	{"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_guc_load_err_log_dump", i915_guc_log_dump, 0, (void *)1},

bikeshed: quite long name, maybe "i915_guc_load_log_dump" ?

>  	{"i915_guc_stage_pool", i915_guc_stage_pool, 0},
>  	{"i915_huc_load_status", i915_huc_load_status_info, 0},
>  	{"i915_frequency_info", i915_frequency_info, 0},
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 16d3b87..31e7bec 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -660,3 +660,20 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>  	guc_log_runtime_destroy(&dev_priv->guc);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  }
> +
> +void intel_guc_log_capture_load_err(struct intel_guc *guc)
> +{
> +	if (!guc->log.vma || i915.guc_log_level < 0)
> +		return;
> +
> +	if (!guc->log.load_err)
> +		guc->log.load_err = i915_gem_object_get(guc->log.vma->obj);
> +
> +	return;
> +}
> +
> +void intel_guc_log_free_load_err(struct intel_guc *guc)
> +{
> +	if (guc->log.load_err)
> +		i915_gem_object_put(guc->log.load_err);
> +}

Based on the discussion from IM, it looks that it would be better to keep
guc->load_err object directly in struct intel_guc, and at the same time
move both capture/free functions to guc_uc.c as static:

	void guc_capture_load_err_log(struct intel_guc *guc)
	void guc_free_load_err_log(struct intel_guc *guc)


> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 07c5658..dfea7d0 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -309,6 +309,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  
>  	guc_disable_communication(guc);
>  	gen9_reset_guc_interrupts(dev_priv);
> +	intel_guc_log_free_load_err(guc);

Hmm, this will free the log from previous failed load, but I was under
impression that you want to keep it... Maybe log reset should be done
as explicit .write operation ?

>  
>  	/* We need to notify the guc whenever we change the GGTT */
>  	i915_ggtt_enable_guc(dev_priv);
> @@ -355,11 +356,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  
>  	/* Did we succeded or run out of retries? */
>  	if (ret)
> -		goto err_submission;
> +		goto err_log_capture;
>  
>  	ret = guc_enable_communication(guc);
>  	if (ret)
> -		goto err_submission;
> +		goto err_log_capture;
>  
>  	intel_guc_auth_huc(dev_priv);
>  	if (i915.enable_guc_submission) {
> @@ -385,6 +386,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  err_interrupts:
>  	guc_disable_communication(guc);
>  	gen9_disable_guc_interrupts(dev_priv);
> +err_log_capture:
> +	intel_guc_log_capture_load_err(guc);
>  err_submission:
>  	if (i915.enable_guc_submission)
>  		i915_guc_submission_fini(dev_priv);
> @@ -407,6 +410,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>  {
> +	/*
> +	 * The err load log exists if guc failed to load, so we need to check
> +	 * for it before checking the module params as we might have updated
> +	 * those to reflect the load failure.

Hmm, but it looks that for now we only touch enable_guc_loading in sanitize
functions. On load failure we just disable enable_guc_submission param (which
btw is a bad decission IMHO)

> +	 */
> +	intel_guc_log_free_load_err(&dev_priv->guc);
> +
>  	if (!i915.enable_guc_loading)
>  		return;
>  
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 7618b71..05fa751 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -176,6 +176,9 @@ struct intel_guc_log {
>  	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
>  	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
>  	u32 flush_count[GUC_MAX_LOG_BUFFER];
> +
> +	/* Log snapshot if GuC errors during load */
> +	struct drm_i915_gem_object *load_err;

As mentioned above, please move it back to guc struct, but keep
close to the fw/log members.


Thanks,
Michal

>  };
>  
>  struct intel_guc {
> @@ -272,6 +275,8 @@ static inline void intel_guc_notify(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);
> +void intel_guc_log_capture_load_err(struct intel_guc *guc);
> +void intel_guc_log_free_load_err(struct intel_guc *guc);
>  
>  static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>  {
> -- 
> 1.9.1
>
Daniele Ceraolo Spurio May 18, 2017, 11:53 p.m. UTC | #2
On 18/05/17 10:08, Michal Wajdeczko wrote:
> On Tue, May 16, 2017 at 02:52:53PM -0700, Daniele Ceraolo Spurio wrote:
>> We're currently deleting the GuC logs if the FW fails to load, but those
>> are still useful to understand why the loading failed. Keeping the
>> object around allows us to access them after driver load is completed.
>>
>> v2: keep the object around instead of using kernel memory (chris)
>>     don't store the logs in the gpu_error struct (Chris)
>>     add a check on guc_log_level to avoid snapshotting empty logs
>>
>> v3: use separate debugfs for error log (Chris)
>>
>> v4: rebased
>>
>> v5: clean up obj selection, move err_load inside guc_log, move err_load
>>     cleanup, rename functions (Michal)
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c  | 38 +++++++++++++++++++++++-------------
>>  drivers/gpu/drm/i915/intel_guc_log.c | 17 ++++++++++++++++
>>  drivers/gpu/drm/i915/intel_uc.c      | 14 +++++++++++--
>>  drivers/gpu/drm/i915/intel_uc.h      |  5 +++++
>>  4 files changed, 58 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index bd9abef..740395c 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2607,27 +2607,36 @@ static int i915_guc_stage_pool(struct seq_file *m, void *data)
>>
>>  static int i915_guc_log_dump(struct seq_file *m, void *data)
>>  {
>> -	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>> -	struct drm_i915_gem_object *obj;
>> -	int i = 0, pg;
>> -
>> -	if (!dev_priv->guc.log.vma)
>> -		return 0;
>> +	struct drm_info_node *node = m->private;
>> +	struct drm_i915_private *dev_priv = node_to_i915(node);
>> +	bool dump_load_err = !!node->info_ent->data;
>> +	struct drm_i915_gem_object *obj = NULL;
>> +	u32 *log;
>> +	int i = 0;
>>
>> -	obj = dev_priv->guc.log.vma->obj;
>> -	for (pg = 0; pg < obj->base.size / PAGE_SIZE; pg++) {
>> -		u32 *log = kmap_atomic(i915_gem_object_get_page(obj, pg));
>> +	if (dump_load_err)
>> +		obj = dev_priv->guc.log.load_err;
>> +	else if (dev_priv->guc.log.vma)
>> +		obj = dev_priv->guc.log.vma->obj;
>>
>> -		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));
>> +	if (!obj)
>> +		return 0;
>>
>> -		kunmap_atomic(log);
>> +	log = i915_gem_object_pin_map(obj, I915_MAP_WC);
>> +	if (IS_ERR(log)) {
>> +		seq_puts(m, "Failed to pin object\n");
>
> Hmm, quite unusual to report internal message. Maybe:
>
> 	DRM_DEBUG("Failed to pin object\n");
> 	seq_puts(m, "(log data unaccessible)\n");
>
>
>> +		return PTR_ERR(log);
>>  	}
>>
>> +	for (i = 0; i < obj->base.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));
>> +
>>  	seq_putc(m, '\n');
>>
>> +	i915_gem_object_unpin_map(obj);
>> +
>>  	return 0;
>>  }
>>
>> @@ -4811,6 +4820,7 @@ static int i915_hpd_storm_ctl_open(struct inode *inode, struct file *file)
>>  	{"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_guc_load_err_log_dump", i915_guc_log_dump, 0, (void *)1},
>
> bikeshed: quite long name, maybe "i915_guc_load_log_dump" ?

I'd prefer to keep it as it is to make it explicit that it is an error log.

>
>>  	{"i915_guc_stage_pool", i915_guc_stage_pool, 0},
>>  	{"i915_huc_load_status", i915_huc_load_status_info, 0},
>>  	{"i915_frequency_info", i915_frequency_info, 0},
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 16d3b87..31e7bec 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -660,3 +660,20 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>>  	guc_log_runtime_destroy(&dev_priv->guc);
>>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>>  }
>> +
>> +void intel_guc_log_capture_load_err(struct intel_guc *guc)
>> +{
>> +	if (!guc->log.vma || i915.guc_log_level < 0)
>> +		return;
>> +
>> +	if (!guc->log.load_err)
>> +		guc->log.load_err = i915_gem_object_get(guc->log.vma->obj);
>> +
>> +	return;
>> +}
>> +
>> +void intel_guc_log_free_load_err(struct intel_guc *guc)
>> +{
>> +	if (guc->log.load_err)
>> +		i915_gem_object_put(guc->log.load_err);
>> +}
>
> Based on the discussion from IM, it looks that it would be better to keep
> guc->load_err object directly in struct intel_guc, and at the same time
> move both capture/free functions to guc_uc.c as static:
>
> 	void guc_capture_load_err_log(struct intel_guc *guc)
> 	void guc_free_load_err_log(struct intel_guc *guc)
>
>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
>> index 07c5658..dfea7d0 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -309,6 +309,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>
>>  	guc_disable_communication(guc);
>>  	gen9_reset_guc_interrupts(dev_priv);
>> +	intel_guc_log_free_load_err(guc);
>
> Hmm, this will free the log from previous failed load, but I was under
> impression that you want to keep it... Maybe log reset should be done
> as explicit .write operation ?
>

True, I've removed it for now. New load attempts after the first set are 
not very interesting and the process is a bit weird at the moment 
because we update one of the 2 enable_guc_* parameters on load failure 
but not the other. I'd prefer to consider only the first set of load 
attempts until we clean all of that up unless you feel strongly about this.

>>
>>  	/* We need to notify the guc whenever we change the GGTT */
>>  	i915_ggtt_enable_guc(dev_priv);
>> @@ -355,11 +356,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>
>>  	/* Did we succeded or run out of retries? */
>>  	if (ret)
>> -		goto err_submission;
>> +		goto err_log_capture;
>>
>>  	ret = guc_enable_communication(guc);
>>  	if (ret)
>> -		goto err_submission;
>> +		goto err_log_capture;
>>
>>  	intel_guc_auth_huc(dev_priv);
>>  	if (i915.enable_guc_submission) {
>> @@ -385,6 +386,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>  err_interrupts:
>>  	guc_disable_communication(guc);
>>  	gen9_disable_guc_interrupts(dev_priv);
>> +err_log_capture:
>> +	intel_guc_log_capture_load_err(guc);
>>  err_submission:
>>  	if (i915.enable_guc_submission)
>>  		i915_guc_submission_fini(dev_priv);
>> @@ -407,6 +410,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>
>>  void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>>  {
>> +	/*
>> +	 * The err load log exists if guc failed to load, so we need to check
>> +	 * for it before checking the module params as we might have updated
>> +	 * those to reflect the load failure.
>
> Hmm, but it looks that for now we only touch enable_guc_loading in sanitize
> functions. On load failure we just disable enable_guc_submission param (which
> btw is a bad decission IMHO)
>

I was trying to be a bit future-proof here because Oscar's change in 
review changes the behavior slightly (enable_guc_loading is replaced by 
enable_guc_submission || HAS_HUC_FW) and we could have cases where this 
is false if that goes through. I'll move it further down and keep and 
eye on that patch to make sure we don't end up leaking stuff.

Thanks,
Daniele

>> +	 */
>> +	intel_guc_log_free_load_err(&dev_priv->guc);
>> +
>>  	if (!i915.enable_guc_loading)
>>  		return;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
>> index 7618b71..05fa751 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -176,6 +176,9 @@ struct intel_guc_log {
>>  	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
>>  	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
>>  	u32 flush_count[GUC_MAX_LOG_BUFFER];
>> +
>> +	/* Log snapshot if GuC errors during load */
>> +	struct drm_i915_gem_object *load_err;
>
> As mentioned above, please move it back to guc struct, but keep
> close to the fw/log members.
>
>
> Thanks,
> Michal
>
>>  };
>>
>>  struct intel_guc {
>> @@ -272,6 +275,8 @@ static inline void intel_guc_notify(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);
>> +void intel_guc_log_capture_load_err(struct intel_guc *guc);
>> +void intel_guc_log_free_load_err(struct intel_guc *guc);
>>
>>  static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>>  {
>> --
>> 1.9.1
>>
Daniele Ceraolo Spurio May 22, 2017, 6:40 p.m. UTC | #3
On 22/05/17 11:21, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915/guc: capture GuC logs if FW fails to load (rev6)
> URL   : https://patchwork.freedesktop.org/series/23982/
> State : warning
>
> == Summary ==
>
> Series 23982v6 drm/i915/guc: capture GuC logs if FW fails to load
> https://patchwork.freedesktop.org/api/1.0/series/23982/revisions/6/mbox/
>
> Test gem_exec_suspend:
>         Subgroup basic-s4-devices:
>                 pass       -> DMESG-WARN (fi-snb-2600)

this is: https://bugs.freedesktop.org/show_bug.cgi?id=100125

Regards,
Daniele

> Test kms_flip:
>         Subgroup basic-flip-vs-dpms:
>                 pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144
>         Subgroup basic-flip-vs-wf_vblank:
>                 fail       -> PASS       (fi-skl-6770hq) fdo#99739
>
> fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
> fdo#99739 https://bugs.freedesktop.org/show_bug.cgi?id=99739
>
> fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:451s
> fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:434s
> fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:582s
> fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:519s
> fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:494s
> fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:487s
> fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:415s
> fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:412s
> fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:419s
> fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:495s
> fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:467s
> fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:463s
> fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:573s
> fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:457s
> fi-skl-6700hq    total:278  pass:260  dwarn:1   dfail:0   fail:0   skip:17  time:581s
> fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:463s
> fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:501s
> fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:440s
> fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:532s
> fi-snb-2600      total:278  pass:248  dwarn:1   dfail:0   fail:0   skip:29  time:416s
>
> 5c5ce2ab8183e4bd85a603169e260cc938837e2c drm-tip: 2017y-05m-22d-13h-42m-03s UTC integration manifest
> 8ddb7a7 drm/i915/guc: capture GuC logs if FW fails to load
>
> == Logs ==
>
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4774/
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index bd9abef..740395c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2607,27 +2607,36 @@  static int i915_guc_stage_pool(struct seq_file *m, void *data)
 
 static int i915_guc_log_dump(struct seq_file *m, void *data)
 {
-	struct drm_i915_private *dev_priv = node_to_i915(m->private);
-	struct drm_i915_gem_object *obj;
-	int i = 0, pg;
-
-	if (!dev_priv->guc.log.vma)
-		return 0;
+	struct drm_info_node *node = m->private;
+	struct drm_i915_private *dev_priv = node_to_i915(node);
+	bool dump_load_err = !!node->info_ent->data;
+	struct drm_i915_gem_object *obj = NULL;
+	u32 *log;
+	int i = 0;
 
-	obj = dev_priv->guc.log.vma->obj;
-	for (pg = 0; pg < obj->base.size / PAGE_SIZE; pg++) {
-		u32 *log = kmap_atomic(i915_gem_object_get_page(obj, pg));
+	if (dump_load_err)
+		obj = dev_priv->guc.log.load_err;
+	else if (dev_priv->guc.log.vma)
+		obj = dev_priv->guc.log.vma->obj;
 
-		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));
+	if (!obj)
+		return 0;
 
-		kunmap_atomic(log);
+	log = i915_gem_object_pin_map(obj, I915_MAP_WC);
+	if (IS_ERR(log)) {
+		seq_puts(m, "Failed to pin object\n");
+		return PTR_ERR(log);
 	}
 
+	for (i = 0; i < obj->base.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));
+
 	seq_putc(m, '\n');
 
+	i915_gem_object_unpin_map(obj);
+
 	return 0;
 }
 
@@ -4811,6 +4820,7 @@  static int i915_hpd_storm_ctl_open(struct inode *inode, struct file *file)
 	{"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_guc_load_err_log_dump", i915_guc_log_dump, 0, (void *)1},
 	{"i915_guc_stage_pool", i915_guc_stage_pool, 0},
 	{"i915_huc_load_status", i915_huc_load_status_info, 0},
 	{"i915_frequency_info", i915_frequency_info, 0},
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 16d3b87..31e7bec 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -660,3 +660,20 @@  void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 	guc_log_runtime_destroy(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
+
+void intel_guc_log_capture_load_err(struct intel_guc *guc)
+{
+	if (!guc->log.vma || i915.guc_log_level < 0)
+		return;
+
+	if (!guc->log.load_err)
+		guc->log.load_err = i915_gem_object_get(guc->log.vma->obj);
+
+	return;
+}
+
+void intel_guc_log_free_load_err(struct intel_guc *guc)
+{
+	if (guc->log.load_err)
+		i915_gem_object_put(guc->log.load_err);
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 07c5658..dfea7d0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -309,6 +309,7 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 	guc_disable_communication(guc);
 	gen9_reset_guc_interrupts(dev_priv);
+	intel_guc_log_free_load_err(guc);
 
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
@@ -355,11 +356,11 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 	/* Did we succeded or run out of retries? */
 	if (ret)
-		goto err_submission;
+		goto err_log_capture;
 
 	ret = guc_enable_communication(guc);
 	if (ret)
-		goto err_submission;
+		goto err_log_capture;
 
 	intel_guc_auth_huc(dev_priv);
 	if (i915.enable_guc_submission) {
@@ -385,6 +386,8 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_interrupts:
 	guc_disable_communication(guc);
 	gen9_disable_guc_interrupts(dev_priv);
+err_log_capture:
+	intel_guc_log_capture_load_err(guc);
 err_submission:
 	if (i915.enable_guc_submission)
 		i915_guc_submission_fini(dev_priv);
@@ -407,6 +410,13 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 {
+	/*
+	 * The err load log exists if guc failed to load, so we need to check
+	 * for it before checking the module params as we might have updated
+	 * those to reflect the load failure.
+	 */
+	intel_guc_log_free_load_err(&dev_priv->guc);
+
 	if (!i915.enable_guc_loading)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7618b71..05fa751 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -176,6 +176,9 @@  struct intel_guc_log {
 	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
 	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
 	u32 flush_count[GUC_MAX_LOG_BUFFER];
+
+	/* Log snapshot if GuC errors during load */
+	struct drm_i915_gem_object *load_err;
 };
 
 struct intel_guc {
@@ -272,6 +275,8 @@  static inline void intel_guc_notify(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);
+void intel_guc_log_capture_load_err(struct intel_guc *guc);
+void intel_guc_log_free_load_err(struct intel_guc *guc);
 
 static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 {