Re: [PATCH 2/3] drm/i915 Rename intel_engine_reset to intel_gt_engine_recover

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

On 4/18/2024 10:10, Nirmoy Das wrote:
> intel_engine_reset() not only reset a engine but also
> tries to recover it so give it a proper name without
> any functional changes.
Not seeing what the difference is. If this was a super low level 
function (with an __ prefix for example) then one might expect it to 
literally just poke the reset register and leave the engine in a dead 
state. But as a high level function, I think it is reasonable to expect 
a reset function to 'recover' the entity being reset.

Also, many of the callers are tests that are explicitly testing reset. 
So now the tests all talk about attempting resets, resets failing, etc. 
but around a call to 'recover' instead of 'reset', which seems confusing.

John.

>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   .../drm/i915/gem/selftests/i915_gem_context.c |  2 +-
>   .../drm/i915/gt/intel_execlists_submission.c  |  2 +-
>   drivers/gpu/drm/i915/gt/intel_reset.c         |  4 ++--
>   drivers/gpu/drm/i915/gt/intel_reset.h         |  4 ++--
>   drivers/gpu/drm/i915/gt/selftest_hangcheck.c  | 20 +++++++++----------
>   drivers/gpu/drm/i915/gt/selftest_mocs.c       |  4 ++--
>   drivers/gpu/drm/i915/gt/selftest_reset.c      |  2 +-
>   .../gpu/drm/i915/gt/selftest_workarounds.c    |  6 +++---
>   8 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> index 89d4dc8b60c6..4f4cde55f621 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> @@ -1171,7 +1171,7 @@ __sseu_finish(const char *name,
>   	int ret = 0;
>   
>   	if (flags & TEST_RESET) {
> -		ret = intel_engine_reset(ce->engine, "sseu");
> +		ret = intel_gt_engine_recover(ce->engine, "sseu");
>   		if (ret)
>   			goto out;
>   	}
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 21829439e686..9485a622a704 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -2404,7 +2404,7 @@ static void execlists_reset(struct intel_engine_cs *engine, const char *msg)
>   
>   	ring_set_paused(engine, 1); /* Freeze the current request in place */
>   	execlists_capture(engine);
> -	intel_engine_reset(engine, msg);
> +	intel_gt_engine_recover(engine, msg);
>   
>   	tasklet_enable(&engine->sched_engine->tasklet);
>   	clear_and_wake_up_bit(bit, lock);
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index b825daace58e..6504e8ba9c58 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -1348,7 +1348,7 @@ int __intel_engine_reset_bh(struct intel_engine_cs *engine, const char *msg)
>   }
>   
>   /**
> - * intel_engine_reset - reset GPU engine to recover from a hang
> + * intel_gt_engine_recover - reset GPU engine to recover from a hang
>    * @engine: engine to reset
>    * @msg: reason for GPU reset; or NULL for no drm_notice()
>    *
> @@ -1360,7 +1360,7 @@ int __intel_engine_reset_bh(struct intel_engine_cs *engine, const char *msg)
>    *  - reset engine (which will force the engine to idle)
>    *  - re-init/configure engine
>    */
> -int intel_engine_reset(struct intel_engine_cs *engine, const char *msg)
> +int intel_gt_engine_recover(struct intel_engine_cs *engine, const char *msg)
>   {
>   	int err;
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.h b/drivers/gpu/drm/i915/gt/intel_reset.h
> index c00de353075c..be984357bf27 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.h
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.h
> @@ -31,8 +31,8 @@ void intel_gt_handle_error(struct intel_gt *gt,
>   void intel_gt_reset(struct intel_gt *gt,
>   		    intel_engine_mask_t stalled_mask,
>   		    const char *reason);
> -int intel_engine_reset(struct intel_engine_cs *engine,
> -		       const char *reason);
> +int intel_gt_engine_recover(struct intel_engine_cs *engine,
> +			    const char *reason);
>   int __intel_engine_reset_bh(struct intel_engine_cs *engine,
>   			    const char *reason);
>   
> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> index 9ce8ff1c04fe..9bfda3f2bd24 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> @@ -495,9 +495,9 @@ static int igt_reset_nop_engine(void *arg)
>   
>   				i915_request_add(rq);
>   			}
> -			err = intel_engine_reset(engine, NULL);
> +			err = intel_gt_engine_recover(engine, NULL);
>   			if (err) {
> -				pr_err("intel_engine_reset(%s) failed, err:%d\n",
> +				pr_err("intel_gt_engine_recover(%s) failed, err:%d\n",
>   				       engine->name, err);
>   				break;
>   			}
> @@ -574,7 +574,7 @@ static int igt_reset_fail_engine(void *arg)
>   					    &gt->reset.flags));
>   
>   		force_reset_timeout(engine);
> -		err = intel_engine_reset(engine, NULL);
> +		err = intel_gt_engine_recover(engine, NULL);
>   		cancel_reset_timeout(engine);
>   		if (err == 0) /* timeouts only generated on gen8+ */
>   			goto skip;
> @@ -623,9 +623,9 @@ static int igt_reset_fail_engine(void *arg)
>   			}
>   
>   			if (count & 1) {
> -				err = intel_engine_reset(engine, NULL);
> +				err = intel_gt_engine_recover(engine, NULL);
>   				if (err) {
> -					GEM_TRACE_ERR("intel_engine_reset(%s) failed, err:%d\n",
> +					GEM_TRACE_ERR("intel_gt_engine_recover(%s) failed, err:%d\n",
>   						      engine->name, err);
>   					GEM_TRACE_DUMP();
>   					i915_request_put(last);
> @@ -633,10 +633,10 @@ static int igt_reset_fail_engine(void *arg)
>   				}
>   			} else {
>   				force_reset_timeout(engine);
> -				err = intel_engine_reset(engine, NULL);
> +				err = intel_gt_engine_recover(engine, NULL);
>   				cancel_reset_timeout(engine);
>   				if (err != -ETIMEDOUT) {
> -					pr_err("intel_engine_reset(%s) did not fail, err:%d\n",
> +					pr_err("intel_gt_engine_recover(%s) did not fail, err:%d\n",
>   					       engine->name, err);
>   					i915_request_put(last);
>   					break;
> @@ -765,9 +765,9 @@ static int __igt_reset_engine(struct intel_gt *gt, bool active)
>   			}
>   
>   			if (!using_guc) {
> -				err = intel_engine_reset(engine, NULL);
> +				err = intel_gt_engine_recover(engine, NULL);
>   				if (err) {
> -					pr_err("intel_engine_reset(%s) failed, err:%d\n",
> +					pr_err("intel_gt_engine_recover(%s) failed, err:%d\n",
>   					       engine->name, err);
>   					goto skip;
>   				}
> @@ -1085,7 +1085,7 @@ static int __igt_reset_engines(struct intel_gt *gt,
>   			}
>   
>   			if (!using_guc) {
> -				err = intel_engine_reset(engine, NULL);
> +				err = intel_gt_engine_recover(engine, NULL);
>   				if (err) {
>   					pr_err("i915_reset_engine(%s:%s): failed, err=%d\n",
>   					       engine->name, test_name, err);
> diff --git a/drivers/gpu/drm/i915/gt/selftest_mocs.c b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> index d73e438fb85f..b7b15dd3163f 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> @@ -336,7 +336,7 @@ static int active_engine_reset(struct intel_context *ce,
>   
>   	err = request_add_spin(rq, &spin);
>   	if (err == 0 && !using_guc)
> -		err = intel_engine_reset(ce->engine, reason);
> +		err = intel_gt_engine_recover(ce->engine, reason);
>   
>   	/* Ensure the reset happens and kills the engine */
>   	if (err == 0)
> @@ -356,7 +356,7 @@ static int __live_mocs_reset(struct live_mocs *mocs,
>   
>   	if (intel_has_reset_engine(gt)) {
>   		if (!using_guc) {
> -			err = intel_engine_reset(ce->engine, "mocs");
> +			err = intel_gt_engine_recover(ce->engine, "mocs");
>   			if (err)
>   				return err;
>   
> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index 2cfc23c58e90..9eaa1aed9f58 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
> @@ -115,7 +115,7 @@ __igt_reset_stolen(struct intel_gt *gt,
>   	} else {
>   		for_each_engine(engine, gt, id) {
>   			if (mask & engine->mask)
> -				intel_engine_reset(engine, NULL);
> +				intel_gt_engine_recover(engine, NULL);
>   		}
>   	}
>   
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index 14a8b25b6204..eb7516c7cb56 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -256,7 +256,7 @@ static int do_device_reset(struct intel_engine_cs *engine)
>   
>   static int do_engine_reset(struct intel_engine_cs *engine)
>   {
> -	return intel_engine_reset(engine, "live_workarounds");
> +	return intel_gt_engine_recover(engine, "live_workarounds");
>   }
>   
>   static int do_guc_reset(struct intel_engine_cs *engine)
> @@ -1282,7 +1282,7 @@ live_engine_reset_workarounds(void *arg)
>   				goto err;
>   			}
>   
> -			ret = intel_engine_reset(engine, "live_workarounds:idle");
> +			ret = intel_gt_engine_recover(engine, "live_workarounds:idle");
>   			if (ret) {
>   				pr_err("%s: Reset failed while idle\n", engine->name);
>   				goto err;
> @@ -1320,7 +1320,7 @@ live_engine_reset_workarounds(void *arg)
>   		}
>   
>   		if (!using_guc) {
> -			ret = intel_engine_reset(engine, "live_workarounds:active");
> +			ret = intel_gt_engine_recover(engine, "live_workarounds:active");
>   			if (ret) {
>   				pr_err("%s: Reset failed on an active spinner\n",
>   				       engine->name);