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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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?

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

Regards,

Tvrtko

> +
>   	__update_guc_busyness_stats(guc);
>   }
>