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@xxxxxxxxx>
---
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);
}
