[Intel-gfx] [PATCH 1/6] drm/i915/guc: More debug print updates - UC firmware
John Harrison
john.c.harrison at intel.com
Fri Feb 3 17:35:35 UTC 2023
On 2/3/2023 01:39, Michal Wajdeczko wrote:
> On 03.02.2023 01:11, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> Update a bunch more debug prints to use the new GT based scheme.
>>
>> Signed-off-by: John Harrison <John.C.Harrison at 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
More information about the Intel-gfx
mailing list