Re: [PATCH 2/3] i915/guc: Ensure busyness counter increases monotonically

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

On Thu, Nov 21, 2024 at 04:34:14PM -0800, John Harrison wrote:
>On 11/21/2024 16:31, John Harrison wrote:
>>On 11/18/2024 15:22, Umesh Nerlige Ramappa wrote:
>>>Active busyness of an engine is calculated using gt timestamp and the
>>>context switch in time. While capturing the gt timestamp, it's possible
>>>that the context switches out. This race could result in an active
>>>busyness value that is greater than the actual context runtime 
>>>value by a
>>>small amount. This leads to a negative delta and throws off busyness
>>>calculations for the user.
>>>
>>>If a subsequent count is smaller than the previous one, just return the
>>>previous one, since we expect the busyness to catch up.
>>>
>>>Fixes: 77cdd054dd2c ("drm/i915/pmu: Connect engine busyness stats 
>>>from GuC to pmu")
>>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>Actually, does this need to be cleared in 
>__reset_guc_busyness_stats()? If there is a reset and the live value 
>is returned to zero then you won't get any more updates until it gets 
>back to where it used to be.

We don't return the live value. We just accumulate the deltas from live 
values into stats->total_gt_clks, compare it to stats->total and return 
the one that is monotonically increasing.

The stats->total_gt_clks and stats->total are retained across resets, so 
we should be good.

Thanks,
Umesh
>
>John.
>
>>
>>>---
>>>  drivers/gpu/drm/i915/gt/intel_engine_types.h      | 5 +++++
>>>  drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 ++++-
>>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h 
>>>b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>index ba55c059063d..fe1f85e5dda3 100644
>>>--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>@@ -343,6 +343,11 @@ struct intel_engine_guc_stats {
>>>       * @start_gt_clk: GT clock time of last idle to active transition.
>>>       */
>>>      u64 start_gt_clk;
>>>+
>>>+    /**
>>>+     * @total: The last value of total returned
>>>+     */
>>>+    u64 total;
>>>  };
>>>    union intel_engine_tlb_inv_reg {
>>>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 c71aedcbce43..56be9f385270 100644
>>>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>@@ -1378,9 +1378,12 @@ static ktime_t guc_engine_busyness(struct 
>>>intel_engine_cs *engine, ktime_t *now)
>>>          total += intel_gt_clock_interval_to_ns(gt, clk);
>>>      }
>>>  +    if (total > stats->total)
>>>+        stats->total = total;
>>>+
>>>      spin_unlock_irqrestore(&guc->timestamp.lock, flags);
>>>  -    return ns_to_ktime(total);
>>>+    return ns_to_ktime(stats->total);
>>>  }
>>>    static void guc_enable_busyness_worker(struct intel_guc *guc)
>>
>