Re: [Intel-gfx] [PATCH] drm/i915/guc: Don't use pr_err when not necessary

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

On 7/14/2022 17:40, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> A bunch of code was copy/pasted using pr_err as the default way to
> report errors. However, drm_err is significantly more useful in
> identifying where the error came from. So update the code to use that
> instead.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
PS: Forgot to include the r-b tag from the previous post of this patch. 
Only change to this one is fix the last minute drm_debug to be drm_dbg 
and word the the commit message a bit better.



On 6/7/2022 15:25, Dixit, Ashutosh wrote:
> On Tue, 07 Jun 2022 15:23:17 -0700, John Harrison wrote:
>> On 6/7/2022 15:07, Dixit, Ashutosh wrote:
>>> On Tue, 07 Jun 2022 14:51:03 -0700, John.C.Harrison@Intel.com wrote:
>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>
>>>> Don't use pr_err in places where we have access to a struct_drm.
>>> Seem to be many more pr_err's in selftests. Is there a reason why drm_err's
>>> cannot be used in selftests (especially those using an i915 device)?
>>> Thanks.
>> I figured I'd start small and just do the gt/uc ones to being with as those
>> are the ones that affect me.
>>
>> It sounds like the only reason to use pr_err is in the mock selftests where
>> there is no easy access to a DRM structure. For everything else, there is
>> no reason that I am aware of.
> Fair enough:
>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>



> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c      | 10 ++---
>   drivers/gpu/drm/i915/gt/uc/selftest_guc.c     | 37 +++++++++----------
>   .../drm/i915/gt/uc/selftest_guc_multi_lrc.c   | 10 ++---
>   3 files changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index 27363091e1afa..19fde6bda8f9c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -195,11 +195,11 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   			    fw_blobs[i].rev < fw_blobs[i - 1].rev)
>   				continue;
>   
> -			pr_err("invalid FW blob order: %s r%u comes before %s r%u\n",
> -			       intel_platform_name(fw_blobs[i - 1].p),
> -			       fw_blobs[i - 1].rev,
> -			       intel_platform_name(fw_blobs[i].p),
> -			       fw_blobs[i].rev);
> +			drm_err(&i915->drm, "Invalid FW blob order: %s r%u comes before %s r%u\n",
> +				intel_platform_name(fw_blobs[i - 1].p),
> +				fw_blobs[i - 1].rev,
> +				intel_platform_name(fw_blobs[i].p),
> +				fw_blobs[i].rev);
>   
>   			uc_fw->path = NULL;
>   		}
> diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> index 1df71d0796aec..20e0c39259fba 100644
> --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> @@ -62,7 +62,7 @@ static int intel_guc_scrub_ctbs(void *arg)
>   		ce = intel_context_create(engine);
>   		if (IS_ERR(ce)) {
>   			ret = PTR_ERR(ce);
> -			pr_err("Failed to create context, %d: %d\n", i, ret);
> +			drm_err(&gt->i915->drm, "Failed to create context, %d: %d\n", i, ret);
>   			goto err;
>   		}
>   
> @@ -83,7 +83,7 @@ static int intel_guc_scrub_ctbs(void *arg)
>   
>   		if (IS_ERR(rq)) {
>   			ret = PTR_ERR(rq);
> -			pr_err("Failed to create request, %d: %d\n", i, ret);
> +			drm_err(&gt->i915->drm, "Failed to create request, %d: %d\n", i, ret);
>   			goto err;
>   		}
>   
> @@ -93,7 +93,7 @@ static int intel_guc_scrub_ctbs(void *arg)
>   	for (i = 0; i < 3; ++i) {
>   		ret = i915_request_wait(last[i], 0, HZ);
>   		if (ret < 0) {
> -			pr_err("Last request failed to complete: %d\n", ret);
> +			drm_err(&gt->i915->drm, "Last request failed to complete: %d\n", ret);
>   			goto err;
>   		}
>   		i915_request_put(last[i]);
> @@ -110,7 +110,7 @@ static int intel_guc_scrub_ctbs(void *arg)
>   	/* GT will not idle if G2H are lost */
>   	ret = intel_gt_wait_for_idle(gt, HZ);
>   	if (ret < 0) {
> -		pr_err("GT failed to idle: %d\n", ret);
> +		drm_err(&gt->i915->drm, "GT failed to idle: %d\n", ret);
>   		goto err;
>   	}
>   
> @@ -150,7 +150,7 @@ static int intel_guc_steal_guc_ids(void *arg)
>   
>   	ce = kcalloc(GUC_MAX_CONTEXT_ID, sizeof(*ce), GFP_KERNEL);
>   	if (!ce) {
> -		pr_err("Context array allocation failed\n");
> +		drm_err(&gt->i915->drm, "Context array allocation failed\n");
>   		return -ENOMEM;
>   	}
>   
> @@ -164,24 +164,24 @@ static int intel_guc_steal_guc_ids(void *arg)
>   	if (IS_ERR(ce[context_index])) {
>   		ret = PTR_ERR(ce[context_index]);
>   		ce[context_index] = NULL;
> -		pr_err("Failed to create context: %d\n", ret);
> +		drm_err(&gt->i915->drm, "Failed to create context: %d\n", ret);
>   		goto err_wakeref;
>   	}
>   	ret = igt_spinner_init(&spin, engine->gt);
>   	if (ret) {
> -		pr_err("Failed to create spinner: %d\n", ret);
> +		drm_err(&gt->i915->drm, "Failed to create spinner: %d\n", ret);
>   		goto err_contexts;
>   	}
>   	spin_rq = igt_spinner_create_request(&spin, ce[context_index],
>   					     MI_ARB_CHECK);
>   	if (IS_ERR(spin_rq)) {
>   		ret = PTR_ERR(spin_rq);
> -		pr_err("Failed to create spinner request: %d\n", ret);
> +		drm_err(&gt->i915->drm, "Failed to create spinner request: %d\n", ret);
>   		goto err_contexts;
>   	}
>   	ret = request_add_spin(spin_rq, &spin);
>   	if (ret) {
> -		pr_err("Failed to add Spinner request: %d\n", ret);
> +		drm_err(&gt->i915->drm, "Failed to add Spinner request: %d\n", ret);
>   		goto err_spin_rq;
>   	}
>   
> @@ -191,7 +191,7 @@ static int intel_guc_steal_guc_ids(void *arg)
>   		if (IS_ERR(ce[context_index])) {
>   			ret = PTR_ERR(ce[context_index--]);
>   			ce[context_index] = NULL;
> -			pr_err("Failed to create context: %d\n", ret);
> +			drm_err(&gt->i915->drm, "Failed to create context: %d\n", ret);
>   			goto err_spin_rq;
>   		}
>   
> @@ -200,8 +200,8 @@ static int intel_guc_steal_guc_ids(void *arg)
>   			ret = PTR_ERR(rq);
>   			rq = NULL;
>   			if (ret != -EAGAIN) {
> -				pr_err("Failed to create request, %d: %d\n",
> -				       context_index, ret);
> +				drm_err(&gt->i915->drm, "Failed to create request, %d: %d\n",
> +					context_index, ret);
>   				goto err_spin_rq;
>   			}
>   		} else {
> @@ -215,7 +215,7 @@ static int intel_guc_steal_guc_ids(void *arg)
>   	igt_spinner_end(&spin);
>   	ret = intel_selftest_wait_for_rq(spin_rq);
>   	if (ret) {
> -		pr_err("Spin request failed to complete: %d\n", ret);
> +		drm_err(&gt->i915->drm, "Spin request failed to complete: %d\n", ret);
>   		i915_request_put(last);
>   		goto err_spin_rq;
>   	}
> @@ -227,7 +227,7 @@ static int intel_guc_steal_guc_ids(void *arg)
>   	ret = i915_request_wait(last, 0, HZ * 30);
>   	i915_request_put(last);
>   	if (ret < 0) {
> -		pr_err("Last request failed to complete: %d\n", ret);
> +		drm_err(&gt->i915->drm, "Last request failed to complete: %d\n", ret);
>   		goto err_spin_rq;
>   	}
>   
> @@ -235,7 +235,7 @@ static int intel_guc_steal_guc_ids(void *arg)
>   	rq = nop_user_request(ce[context_index], NULL);
>   	if (IS_ERR(rq)) {
>   		ret = PTR_ERR(rq);
> -		pr_err("Failed to steal guc_id, %d: %d\n", context_index, ret);
> +		drm_err(&gt->i915->drm, "Failed to steal guc_id, %d: %d\n", context_index, ret);
>   		goto err_spin_rq;
>   	}
>   
> @@ -243,21 +243,20 @@ static int intel_guc_steal_guc_ids(void *arg)
>   	ret = i915_request_wait(rq, 0, HZ);
>   	i915_request_put(rq);
>   	if (ret < 0) {
> -		pr_err("Request with stolen guc_id failed to complete: %d\n",
> -		       ret);
> +		drm_err(&gt->i915->drm, "Request with stolen guc_id failed to complete: %d\n", ret);
>   		goto err_spin_rq;
>   	}
>   
>   	/* Wait for idle */
>   	ret = intel_gt_wait_for_idle(gt, HZ * 30);
>   	if (ret < 0) {
> -		pr_err("GT failed to idle: %d\n", ret);
> +		drm_err(&gt->i915->drm, "GT failed to idle: %d\n", ret);
>   		goto err_spin_rq;
>   	}
>   
>   	/* Verify a guc_id was stolen */
>   	if (guc->number_guc_id_stolen == number_guc_id_stolen) {
> -		pr_err("No guc_id was stolen");
> +		drm_err(&gt->i915->drm, "No guc_id was stolen");
>   		ret = -EINVAL;
>   	} else {
>   		ret = 0;
> diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c
> index 812220a43df81..d17982c36d256 100644
> --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c
> @@ -115,30 +115,30 @@ static int __intel_guc_multi_lrc_basic(struct intel_gt *gt, unsigned int class)
>   
>   	parent = multi_lrc_create_parent(gt, class, 0);
>   	if (IS_ERR(parent)) {
> -		pr_err("Failed creating contexts: %ld", PTR_ERR(parent));
> +		drm_err(&gt->i915->drm, "Failed creating contexts: %ld", PTR_ERR(parent));
>   		return PTR_ERR(parent);
>   	} else if (!parent) {
> -		pr_debug("Not enough engines in class: %d", class);
> +		drm_dbg(&gt->i915->drm, "Not enough engines in class: %d", class);
>   		return 0;
>   	}
>   
>   	rq = multi_lrc_nop_request(parent);
>   	if (IS_ERR(rq)) {
>   		ret = PTR_ERR(rq);
> -		pr_err("Failed creating requests: %d", ret);
> +		drm_err(&gt->i915->drm, "Failed creating requests: %d", ret);
>   		goto out;
>   	}
>   
>   	ret = intel_selftest_wait_for_rq(rq);
>   	if (ret)
> -		pr_err("Failed waiting on request: %d", ret);
> +		drm_err(&gt->i915->drm, "Failed waiting on request: %d", ret);
>   
>   	i915_request_put(rq);
>   
>   	if (ret >= 0) {
>   		ret = intel_gt_wait_for_idle(gt, HZ * 5);
>   		if (ret < 0)
> -			pr_err("GT failed to idle: %d\n", ret);
> +			drm_err(&gt->i915->drm, "GT failed to idle: %d\n", ret);
>   	}
>   
>   out: