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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Thu, Nov 21, 2024 at 04:37:31PM -0800, John Harrison wrote:
>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.

Sure, I will move it up.

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