From: John Harrison <john.c.harrison@intel.com>
To: Michal Wajdeczko <michal.wajdeczko@intel.com>,
<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 1/6] drm/i915/guc: More debug print updates - UC firmware
Date: Fri, 3 Feb 2023 09:35:35 -0800 [thread overview]
Message-ID: <c9fc1ce1-15a0-0849-3c93-0c98a21220e3@intel.com> (raw)
In-Reply-To: <8115b355-53a1-6337-3984-ec638ebda674@intel.com>
On 2/3/2023 01:39, Michal Wajdeczko wrote:
> On 03.02.2023 01:11, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Update a bunch more debug prints to use the new GT based scheme.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 42 ++++----
>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 116 +++++++++++------------
>> 2 files changed, 73 insertions(+), 85 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index de7f987cf6111..6648691bd6450 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -83,15 +83,15 @@ static int __intel_uc_reset_hw(struct intel_uc *uc)
>>
>> static void __confirm_options(struct intel_uc *uc)
>> {
>> - struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
>> + struct intel_gt *gt = uc_to_gt(uc);
>> + struct drm_i915_private *i915 = gt->i915;
>>
>> - drm_dbg(&i915->drm,
>> - "enable_guc=%d (guc:%s submission:%s huc:%s slpc:%s)\n",
>> - i915->params.enable_guc,
>> - str_yes_no(intel_uc_wants_guc(uc)),
>> - str_yes_no(intel_uc_wants_guc_submission(uc)),
>> - str_yes_no(intel_uc_wants_huc(uc)),
>> - str_yes_no(intel_uc_wants_guc_slpc(uc)));
>> + gt_dbg(gt, "enable_guc=%d (guc:%s submission:%s huc:%s slpc:%s)\n",
>> + i915->params.enable_guc,
>> + str_yes_no(intel_uc_wants_guc(uc)),
>> + str_yes_no(intel_uc_wants_guc_submission(uc)),
>> + str_yes_no(intel_uc_wants_huc(uc)),
>> + str_yes_no(intel_uc_wants_guc_slpc(uc)));
>>
>> if (i915->params.enable_guc == 0) {
>> GEM_BUG_ON(intel_uc_wants_guc(uc));
>> @@ -102,26 +102,22 @@ static void __confirm_options(struct intel_uc *uc)
>> }
>>
>> if (!intel_uc_supports_guc(uc))
>> - drm_info(&i915->drm,
>> - "Incompatible option enable_guc=%d - %s\n",
>> - i915->params.enable_guc, "GuC is not supported!");
>> + gt_info(gt, "Incompatible option enable_guc=%d - %s\n",
>> + i915->params.enable_guc, "GuC is not supported!");
>>
>> if (i915->params.enable_guc & ENABLE_GUC_LOAD_HUC &&
>> !intel_uc_supports_huc(uc))
>> - drm_info(&i915->drm,
>> - "Incompatible option enable_guc=%d - %s\n",
>> - i915->params.enable_guc, "HuC is not supported!");
>> + gt_info(gt, "Incompatible option enable_guc=%d - %s\n",
>> + i915->params.enable_guc, "HuC is not supported!");
>>
>> if (i915->params.enable_guc & ENABLE_GUC_SUBMISSION &&
>> !intel_uc_supports_guc_submission(uc))
>> - drm_info(&i915->drm,
>> - "Incompatible option enable_guc=%d - %s\n",
>> - i915->params.enable_guc, "GuC submission is N/A");
>> + gt_info(gt, "Incompatible option enable_guc=%d - %s\n",
>> + i915->params.enable_guc, "GuC submission is N/A");
>>
>> if (i915->params.enable_guc & ~ENABLE_GUC_MASK)
>> - drm_info(&i915->drm,
>> - "Incompatible option enable_guc=%d - %s\n",
>> - i915->params.enable_guc, "undocumented flag");
>> + gt_info(gt, "Incompatible option enable_guc=%d - %s\n",
>> + i915->params.enable_guc, "undocumented flag");
> all these above messages are about i915->params so IMHO using drm_info()
> is still more applicable than gt_info() ...
But they are explaining the path taken on a particular GT to reach the
final decision about what to load or run. That could be important. E.g.
on MTL render tile does not have HuC so the decisions will be different
even though the modparams are the same.
>
>> }
>>
>> void intel_uc_init_early(struct intel_uc *uc)
>> @@ -549,10 +545,8 @@ static int __uc_init_hw(struct intel_uc *uc)
>>
>> intel_gsc_uc_load_start(&uc->gsc);
>>
>> - gt_info(gt, "GuC submission %s\n",
>> - str_enabled_disabled(intel_uc_uses_guc_submission(uc)));
>> - gt_info(gt, "GuC SLPC %s\n",
>> - str_enabled_disabled(intel_uc_uses_guc_slpc(uc)));
>> + guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc)));
>> + guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc)));
>>
>> return 0;
>>
>> 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 65672ff826054..7d2558d53e972 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -11,6 +11,7 @@
>> #include <drm/drm_print.h>
>>
>> #include "gem/i915_gem_lmem.h"
>> +#include "gt/intel_gt_print.h"
>> #include "intel_uc_fw.h"
>> #include "intel_uc_fw_abi.h"
>> #include "i915_drv.h"
>> @@ -44,11 +45,10 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
>> enum intel_uc_fw_status status)
>> {
>> uc_fw->__status = status;
>> - drm_dbg(&__uc_fw_to_gt(uc_fw)->i915->drm,
>> - "%s firmware -> %s\n",
>> - intel_uc_fw_type_repr(uc_fw->type),
>> - status == INTEL_UC_FIRMWARE_SELECTED ?
>> - uc_fw->file_selected.path : intel_uc_fw_status_repr(status));
>> + gt_dbg(__uc_fw_to_gt(uc_fw), "%s firmware -> %s\n",
>> + intel_uc_fw_type_repr(uc_fw->type),
>> + status == INTEL_UC_FIRMWARE_SELECTED ?
>> + uc_fw->file_selected.path : intel_uc_fw_status_repr(status));
>> }
>> #endif
>>
>> @@ -562,15 +562,14 @@ static int check_ccs_header(struct intel_gt *gt,
>> const struct firmware *fw,
>> struct intel_uc_fw *uc_fw)
>> {
>> - struct drm_i915_private *i915 = gt->i915;
>> struct uc_css_header *css;
>> size_t size;
>>
>> /* Check the size of the blob before examining buffer contents */
>> if (unlikely(fw->size < sizeof(struct uc_css_header))) {
>> - drm_warn(&i915->drm, "%s firmware %s: invalid size: %zu < %zu\n",
>> - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> - fw->size, sizeof(struct uc_css_header));
>> + gt_warn(gt, "%s firmware %s: invalid size: %zu < %zu\n",
>> + intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> + fw->size, sizeof(struct uc_css_header));
>> return -ENODATA;
>> }
>>
>> @@ -580,10 +579,9 @@ static int check_ccs_header(struct intel_gt *gt,
>> size = (css->header_size_dw - css->key_size_dw - css->modulus_size_dw -
>> css->exponent_size_dw) * sizeof(u32);
>> if (unlikely(size != sizeof(struct uc_css_header))) {
>> - drm_warn(&i915->drm,
>> - "%s firmware %s: unexpected header size: %zu != %zu\n",
>> - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> - fw->size, sizeof(struct uc_css_header));
>> + gt_warn(gt, "%s firmware %s: unexpected header size: %zu != %zu\n",
>> + intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> + fw->size, sizeof(struct uc_css_header));
>> return -EPROTO;
>> }
>>
>> @@ -596,18 +594,18 @@ static int check_ccs_header(struct intel_gt *gt,
>> /* At least, it should have header, uCode and RSA. Size of all three. */
>> size = sizeof(struct uc_css_header) + uc_fw->ucode_size + uc_fw->rsa_size;
>> if (unlikely(fw->size < size)) {
>> - drm_warn(&i915->drm, "%s firmware %s: invalid size: %zu < %zu\n",
>> - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> - fw->size, size);
>> + gt_warn(gt, "%s firmware %s: invalid size: %zu < %zu\n",
>> + intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> + fw->size, size);
>> return -ENOEXEC;
>> }
>>
>> /* Sanity check whether this fw is not larger than whole WOPCM memory */
>> size = __intel_uc_fw_get_upload_size(uc_fw);
>> if (unlikely(size >= gt->wopcm.size)) {
>> - drm_warn(&i915->drm, "%s firmware %s: invalid size: %zu > %zu\n",
>> - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> - size, (size_t)gt->wopcm.size);
>> + gt_warn(gt, "%s firmware %s: invalid size: %zu > %zu\n",
>> + intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> + size, (size_t)gt->wopcm.size);
>> return -E2BIG;
>> }
>>
>> @@ -635,20 +633,20 @@ static bool guc_check_version_range(struct intel_uc_fw *uc_fw)
>> */
>>
> this is GuC specific function, shouldn't we use guc_warn() instead ?
Much/some of this code is not actually GuC specific. Also, GuC can
sometimes mean Springboard. And there's always the potential for bugs to
cause the wrong code path to occur. So I would rather the keep the '%s
firmware' string rather than forcing it to be a fixed prefix. Which
means that switching to guc_warn will duplicate the GuC prefix.
>
>> if (!is_ver_8bit(&uc_fw->file_selected.ver)) {
>> - drm_warn(&__uc_fw_to_gt(uc_fw)->i915->drm, "%s firmware: invalid file version: 0x%02X:%02X:%02X\n",
>> - intel_uc_fw_type_repr(uc_fw->type),
>> - uc_fw->file_selected.ver.major,
>> - uc_fw->file_selected.ver.minor,
>> - uc_fw->file_selected.ver.patch);
>> + gt_warn(__uc_fw_to_gt(uc_fw), "%s firmware: invalid file version: 0x%02X:%02X:%02X\n",
>> + intel_uc_fw_type_repr(uc_fw->type),
>> + uc_fw->file_selected.ver.major,
>> + uc_fw->file_selected.ver.minor,
>> + uc_fw->file_selected.ver.patch);
>> return false;
>> }
>>
>> if (!is_ver_8bit(&guc->submission_version)) {
>> - drm_warn(&__uc_fw_to_gt(uc_fw)->i915->drm, "%s firmware: invalid submit version: 0x%02X:%02X:%02X\n",
>> - intel_uc_fw_type_repr(uc_fw->type),
>> - guc->submission_version.major,
>> - guc->submission_version.minor,
>> - guc->submission_version.patch);
>> + gt_warn(__uc_fw_to_gt(uc_fw), "%s firmware: invalid submit version: 0x%02X:%02X:%02X\n",
>> + intel_uc_fw_type_repr(uc_fw->type),
>> + guc->submission_version.major,
>> + guc->submission_version.minor,
>> + guc->submission_version.patch);
>> return false;
>> }
>>
>> @@ -687,10 +685,9 @@ static int try_firmware_load(struct intel_uc_fw *uc_fw, const struct firmware **
>> return err;
>>
>> if ((*fw)->size > INTEL_UC_RSVD_GGTT_PER_FW) {
>> - drm_err(>->i915->drm,
>> - "%s firmware %s: size (%zuKB) exceeds max supported size (%uKB)\n",
>> - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> - (*fw)->size / SZ_1K, INTEL_UC_RSVD_GGTT_PER_FW / SZ_1K);
>> + gt_err(gt, "%s firmware %s: size (%zuKB) exceeds max supported size (%uKB)\n",
>> + intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> + (*fw)->size / SZ_1K, INTEL_UC_RSVD_GGTT_PER_FW / SZ_1K);
>>
>> /* try to find another blob to load */
>> release_firmware(*fw);
>> @@ -768,10 +765,10 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>> if (uc_fw->file_wanted.ver.major && uc_fw->file_selected.ver.major) {
>> /* Check the file's major version was as it claimed */
>> if (uc_fw->file_selected.ver.major != uc_fw->file_wanted.ver.major) {
>> - drm_notice(&i915->drm, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
>> - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> - uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
>> - uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
>> + gt_notice(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
>> + intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> + uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
>> + uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
>> if (!intel_uc_fw_is_overridden(uc_fw)) {
>> err = -ENOEXEC;
>> goto fail;
>> @@ -786,16 +783,14 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>> /* Preserve the version that was really wanted */
>> memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
>>
>> - drm_notice(&i915->drm,
>> - "%s firmware %s (%d.%d) is recommended, but only %s (%d.%d) was found\n",
>> - intel_uc_fw_type_repr(uc_fw->type),
>> - uc_fw->file_wanted.path,
>> - uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor,
>> - uc_fw->file_selected.path,
>> - uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor);
>> - drm_info(&i915->drm,
>> - "Consider updating your linux-firmware pkg or downloading from %s\n",
>> - INTEL_UC_FIRMWARE_URL);
>> + gt_notice(gt, "%s firmware %s (%d.%d) is recommended, but only %s (%d.%d) was found\n",
>> + intel_uc_fw_type_repr(uc_fw->type),
>> + uc_fw->file_wanted.path,
>> + uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor,
>> + uc_fw->file_selected.path,
>> + uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor);
>> + gt_info(gt, "Consider updating your linux-firmware pkg or downloading from %s\n",
>> + INTEL_UC_FIRMWARE_URL);
>> }
>>
>> if (HAS_LMEM(i915)) {
>> @@ -823,10 +818,10 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>> INTEL_UC_FIRMWARE_MISSING :
>> INTEL_UC_FIRMWARE_ERROR);
>>
>> - i915_probe_error(i915, "%s firmware %s: fetch failed with error %d\n",
>> - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, err);
>> - drm_info(&i915->drm, "%s firmware(s) can be downloaded from %s\n",
>> - intel_uc_fw_type_repr(uc_fw->type), INTEL_UC_FIRMWARE_URL);
>> + gt_probe_error(gt, "%s firmware %s: fetch failed with error %d\n",
>> + intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, err);
>> + gt_info(gt, "%s firmware(s) can be downloaded from %s\n",
>> + intel_uc_fw_type_repr(uc_fw->type), INTEL_UC_FIRMWARE_URL);
>>
>> release_firmware(fw); /* OK even if fw is NULL */
>> return err;
>> @@ -932,9 +927,9 @@ static int uc_fw_xfer(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags)
>> /* Wait for DMA to finish */
>> ret = intel_wait_for_register_fw(uncore, DMA_CTRL, START_DMA, 0, 100);
>> if (ret)
>> - drm_err(>->i915->drm, "DMA for %s fw failed, DMA_CTRL=%u\n",
>> - intel_uc_fw_type_repr(uc_fw->type),
>> - intel_uncore_read_fw(uncore, DMA_CTRL));
>> + gt_err(gt, "DMA for %s fw failed, DMA_CTRL=%u\n",
>> + intel_uc_fw_type_repr(uc_fw->type),
>> + intel_uncore_read_fw(uncore, DMA_CTRL));
>>
>> /* Disable the bits once DMA is over */
>> intel_uncore_write_fw(uncore, DMA_CTRL, _MASKED_BIT_DISABLE(dma_flags));
>> @@ -950,9 +945,8 @@ int intel_uc_fw_mark_load_failed(struct intel_uc_fw *uc_fw, int err)
>>
>> GEM_BUG_ON(!intel_uc_fw_is_loadable(uc_fw));
>>
>> - i915_probe_error(gt->i915, "Failed to load %s firmware %s (%d)\n",
>> - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>> - err);
>> + gt_probe_error(gt, "Failed to load %s firmware %s (%d)\n",
>> + intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, err);
>> intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
>>
>> return err;
>> @@ -1078,15 +1072,15 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
>>
>> err = i915_gem_object_pin_pages_unlocked(uc_fw->obj);
>> if (err) {
>> - DRM_DEBUG_DRIVER("%s fw pin-pages err=%d\n",
>> - intel_uc_fw_type_repr(uc_fw->type), err);
>> + gt_dbg(__uc_fw_to_gt(uc_fw), "%s fw pin-pages err=%d\n",
>> + intel_uc_fw_type_repr(uc_fw->type), err);
>> goto out;
>> }
>>
>> err = uc_fw_rsa_data_create(uc_fw);
>> if (err) {
>> - DRM_DEBUG_DRIVER("%s fw rsa data creation failed, err=%d\n",
>> - intel_uc_fw_type_repr(uc_fw->type), err);
>> + gt_dbg(__uc_fw_to_gt(uc_fw), "%s fw rsa data creation failed, err=%d\n",
>> + intel_uc_fw_type_repr(uc_fw->type), err);
>> goto out_unpin;
>> }
>>
> rest of the patch LGTM except that since we are around and to be more
> friendly I would use %pe to show error codes
Yeah, missed those.
John.
>
> Michal
next prev parent reply other threads:[~2023-02-03 17:35 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-03 0:11 [PATCH 0/6] More drm_dbg to guc_dbg changes John.C.Harrison
2023-02-03 0:11 ` [Intel-gfx] " John.C.Harrison
2023-02-03 0:11 ` [PATCH 1/6] drm/i915/guc: More debug print updates - UC firmware John.C.Harrison
2023-02-03 0:11 ` [Intel-gfx] " John.C.Harrison
2023-02-03 9:39 ` Michal Wajdeczko
2023-02-03 17:35 ` John Harrison [this message]
2023-02-03 0:11 ` [PATCH 2/6] drm/i915/guc: More debug print updates - GSC firmware John.C.Harrison
2023-02-03 0:11 ` [Intel-gfx] " John.C.Harrison
2023-02-03 9:43 ` Michal Wajdeczko
2023-02-03 9:43 ` [Intel-gfx] " Michal Wajdeczko
2023-02-03 0:11 ` [PATCH 3/6] drm/i915/guc: More debug print updates - GuC reg capture John.C.Harrison
2023-02-03 0:11 ` [Intel-gfx] " John.C.Harrison
2023-02-04 8:19 ` Teres Alexis, Alan Previn
2023-02-07 2:55 ` John Harrison
2023-02-03 0:11 ` [PATCH 4/6] drm/i915/guc: More debug print updates - GuC selftests John.C.Harrison
2023-02-03 0:11 ` [Intel-gfx] " John.C.Harrison
2023-02-03 9:54 ` Michal Wajdeczko
2023-02-03 17:43 ` John Harrison
2023-02-03 0:11 ` [PATCH 5/6] drm/i915/guc: More debug print updates - GuC SLPC John.C.Harrison
2023-02-03 0:11 ` [Intel-gfx] " John.C.Harrison
2023-02-03 10:00 ` Michal Wajdeczko
2023-02-03 10:00 ` [Intel-gfx] " Michal Wajdeczko
2023-02-03 0:11 ` [PATCH 6/6] drm/i915/guc: More debug print updates - GuC logging John.C.Harrison
2023-02-03 0:11 ` [Intel-gfx] " John.C.Harrison
2023-02-03 10:02 ` Michal Wajdeczko
2023-02-03 10:02 ` [Intel-gfx] " Michal Wajdeczko
2023-02-03 1:18 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for More drm_dbg to guc_dbg changes Patchwork
2023-02-03 12:24 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c9fc1ce1-15a0-0849-3c93-0c98a21220e3@intel.com \
--to=john.c.harrison@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=michal.wajdeczko@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.