Re: [PATCH 1/3] drm/i915: Refactor confusing __intel_gt_reset()

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

On 4/18/2024 10:10, Nirmoy Das wrote:
> __intel_gt_reset() is really for resetting engines though
> the name might suggest something else. So add two helper functions
> to remove confusions with no functional changes.
Technically you only added one and just moved the other :). It already 
existed, it just wasn't being used everywhere that it could be!

>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c     |  2 +-
>   .../drm/i915/gt/intel_execlists_submission.c  |  2 +-
>   drivers/gpu/drm/i915/gt/intel_gt.c            |  2 +-
>   drivers/gpu/drm/i915/gt/intel_gt_pm.c         |  2 +-
>   drivers/gpu/drm/i915/gt/intel_reset.c         | 43 ++++++++++++++-----
>   drivers/gpu/drm/i915/gt/intel_reset.h         |  3 +-
>   drivers/gpu/drm/i915/gt/selftest_reset.c      |  2 +-
>   drivers/gpu/drm/i915/i915_driver.c            |  2 +-
>   8 files changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 8c44af1c3451..5c8e9ee3b008 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -678,7 +678,7 @@ void intel_engines_release(struct intel_gt *gt)
>   	 */
>   	GEM_BUG_ON(intel_gt_pm_is_awake(gt));
>   	if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
> -		__intel_gt_reset(gt, ALL_ENGINES);
> +		intel_gt_reset_all_engines(gt);
>   
>   	/* Decouple the backend; but keep the layout for late GPU resets */
>   	for_each_engine(engine, gt, id) {
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 355aab5b38ba..21829439e686 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -2898,7 +2898,7 @@ static void enable_error_interrupt(struct intel_engine_cs *engine)
>   		drm_err(&engine->i915->drm,
>   			"engine '%s' resumed still in error: %08x\n",
>   			engine->name, status);
> -		__intel_gt_reset(engine->gt, engine->mask);
> +		intel_gt_reset_engine(engine);
>   	}
>   
>   	/*
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 580b5141ce1e..626b166e67ef 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -832,7 +832,7 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
>   
>   	/* Scrub all HW state upon release */
>   	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> -		__intel_gt_reset(gt, ALL_ENGINES);
> +		intel_gt_reset_all_engines(gt);
>   }
>   
>   void intel_gt_driver_release(struct intel_gt *gt)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> index 220ac4f92edf..c08fdb65cc69 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> @@ -159,7 +159,7 @@ static bool reset_engines(struct intel_gt *gt)
>   	if (INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
>   		return false;
>   
> -	return __intel_gt_reset(gt, ALL_ENGINES) == 0;
> +	return intel_gt_reset_all_engines(gt) == 0;
>   }
>   
>   static void gt_sanitize(struct intel_gt *gt, bool force)
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index c8e9aa41fdea..b825daace58e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -764,7 +764,7 @@ wa_14015076503_end(struct intel_gt *gt, intel_engine_mask_t engine_mask)
>   			 HECI_H_GS1_ER_PREP, 0);
>   }
>   
> -int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> +static int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
>   {
>   	const int retries = engine_mask == ALL_ENGINES ? RESET_MAX_RETRIES : 1;
>   	reset_func reset;
> @@ -795,6 +795,34 @@ int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
>   	return ret;
>   }
>   
> +/**
> + * intel_gt_reset_all_engines() - Reset all engines in the given gt.
> + * @gt: the GT to reset all engines for.
> + *
> + * This function resets all engines within the given gt.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int intel_gt_reset_all_engines(struct intel_gt *gt)
> +{
> +	return __intel_gt_reset(gt, ALL_ENGINES);
> +}
> +
> +/**
> + * intel_gt_reset_engine() - Reset a specific engine within a gt.
> + * @engine: engine to be reset.
> + *
> + * This function resets the specified engine within a gt.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int intel_gt_reset_engine(struct intel_engine_cs *engine)
> +{
> +	return __intel_gt_reset(engine->gt, engine->mask);
> +}
> +
You could have just dropped the 'static' from the existing copy of this 
function and added the new version next to it. That would make the diff 
simpler and therefore clearer. Unless you think there is a good reason 
to move the code up here?

John.

>   bool intel_has_gpu_reset(const struct intel_gt *gt)
>   {
>   	if (!gt->i915->params.reset)
> @@ -978,7 +1006,7 @@ static void __intel_gt_set_wedged(struct intel_gt *gt)
>   
>   	/* Even if the GPU reset fails, it should still stop the engines */
>   	if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
> -		__intel_gt_reset(gt, ALL_ENGINES);
> +		intel_gt_reset_all_engines(gt);
>   
>   	for_each_engine(engine, gt, id)
>   		engine->submit_request = nop_submit_request;
> @@ -1089,7 +1117,7 @@ static bool __intel_gt_unset_wedged(struct intel_gt *gt)
>   	/* We must reset pending GPU events before restoring our submission */
>   	ok = !HAS_EXECLISTS(gt->i915); /* XXX better agnosticism desired */
>   	if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
> -		ok = __intel_gt_reset(gt, ALL_ENGINES) == 0;
> +		ok = intel_gt_reset_all_engines(gt) == 0;
>   	if (!ok) {
>   		/*
>   		 * Warn CI about the unrecoverable wedged condition.
> @@ -1133,10 +1161,10 @@ static int do_reset(struct intel_gt *gt, intel_engine_mask_t stalled_mask)
>   {
>   	int err, i;
>   
> -	err = __intel_gt_reset(gt, ALL_ENGINES);
> +	err = intel_gt_reset_all_engines(gt);
>   	for (i = 0; err && i < RESET_MAX_RETRIES; i++) {
>   		msleep(10 * (i + 1));
> -		err = __intel_gt_reset(gt, ALL_ENGINES);
> +		err = intel_gt_reset_all_engines(gt);
>   	}
>   	if (err)
>   		return err;
> @@ -1270,11 +1298,6 @@ void intel_gt_reset(struct intel_gt *gt,
>   	goto finish;
>   }
>   
> -static int intel_gt_reset_engine(struct intel_engine_cs *engine)
> -{
> -	return __intel_gt_reset(engine->gt, engine->mask);
> -}
> -
>   int __intel_engine_reset_bh(struct intel_engine_cs *engine, const char *msg)
>   {
>   	struct intel_gt *gt = engine->gt;
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.h b/drivers/gpu/drm/i915/gt/intel_reset.h
> index f615b30b81c5..c00de353075c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.h
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.h
> @@ -54,7 +54,8 @@ int intel_gt_terminally_wedged(struct intel_gt *gt);
>   void intel_gt_set_wedged_on_init(struct intel_gt *gt);
>   void intel_gt_set_wedged_on_fini(struct intel_gt *gt);
>   
> -int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask);
> +int intel_gt_reset_engine(struct intel_engine_cs *engine);
> +int intel_gt_reset_all_engines(struct intel_gt *gt);
>   
>   int intel_reset_guc(struct intel_gt *gt);
>   
> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index f40de408cd3a..2cfc23c58e90 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
> @@ -281,7 +281,7 @@ static int igt_atomic_reset(void *arg)
>   		awake = reset_prepare(gt);
>   		p->critical_section_begin();
>   
> -		err = __intel_gt_reset(gt, ALL_ENGINES);
> +		err = intel_gt_reset_all_engines(gt);
>   
>   		p->critical_section_end();
>   		reset_finish(gt, awake);
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 4b9233c07a22..622a24305bc2 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -202,7 +202,7 @@ static void sanitize_gpu(struct drm_i915_private *i915)
>   		unsigned int i;
>   
>   		for_each_gt(gt, i915, i)
> -			__intel_gt_reset(gt, ALL_ENGINES);
> +			intel_gt_reset_all_engines(gt);
>   	}
>   }
>