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

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

 



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@xxxxxxxxxxxxxxxxxx>
> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> ---
>  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
> 
_______________________________________________
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