[Intel-gfx] [Intel-gfx v2 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Umesh Nerlige Ramappa]

On Fri, Jun 17, 2022 at 10:43:45PM -0700, Alan Previn wrote:
>Using igt's gem-create and with additional patches to track object
>creation time, it was measured that guc_update_engine_gt_clks was
>getting called over 188 thousand times in the span of 15 seconds
>(running the test three times).
>
>Get a jiffies sample on every trigger and ensure we skip sampling
>if we are being called too soon. Use half of the ping_delay as a
>safe threshold.
>
>NOTE: with this change, the number of calls went down to just 14
>over the same span of time (matching the original intent of running
>about once every 24 seconds, at 19.2Mhz GT freq, per engine).
>
>Signed-off-by: Alan Previn <alan.previn.teres.alexis at intel.com>
>---
> drivers/gpu/drm/i915/gt/uc/intel_guc.h            |  8 ++++++++
> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 12 ++++++++++++
> 2 files changed, 20 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>index 966e69a8b1c1..26f3e4403de7 100644
>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>@@ -230,6 +230,14 @@ struct intel_guc {
> 		 * @shift: Right shift value for the gpm timestamp
> 		 */
> 		u32 shift;
>+
>+		/**
>+		 * @last_jiffies: jiffies at last actual stats collection time
>+		 * We use this timestamp to ensure we don't oversample the
>+		 * stats because runtime power management events can trigger
>+		 * stats collection at much higher rates than required.
>+		 */
>+		u64 last_stat_jiffs;
> 	} timestamp;
>
> #ifdef CONFIG_DRM_I915_SELFTEST
>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 e62ea35513ea..05c945f14ef5 100644
>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>@@ -1314,6 +1314,8 @@ static void __update_guc_busyness_stats(struct intel_guc *guc)
> 	unsigned long flags;
> 	ktime_t unused;
>
>+	guc->timestamp.last_stat_jiffs = get_jiffies_64();
>+
> 	spin_lock_irqsave(&guc->timestamp.lock, flags);
>
> 	guc_update_pm_timestamp(guc, &unused);
>@@ -1386,6 +1388,16 @@ void intel_guc_busyness_park(struct intel_gt *gt)
> 		return;
>
> 	cancel_delayed_work(&guc->timestamp.work);
>+
>+	/*
>+	 * Before parking, we should sample engine busyness stats if we need to.
>+	 * We can skip it if we are less than half a ping from the last time we
>+	 * sampled the business stats.
>+	 */
>+	if (guc->timestamp.last_stat_jiffs && (get_jiffies_64() - guc->timestamp.last_stat_jiffs  <
>+	   (guc->timestamp.ping_delay >> 1)))
>+		return;
>+

I would just sample the jiffies here instead of inside 
__update_guc_busyness_stats(), so all the logic is within this function 
and easy to read.

Either ways, this should work too, so this is

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>

Regards,
Umesh

> 	__update_guc_busyness_stats(guc);
> }
>
>-- 
>2.25.1
>