Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/guc: Improve clean up of busyness stats worker

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx>]

On 2/17/2023 2:33 PM, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> The stats worker thread management was mis-matched between
> enable/disable call sites. Fix those up. Also, abstract the
> cancel/enable code into a helper function rather than replicating in
> multiple places.
>
> v2: Rename the helpers and wrap the enable as well as the cancel
> (review feedback from Daniele).
>
> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>

Daniele

> ---
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 38 +++++++++++--------
>   1 file changed, 23 insertions(+), 15 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 be495e657d66b..a04d7049a2c2f 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1352,6 +1352,16 @@ static ktime_t guc_engine_busyness(struct intel_engine_cs *engine, ktime_t *now)
>   	return ns_to_ktime(total);
>   }
>   
> +static void guc_enable_busyness_worker(struct intel_guc *guc)
> +{
> +	mod_delayed_work(system_highpri_wq, &guc->timestamp.work, guc->timestamp.ping_delay);
> +}
> +
> +static void guc_cancel_busyness_worker(struct intel_guc *guc)
> +{
> +	cancel_delayed_work_sync(&guc->timestamp.work);
> +}
> +
>   static void __reset_guc_busyness_stats(struct intel_guc *guc)
>   {
>   	struct intel_gt *gt = guc_to_gt(guc);
> @@ -1360,7 +1370,7 @@ static void __reset_guc_busyness_stats(struct intel_guc *guc)
>   	unsigned long flags;
>   	ktime_t unused;
>   
> -	cancel_delayed_work_sync(&guc->timestamp.work);
> +	guc_cancel_busyness_worker(guc);
>   
>   	spin_lock_irqsave(&guc->timestamp.lock, flags);
>   
> @@ -1416,8 +1426,7 @@ static void guc_timestamp_ping(struct work_struct *wrk)
>   
>   	intel_gt_reset_unlock(gt, srcu);
>   
> -	mod_delayed_work(system_highpri_wq, &guc->timestamp.work,
> -			 guc->timestamp.ping_delay);
> +	guc_enable_busyness_worker(guc);
>   }
>   
>   static int guc_action_enable_usage_stats(struct intel_guc *guc)
> @@ -1436,16 +1445,15 @@ static void guc_init_engine_stats(struct intel_guc *guc)
>   {
>   	struct intel_gt *gt = guc_to_gt(guc);
>   	intel_wakeref_t wakeref;
> +	int ret;
>   
> -	mod_delayed_work(system_highpri_wq, &guc->timestamp.work,
> -			 guc->timestamp.ping_delay);
> -
> -	with_intel_runtime_pm(&gt->i915->runtime_pm, wakeref) {
> -		int ret = guc_action_enable_usage_stats(guc);
> +	with_intel_runtime_pm(&gt->i915->runtime_pm, wakeref)
> +		ret = guc_action_enable_usage_stats(guc);
>   
> -		if (ret)
> -			guc_err(guc, "Failed to enable usage stats: %pe\n", ERR_PTR(ret));
> -	}
> +	if (ret)
> +		guc_err(guc, "Failed to enable usage stats: %pe\n", ERR_PTR(ret));
> +	else
> +		guc_enable_busyness_worker(guc);
>   }
>   
>   void intel_guc_busyness_park(struct intel_gt *gt)
> @@ -1460,7 +1468,7 @@ void intel_guc_busyness_park(struct intel_gt *gt)
>   	 * and causes an unclaimed register access warning. Cancel the worker
>   	 * synchronously here.
>   	 */
> -	cancel_delayed_work_sync(&guc->timestamp.work);
> +	guc_cancel_busyness_worker(guc);
>   
>   	/*
>   	 * Before parking, we should sample engine busyness stats if we need to.
> @@ -1487,8 +1495,7 @@ void intel_guc_busyness_unpark(struct intel_gt *gt)
>   	spin_lock_irqsave(&guc->timestamp.lock, flags);
>   	guc_update_pm_timestamp(guc, &unused);
>   	spin_unlock_irqrestore(&guc->timestamp.lock, flags);
> -	mod_delayed_work(system_highpri_wq, &guc->timestamp.work,
> -			 guc->timestamp.ping_delay);
> +	guc_enable_busyness_worker(guc);
>   }
>   
>   static inline bool
> @@ -4408,11 +4415,12 @@ void intel_guc_submission_enable(struct intel_guc *guc)
>   	guc_init_global_schedule_policy(guc);
>   }
>   
> +/* Note: By the time we're here, GuC may have already been reset */
>   void intel_guc_submission_disable(struct intel_guc *guc)
>   {
>   	struct intel_gt *gt = guc_to_gt(guc);
>   
> -	/* Note: By the time we're here, GuC may have already been reset */
> +	guc_cancel_busyness_worker(guc);
>   
>   	/* Disable and route to host */
>   	if (GRAPHICS_VER(gt->i915) >= 12)