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

[Umesh Nerlige Ramappa]

On Mon, Jun 20, 2022 at 09:46:30AM +0100, Tvrtko Ursulin wrote:
>
>On 18/06/2022 06:43, 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).
>
>It would be beneficial to record the root cause. Both frequency of 
>parking/unparking caused by <insert what> and lmem access cost.
>
>>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;
>
>Why the new "jiffs" naming and not the usual jiffies?
>
>Otherwise a good comment - just align the member name with the 
>kerneldoc name.
>
>>  	} 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();
>
>Why the 64 bit flavour? It's a first in i915 but it doesn't feel so special.
>
>>+
>>  	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.
>
>busyness
>
>>+	 */
>>+	if (guc->timestamp.last_stat_jiffs && (get_jiffies_64() - guc->timestamp.last_stat_jiffs  <
>>+	   (guc->timestamp.ping_delay >> 1)))
>>+		return;
>
>1)
>Recommend a division instead of a shift.
>
>2)
>Is there a time_after() macro for this?
>
>3)
>Should the logic be contained/consolidated in __update_guc_busyness_stats?
>
It wouldn't hurt to have it in __update_guc_busyness_stats, but 
__update_guc_busyness_stats is also called from the ping worker. Since 
this is targeted for gt_park/gt_unpark events, I would leave it here.

>There is cancel_delayed_work in there - is it okay for that to be 
>bypassed from here?

I don't see it being bypassed. cancel_delayed_work is called before the 
jiffies logic here.

Regards,
Umesh


>
>Regards,
>
>Tvrtko
>
>>+
>>  	__update_guc_busyness_stats(guc);
>>  }