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@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;
> > } 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@xxxxxxxxx>
>
> Regards,
> Umesh
>
> > __update_guc_busyness_stats(guc);
> > }
> >
> > --
> > 2.25.1
> >
