Re: [Intel-gfx] [PATCH v6] drm/i915/guc: capture GuC logs if FW fails to load

[Chris Wilson]

On Thu, May 25, 2017 at 11:58:25AM -0700, Michel Thierry wrote:
> On 25/05/17 08:31, Daniele Ceraolo Spurio wrote:
> >
> >
> >On 22/05/17 12:56, Michal Wajdeczko wrote:
> >>On Mon, May 22, 2017 at 10:50:28AM -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)
> >>>
> >>>v6: move obj back to intel_guc, move functions to intel_uc.c, don't
> >>>clear obj on new GuC load, free object only if enable_guc_loading
> >>>is set (Michal)
> >>>
> >>>Cc: Chris Wilson 
> >>>Cc: Oscar Mateo 
> >>>Cc: Michal Wajdeczko 
> >>>Signed-off-by: Daniele Ceraolo Spurio 
> >>>---
> >>
> >>Reviewed-by: Michal Wajdeczko 
> >>
> >>Regards,
> >>Michal
> >>
> >
> >Does anyone else have any concern or can this patch be merged?
> >
> 
> +1 about being merged, and since I've been using this patch you could say,

And done. Thanks for the patch, testing and review.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6] drm/i915/guc: capture GuC logs if FW fails to load

[Michel Thierry]

On 25/05/17 08:31, Daniele Ceraolo Spurio wrote:



On 22/05/17 12:56, Michal Wajdeczko wrote:

On Mon, May 22, 2017 at 10:50:28AM -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)

v6: move obj back to intel_guc, move functions to intel_uc.c, don't
clear obj on new GuC load, free object only if enable_guc_loading
is set (Michal)

Cc: Chris Wilson 
Cc: Oscar Mateo 
Cc: Michal Wajdeczko 
Signed-off-by: Daniele Ceraolo Spurio 
---


Reviewed-by: Michal Wajdeczko 

Regards,
Michal



Does anyone else have any concern or can this patch be merged?



+1 about being merged, and since I've been using this patch you could say,

Tested-by: Michel Thierry 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6] drm/i915/guc: capture GuC logs if FW fails to load

[Daniele Ceraolo Spurio]

On 22/05/17 12:56, Michal Wajdeczko wrote:

On Mon, May 22, 2017 at 10:50:28AM -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)

v6: move obj back to intel_guc, move functions to intel_uc.c, don't
clear obj on new GuC load, free object only if enable_guc_loading
is set (Michal)

Cc: Chris Wilson 
Cc: Oscar Mateo 
Cc: Michal Wajdeczko 
Signed-off-by: Daniele Ceraolo Spurio 
---


Reviewed-by: Michal Wajdeczko 

Regards,
Michal



Does anyone else have any concern or can this patch be merged?

Thanks,
Daniele
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6] drm/i915/guc: capture GuC logs if FW fails to load

[Michal Wajdeczko]

On Mon, May 22, 2017 at 10:50:28AM -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)
> 
> v6: move obj back to intel_guc, move functions to intel_uc.c, don't
> clear obj on new GuC load, free object only if enable_guc_loading
> is set (Michal)
> 
> Cc: Chris Wilson 
> Cc: Oscar Mateo 
> Cc: Michal Wajdeczko 
> Signed-off-by: Daniele Ceraolo Spurio 
> ---

Reviewed-by: Michal Wajdeczko 

Regards,
Michal
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v6] drm/i915/guc: capture GuC logs if FW fails to load

[Daniele Ceraolo Spurio]

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)

v6: move obj back to intel_guc, move functions to intel_uc.c, don't
clear obj on new GuC load, free object only if enable_guc_loading
is set (Michal)

Cc: Chris Wilson 
Cc: Oscar Mateo 
Cc: Michal Wajdeczko 
Signed-off-by: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/i915_debugfs.c | 39 -
 drivers/gpu/drm/i915/intel_uc.c | 25 ++--
 drivers/gpu/drm/i915/intel_uc.h |  3 +++
 3 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index c08a6d8..7e0816c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2586,27 +2586,37 @@ 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.load_err_log;
+   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)) {
+   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;
 }
 
@@ -4791,6 +4801,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_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index d27b527..e6e913f 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -286,6 +286,23 @@ static void guc_init_send_regs(struct intel_guc *guc)
guc->send_regs.fw_domains = fw_domains;
 }
 
+static void guc_capture_load_err_log(struct intel_guc *guc)
+{
+   if (!guc->log.vma || i915.guc_log_level < 0)
+   return;
+
+   if (!guc->load_err_log)
+   guc->load_err_log = i915_gem_object_get(guc->log.vma->obj);
+
+   return;
+}
+
+static void guc_free_load_err_log(struct intel_guc *guc)
+{
+   if (guc->load_err_log)
+   i915_gem_object_put(guc->load_err_log);
+}
+
 static int guc_enable_communication(struct intel_guc *guc)
 {
/* XXX: placeholder for alternate setup */
@@ -355,11 +372,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