Re: [Intel-gfx] [PATCH] drm/i915/guc: Propagate the fw xfer timeout

[Michal Wajdeczko]

On Thu, 18 Oct 2018 01:09:19 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospu...@intel.com> wrote:



On 17/10/18 13:29, Chris Wilson wrote:
Propagate the timeout on transferring the fw back to the caller where it
may act upon it, usually by restarting the xfer before failing.


Did you see any case where we failed the xfer and didn't get a timeout  
out of guc_wait_ucode? that'd be quite weird

Anyway, definitely better and cleaner than before:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>

small nitpick below

Testcase: igt/drv_selftest/live_hangcheck
Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>
---
  drivers/gpu/drm/i915/intel_guc_fw.c | 23 ++++++-----------------
  1 file changed, 6 insertions(+), 17 deletions(-)
 diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c  
b/drivers/gpu/drm/i915/intel_guc_fw.c
index a9e6fcce467c..b68a05892dab 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -125,22 +125,17 @@ static void guc_prepare_xfer(struct intel_guc  
*guc)
  }
    /* Copy RSA signature from the fw image to HW for verification */
-static int guc_xfer_rsa(struct intel_guc *guc, struct i915_vma *vma)
+static void guc_xfer_rsa(struct intel_guc *guc, struct i915_vma *vma)
  {
        struct drm_i915_private *dev_priv = guc_to_i915(guc);
-       struct intel_uc_fw *guc_fw = &guc->fw;
-       struct sg_table *sg = vma->pages;
        u32 rsa[UOS_RSA_SCRATCH_COUNT];
        int i;
  -     if (sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, sizeof(rsa),
-                              guc_fw->rsa_offset) != sizeof(rsa))
-               return -EINVAL;
+       sg_pcopy_to_buffer(vma->pages->sgl, vma->pages->nents,
+                          rsa, sizeof(rsa), guc->fw.rsa_offset);
        for (i = 0; i < UOS_RSA_SCRATCH_COUNT; i++)
                I915_WRITE(UOS_RSA_SCRATCH(i), rsa[i]);
-
-       return 0;
  }
    /*
@@ -251,17 +246,11 @@ static int guc_fw_xfer(struct intel_uc_fw  
*guc_fw, struct i915_vma *vma)
         * by the DMA engine in one operation, whereas the RSA signature is
         * loaded via MMIO.
         */
-       ret = guc_xfer_rsa(guc, vma);
-       if (ret)
-               DRM_WARN("GuC firmware signature xfer error %d\n", ret);
+       guc_xfer_rsa(guc, vma);
        ret = guc_xfer_ucode(guc, vma);
-       if (ret)
-               DRM_WARN("GuC firmware code xfer error %d\n", ret);
-
-       ret = guc_wait_ucode(guc);
-       if (ret)
-               DRM_ERROR("GuC firmware xfer error %d\n", ret);

With the logs gone we don't have any more error message to understand  
where exactly we hit issue (xfer vs wait_ucode). We do have debug logs  
printing the status that would give the info, but might be worth  
retaining at least 1 error-level log.

+1 for leaving error-level logs (here or at helper functions)
also, please let CI re-run this patch with GuC enabled ;)

Michal


Daniele

+       if (ret == 0)
+               ret = guc_wait_ucode(guc);
        intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx