Re: [PATCH v1] drm/i915/gsc: Fix intel_gsc_uc_fw_proxy_init_done with directed wakerefs

"Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx> · Tue, 20 Jun 2023 11:05:31 -0700

On 6/16/2023 8:54 AM, Ceraolo Spurio, Daniele wrote:


On 6/15/2023 2:19 PM, Alan Previn wrote:
intel_gsc_uc_fw_proxy_init_done is used by a few code paths
and usages. However, certain paths need a wakeref while others
can't take a wakeref such as from the runtime_pm_resume callstack.

Add a param into this helper to allow callers to direct whether
to take the wakeref or not. This resolves the following bug:

    INFO: task sh:2607 blocked for more than 61 seconds.
    Not tainted 6.3.0-pxp-gsc-final-jun14+ #297
    "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this 
message.
    task:sh              state:D stack:13016 pid:2607 ppid:2602   
flags:0x00004000
    Call Trace:
       <TASK>
       __schedule+0x47b/0xe10
       schedule+0x58/0xd0
       rpm_resume+0x1cc/0x800
       ? __pfx_autoremove_wake_function+0x10/0x10
       __pm_runtime_resume+0x42/0x80
       __intel_runtime_pm_get+0x19/0x80 [i915]
       gsc_uc_get_fw_status+0x10/0x50 [i915]
       intel_gsc_uc_fw_init_done+0x9/0x20 [i915]
       intel_gsc_uc_load_start+0x5b/0x130 [i915]
       __uc_resume+0xa5/0x280 [i915]
       intel_runtime_resume+0xd4/0x250 [i915]
       ? __pfx_pci_pm_runtime_resume+0x10/0x10
    __rpm_callback+0x3c/0x160

Fixes: 8c33c3755b75 ("drm/i915/gsc: take a wakeref for the 
proxy-init-completion check")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>

I'm going to send a trybot of this patch with the FW definition patch, 
just to make sure there aren't any other issues that kick in once the 
FW is defined and the code starts being executed, and merge if the 
results are ok.

Trybot came back green 
(https://patchwork.freedesktop.org/series/119471/), so pushed.

Daniele


Daniele

---
  drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c  | 17 +++++++++++------
  drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h  |  2 +-
  drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c  |  2 +-
  drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c |  2 +-
  4 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c 
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
index 856de9af1e3a..ab1a456f833d 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -22,27 +22,32 @@ static bool gsc_is_in_reset(struct intel_uncore 
*uncore)
              HECI1_FWSTS1_CURRENT_STATE_RESET;
  }
  -static u32 gsc_uc_get_fw_status(struct intel_uncore *uncore)
+static u32 gsc_uc_get_fw_status(struct intel_uncore *uncore, bool 
needs_wakeref)
  {
      intel_wakeref_t wakeref;
      u32 fw_status = 0;
  -    with_intel_runtime_pm(uncore->rpm, wakeref)
-        fw_status = intel_uncore_read(uncore, 
HECI_FWSTS(MTL_GSC_HECI1_BASE, 1));
+    if (needs_wakeref)
+        wakeref = intel_runtime_pm_get(uncore->rpm);
  +    fw_status = intel_uncore_read(uncore, 
HECI_FWSTS(MTL_GSC_HECI1_BASE, 1));
+
+    if (needs_wakeref)
+        intel_runtime_pm_put(uncore->rpm, wakeref);
      return fw_status;
  }
  -bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
+bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc, bool 
needs_wakeref)
  {
      return REG_FIELD_GET(HECI1_FWSTS1_CURRENT_STATE,
- gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore)) ==
+ gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore,
+                          needs_wakeref)) ==
             HECI1_FWSTS1_PROXY_STATE_NORMAL;
  }
    bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
  {
-    return gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore) &
+    return gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore, false) &
             HECI1_FWSTS1_INIT_COMPLETE;
  }
  diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h 
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
index 8d7b9e4f1ffc..ad2167ce9137 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
@@ -15,6 +15,6 @@ struct intel_uncore;
  int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const 
void *data, size_t size);
  int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc);
  bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc);
-bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc);
+bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc, bool 
needs_wakeref);
    #endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c 
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index 85d90f0a15e3..75a3a0790ef3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -72,7 +72,7 @@ static void gsc_work(struct work_struct *work)
               * complete the request handling cleanly, so we need to 
check the
               * status register to check if the proxy init was 
actually successful
               */
-            if (intel_gsc_uc_fw_proxy_init_done(gsc)) {
+            if (intel_gsc_uc_fw_proxy_init_done(gsc, false)) {
                  drm_dbg(&gt->i915->drm, "GSC Proxy initialized\n");
                  intel_uc_fw_change_status(&gsc->fw, 
INTEL_UC_FIRMWARE_RUNNING);
              } else {
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
index f13890ec7db1..c7df47364013 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -197,7 +197,7 @@ bool intel_pxp_gsccs_is_ready_for_sessions(struct 
intel_pxp *pxp)
       * are out of order) will suffice.
       */
      if (intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc, 
INTEL_HUC_AUTH_BY_GSC) &&
- intel_gsc_uc_fw_proxy_init_done(&pxp->ctrl_gt->uc.gsc))
+ intel_gsc_uc_fw_proxy_init_done(&pxp->ctrl_gt->uc.gsc, true))
          return true;
        return false;

base-commit: 134d180cacae82fadbc5ee32f86014cc290f5e0c








