| Message ID | 20220715004028.2126239-1-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc: Don't use pr_err when not necessary | expand |
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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->i915->drm, "GT failed to idle: %d\n", ret); > } > > out:
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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->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(>->i915->drm, "GT failed to idle: %d\n", ret); } out: