Re: [PATCH 3/3] i915/guc: Accumulate active runtime on gt reset

[John Harrison <john.c.harrison@intel.com>]

On 11/18/2024 15:22, Umesh Nerlige Ramappa wrote:
> On gt reset, if a context is running, then accumulate it's active time
> into the busyness counter since there will be no chance for the context
> to switch out and update it's run time.
>
> Fixes: 77cdd054dd2c ("drm/i915/pmu: Connect engine busyness stats from GuC to pmu")
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> 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 56be9f385270..0c204b7f3b2b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1449,8 +1449,21 @@ static void __reset_guc_busyness_stats(struct intel_guc *guc)
>   
>   	guc_update_pm_timestamp(guc, &unused);
>   	for_each_engine(engine, gt, id) {
> +		struct intel_engine_guc_stats *stats = &engine->stats.guc;
> +
>   		guc_update_engine_gt_clks(engine);
> -		engine->stats.guc.prev_total = 0;
> +

I think the comment should be here given that this is the 'if' that it 
starts with.
> +		if (stats->running) {
> +			u64 clk = guc->timestamp.gt_stamp - stats->start_gt_clk;
> +
> +			/*
> +			 * If resetting a running context, accumulate the active
> +			 * time as well since there will be no context switch.
> +			 */
Having the comment here implies the calculation below has some kind of 
condition, which it doesn't. Plus the comment also refers to the 
calculation above that determines the 'active time' it mentions.

John.

> +			stats->total_gt_clks += clk;
> +		}
> +		stats->prev_total = 0;
> +		stats->running = 0;
>   	}
>   
>   	spin_unlock_irqrestore(&guc->timestamp.lock, flags);