On 03.02.2023 01:11, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> Update a bunch more debug prints to use the new GT based scheme.
>
> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> ---
> 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() ...
> }
>
> 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 ?
> 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
Michal
