[Intel-gfx] [PATCH v2 2/3] drm/i915/uc: Add patch level version number support
John Harrison
john.c.harrison at intel.com
Sat Aug 27 01:07:24 UTC 2022
On 8/26/2022 16:54, Ceraolo Spurio, Daniele wrote:
> On 8/25/2022 8:05 PM, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> With the move to un-versioned filenames, it becomes more difficult to
>> know exactly what version of a given firmware is being used. So add
>> the patch level version number to the debugfs output.
>>
>> Also, support matching by patch level when selecting code paths for
>> firmware compatibility. While a patch level change cannot be backwards
>> breaking, it is potentially possible that a new feature only works
>> from a given patch level onwards (even though it was theoretically
>> added in an earlier version that bumped the major or minor version).
>>
>> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>> ---
>> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 10 +++---
>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 ++--
>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 36 ++++++++++++++-----
>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 6 ++++
>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h | 8 +++--
>> 5 files changed, 47 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> index 04393932623c7..64c4e83153f47 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> @@ -1868,7 +1868,7 @@ int intel_guc_submission_init(struct intel_guc
>> *guc)
>> if (guc->submission_initialized)
>> return 0;
>> - if (guc->fw.file_selected.major_ver < 70) {
>> + if (GET_UC_VER(guc) < MAKE_UC_VER(70, 0, 0)) {
>> ret = guc_lrc_desc_pool_create_v69(guc);
>> if (ret)
>> return ret;
>> @@ -2303,7 +2303,7 @@ static int register_context(struct
>> intel_context *ce, bool loop)
>> GEM_BUG_ON(intel_context_is_child(ce));
>> trace_intel_context_register(ce);
>> - if (guc->fw.file_selected.major_ver >= 70)
>> + if (GET_UC_VER(guc) >= MAKE_UC_VER(70, 0, 0))
>> ret = register_context_v70(guc, ce, loop);
>> else
>> ret = register_context_v69(guc, ce, loop);
>> @@ -2315,7 +2315,7 @@ static int register_context(struct
>> intel_context *ce, bool loop)
>> set_context_registered(ce);
>> spin_unlock_irqrestore(&ce->guc_state.lock, flags);
>> - if (guc->fw.file_selected.major_ver >= 70)
>> + if (GET_UC_VER(guc) >= MAKE_UC_VER(70, 0, 0))
>> guc_context_policy_init_v70(ce, loop);
>> }
>> @@ -2921,7 +2921,7 @@ static void
>> __guc_context_set_preemption_timeout(struct intel_guc *guc,
>> u16 guc_id,
>> u32 preemption_timeout)
>> {
>> - if (guc->fw.file_selected.major_ver >= 70) {
>> + if (GET_UC_VER(guc) >= MAKE_UC_VER(70, 0, 0)) {
>> struct context_policy policy;
>> __guc_context_policy_start_klv(&policy, guc_id);
>> @@ -3186,7 +3186,7 @@ static int guc_context_alloc(struct
>> intel_context *ce)
>> static void __guc_context_set_prio(struct intel_guc *guc,
>> struct intel_context *ce)
>> {
>> - if (guc->fw.file_selected.major_ver >= 70) {
>> + if (GET_UC_VER(guc) >= MAKE_UC_VER(70, 0, 0)) {
>> struct context_policy policy;
>> __guc_context_policy_start_klv(&policy, ce->guc_id.id);
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index d965ac4832d60..abf4e142596d0 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -435,9 +435,11 @@ static void print_fw_ver(struct intel_uc *uc,
>> struct intel_uc_fw *fw)
>> {
>> struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
>> - drm_info(&i915->drm, "%s firmware %s version %u.%u\n",
>> + drm_info(&i915->drm, "%s firmware %s version %u.%u.%u\n",
>> intel_uc_fw_type_repr(fw->type), fw->file_selected.path,
>> - fw->file_selected.major_ver, fw->file_selected.minor_ver);
>> + fw->file_selected.major_ver,
>> + fw->file_selected.minor_ver,
>> + fw->file_selected.patch_ver);
>> }
>> static int __uc_init_hw(struct intel_uc *uc)
>> 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 94cf2d4a46e6f..7c45c097d6845 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -447,10 +447,12 @@ static int check_gsc_manifest(const struct
>> firmware *fw,
>> struct intel_uc_fw *uc_fw)
>> {
>> u32 *dw = (u32 *)fw->data;
>> - u32 version = dw[HUC_GSC_VERSION_DW];
>> + u32 version_hi = dw[HUC_GSC_VERSION_HI_DW];
>> + u32 version_lo = dw[HUC_GSC_VERSION_LO_DW];
>> - uc_fw->file_selected.major_ver =
>> FIELD_GET(HUC_GSC_MAJOR_VER_MASK, version);
>> - uc_fw->file_selected.minor_ver =
>> FIELD_GET(HUC_GSC_MINOR_VER_MASK, version);
>> + uc_fw->file_selected.major_ver =
>> FIELD_GET(HUC_GSC_MAJOR_VER_HI_MASK, version_hi);
>> + uc_fw->file_selected.minor_ver =
>> FIELD_GET(HUC_GSC_MINOR_VER_HI_MASK, version_hi);
>> + uc_fw->file_selected.patch_ver =
>> FIELD_GET(HUC_GSC_PATCH_VER_LO_MASK, version_lo);
>> return 0;
>> }
>> @@ -512,6 +514,8 @@ static int check_ccs_header(struct
>> drm_i915_private *i915,
>> css->sw_version);
>> uc_fw->file_selected.minor_ver =
>> FIELD_GET(CSS_SW_VERSION_UC_MINOR,
>> css->sw_version);
>> + uc_fw->file_selected.patch_ver = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
>> + css->sw_version);
>> if (uc_fw->type == INTEL_UC_FW_TYPE_GUC)
>> uc_fw->private_data_size = css->private_data_size;
>> @@ -1000,6 +1004,8 @@ size_t intel_uc_fw_copy_rsa(struct intel_uc_fw
>> *uc_fw, void *dst, u32 max_len)
>> */
>> void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct
>> drm_printer *p)
>> {
>> + u32 ver_sel, ver_want;
>> +
>> drm_printf(p, "%s firmware: %s\n",
>> intel_uc_fw_type_repr(uc_fw->type),
>> uc_fw->file_selected.path);
>> if (uc_fw->file_selected.path != uc_fw->file_wanted.path)
>> @@ -1007,13 +1013,25 @@ void intel_uc_fw_dump(const struct
>> intel_uc_fw *uc_fw, struct drm_printer *p)
>> intel_uc_fw_type_repr(uc_fw->type),
>> uc_fw->file_wanted.path);
>> drm_printf(p, "\tstatus: %s\n",
>> intel_uc_fw_status_repr(uc_fw->status));
>> - if (uc_fw->file_wanted.major_ver)
>> - drm_printf(p, "\tversion: wanted %u.%u, found %u.%u\n",
>> - uc_fw->file_wanted.major_ver,
>> uc_fw->file_wanted.minor_ver,
>> - uc_fw->file_selected.major_ver,
>> uc_fw->file_selected.minor_ver);
>> + ver_sel = MAKE_UC_VER(uc_fw->file_selected.major_ver,
>> + uc_fw->file_selected.minor_ver,
>> + uc_fw->file_selected.patch_ver);
>> + ver_want = MAKE_UC_VER(uc_fw->file_wanted.major_ver,
>> + uc_fw->file_wanted.minor_ver,
>> + uc_fw->file_wanted.patch_ver);
>> + if (ver_sel < ver_want)
>> + drm_printf(p, "\tversion: wanted %u.%u.%u, found %u.%u.%u\n",
>> + uc_fw->file_wanted.major_ver,
>> + uc_fw->file_wanted.minor_ver,
>> + uc_fw->file_wanted.patch_ver,
>> + uc_fw->file_selected.major_ver,
>> + uc_fw->file_selected.minor_ver,
>> + uc_fw->file_selected.patch_ver);
>> else
>> - drm_printf(p, "\tversion: found %u.%u\n",
>> - uc_fw->file_selected.major_ver,
>> uc_fw->file_selected.minor_ver);
>> + drm_printf(p, "\tversion: found %u.%u.%u\n",
>> + uc_fw->file_selected.major_ver,
>> + uc_fw->file_selected.minor_ver,
>> + uc_fw->file_selected.patch_ver);
>> drm_printf(p, "\tuCode: %u bytes\n", uc_fw->ucode_size);
>> drm_printf(p, "\tRSA: %u bytes\n", uc_fw->rsa_size);
>> }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> index 344763c942e37..cb586f7df270b 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> @@ -73,6 +73,7 @@ struct intel_uc_fw_file {
>> const char *path;
>> u16 major_ver;
>> u16 minor_ver;
>> + u16 patch_ver;
>> };
>> /*
>> @@ -108,6 +109,11 @@ struct intel_uc_fw {
>> bool loaded_via_gsc;
>> };
>> +#define MAKE_UC_VER(maj, min, pat) ((pat) | ((min) << 8) |
>> ((maj) << 16))
>> +#define GET_UC_VER(uc) (MAKE_UC_VER((uc)->fw.file_selected.major_ver, \
>> + (uc)->fw.file_selected.minor_ver, \
>> + (uc)->fw.file_selected.patch_ver))
>
> Might be worth saving this in a variable to not have to recalculate it
> each time. not a blocker.
>
>> +
>> #ifdef CONFIG_DRM_I915_DEBUG_GUC
>> void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
>> enum intel_uc_fw_status status);
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>> index b05e0e35b734c..f214d24fbcf0d 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>> @@ -83,8 +83,10 @@ struct uc_css_header {
>> } __packed;
>> static_assert(sizeof(struct uc_css_header) == 128);
>> -#define HUC_GSC_VERSION_DW 44
>> -#define HUC_GSC_MAJOR_VER_MASK (0xFF << 0)
>> -#define HUC_GSC_MINOR_VER_MASK (0xFF << 16)
>> +#define HUC_GSC_VERSION_HI_DW 44
>> +#define HUC_GSC_MAJOR_VER_HI_MASK (0xFF << 0)
>> +#define HUC_GSC_MINOR_VER_HI_MASK (0xFF << 16)
>> +#define HUC_GSC_VERSION_LO_DW 45
>> +#define HUC_GSC_PATCH_VER_LO_MASK (0xFF << 16)
>
> AFAICS the patch version is in the lower 16 bits here, while the
> higher 16 are the build number. e.g for dg2_7.10.0_gsc.bin (available
> in drm-firmware) I see:
>
> [00000b0] 0007 000a 0000 0574
>
> Daniele
Hmm. You would indeed be correct. I swear I tested it. But probably I
tested on a non-GSC version or something!
John.
>
>> #endif /* _INTEL_UC_FW_ABI_H */
>
More information about the Intel-gfx
mailing list