Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/guc: Improve clean up of busyness stats worker

[Ceraolo Spurio, Daniele]

On 2/17/2023 2:33 PM, john.c.harri...@intel.com wrote:

From: John Harrison <john.c.harri...@intel.com>

The stats worker thread management was mis-matched between
enable/disable call sites. Fix those up. Also, abstract the
cancel/enable code into a helper function rather than replicating in
multiple places.

v2: Rename the helpers and wrap the enable as well as the cancel
(review feedback from Daniele).

Signed-off-by: John Harrison <john.c.harri...@intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>

Daniele

---
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 38 +++++++++++--------
  1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index be495e657d66b..a04d7049a2c2f 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1352,6 +1352,16 @@ static ktime_t guc_engine_busyness(struct 
intel_engine_cs *engine, ktime_t *now)
        return ns_to_ktime(total);
  }

+static void guc_enable_busyness_worker(struct intel_guc *guc)

+{
+       mod_delayed_work(system_highpri_wq, &guc->timestamp.work, 
guc->timestamp.ping_delay);
+}
+
+static void guc_cancel_busyness_worker(struct intel_guc *guc)
+{
+       cancel_delayed_work_sync(&guc->timestamp.work);
+}
+
  static void __reset_guc_busyness_stats(struct intel_guc *guc)
  {
        struct intel_gt *gt = guc_to_gt(guc);
@@ -1360,7 +1370,7 @@ static void __reset_guc_busyness_stats(struct intel_guc 
*guc)
        unsigned long flags;
        ktime_t unused;

- cancel_delayed_work_sync(&guc->timestamp.work);

+       guc_cancel_busyness_worker(guc);

spin_lock_irqsave(&guc->timestamp.lock, flags); @@ -1416,8 +1426,7 @@ static void guc_timestamp_ping(struct work_struct *wrk) intel_gt_reset_unlock(gt, srcu); - mod_delayed_work(system_highpri_wq, &guc->timestamp.work,

-                        guc->timestamp.ping_delay);
+       guc_enable_busyness_worker(guc);
  }

static int guc_action_enable_usage_stats(struct intel_guc *guc)

@@ -1436,16 +1445,15 @@ static void guc_init_engine_stats(struct intel_guc *guc)
  {
        struct intel_gt *gt = guc_to_gt(guc);
        intel_wakeref_t wakeref;
+       int ret;

- mod_delayed_work(system_highpri_wq, &guc->timestamp.work,

-                        guc->timestamp.ping_delay);
-
-       with_intel_runtime_pm(&gt->i915->runtime_pm, wakeref) {
-               int ret = guc_action_enable_usage_stats(guc);
+       with_intel_runtime_pm(&gt->i915->runtime_pm, wakeref)
+               ret = guc_action_enable_usage_stats(guc);

- if (ret)

-                       guc_err(guc, "Failed to enable usage stats: %pe\n", 
ERR_PTR(ret));
-       }
+       if (ret)
+               guc_err(guc, "Failed to enable usage stats: %pe\n", 
ERR_PTR(ret));
+       else
+               guc_enable_busyness_worker(guc);
  }

void intel_guc_busyness_park(struct intel_gt *gt)

@@ -1460,7 +1468,7 @@ void intel_guc_busyness_park(struct intel_gt *gt)
         * and causes an unclaimed register access warning. Cancel the worker
         * synchronously here.
         */
-       cancel_delayed_work_sync(&guc->timestamp.work);
+       guc_cancel_busyness_worker(guc);

/*

         * Before parking, we should sample engine busyness stats if we need to.
@@ -1487,8 +1495,7 @@ void intel_guc_busyness_unpark(struct intel_gt *gt)
        spin_lock_irqsave(&guc->timestamp.lock, flags);
        guc_update_pm_timestamp(guc, &unused);
        spin_unlock_irqrestore(&guc->timestamp.lock, flags);
-       mod_delayed_work(system_highpri_wq, &guc->timestamp.work,
-                        guc->timestamp.ping_delay);
+       guc_enable_busyness_worker(guc);
  }

static inline bool

@@ -4408,11 +4415,12 @@ void intel_guc_submission_enable(struct intel_guc *guc)
        guc_init_global_schedule_policy(guc);
  }

+/* Note: By the time we're here, GuC may have already been reset */

  void intel_guc_submission_disable(struct intel_guc *guc)
  {
        struct intel_gt *gt = guc_to_gt(guc);

- /* Note: By the time we're here, GuC may have already been reset */

+       guc_cancel_busyness_worker(guc);

/* Disable and route to host */

        if (GRAPHICS_VER(gt->i915) >= 12)