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

[Teres Alexis, Alan Previn]

Thanks Umesh. As per offline - i will address Tvrtko's comments too.
...alan

On Wed, 2022-06-22 at 16:00 -0700, Nerlige Ramappa, Umesh wrote:
> 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
> >