Re: [Intel-gfx] [PATCH] drm/i915/pmu: Fix synchronization of PMU callback with reset

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Nov 03, 2021 at 03:47:08PM -0700, Umesh Nerlige Ramappa wrote:
> Since the PMU callback runs in irq context, it synchronizes with gt
> reset using the reset count. We could run into a case where the PMU
> callback could read the reset count before it is updated. This has a
> potential of corrupting the busyness stats.
> 
> In addition to the reset count, check if the reset bit is set before
> capturing busyness.
> 
> In addition save the previous stats only if you intend to update them.
> 
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> 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 5cc49c0b3889..d83ade77ca07 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1183,6 +1183,7 @@ static ktime_t guc_engine_busyness(struct intel_engine_cs *engine, ktime_t *now)
>  	u64 total, gt_stamp_saved;
>  	unsigned long flags;
>  	u32 reset_count;
> +	bool in_reset;
>  
>  	spin_lock_irqsave(&guc->timestamp.lock, flags);
>  
> @@ -1191,7 +1192,9 @@ static ktime_t guc_engine_busyness(struct intel_engine_cs *engine, ktime_t *now)
>  	 * engine busyness from GuC, so we just use the driver stored
>  	 * copy of busyness. Synchronize with gt reset using reset_count.
>  	 */
> -	reset_count = i915_reset_count(gpu_error);
> +	rcu_read_lock();
> +	in_reset = test_bit(I915_RESET_BACKOFF, &gt->reset.flags);
> +	rcu_read_unlock();
>  
>  	*now = ktime_get();
>  
> @@ -1201,9 +1204,10 @@ static ktime_t guc_engine_busyness(struct intel_engine_cs *engine, ktime_t *now)
>  	 * start_gt_clk is derived from GuC state. To get a consistent
>  	 * view of activity, we query the GuC state only if gt is awake.
>  	 */
> -	stats_saved = *stats;
> -	gt_stamp_saved = guc->timestamp.gt_stamp;
> -	if (intel_gt_pm_get_if_awake(gt)) {
> +	if (intel_gt_pm_get_if_awake(gt) && !in_reset) {
> +		stats_saved = *stats;
> +		gt_stamp_saved = guc->timestamp.gt_stamp;
> +		reset_count = i915_reset_count(gpu_error);
>  		guc_update_engine_gt_clks(engine);
>  		guc_update_pm_timestamp(guc, engine, now);
>  		intel_gt_pm_put_async(gt);
> -- 
> 2.20.1
>