diff mbox series

[v2,2/3] drm/i915/uc: Add patch level version number support

Message ID 20220826030553.2611574-3-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series Drop version numbers from firmware files | expand

Commit Message

John Harrison Aug. 26, 2022, 3:05 a.m. UTC
From: John Harrison <John.C.Harrison@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@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(-)

Comments

Daniele Ceraolo Spurio Aug. 26, 2022, 11:54 p.m. UTC | #1
On 8/25/2022 8:05 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@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@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

>   
>   #endif /* _INTEL_UC_FW_ABI_H */
John Harrison Aug. 27, 2022, 1:07 a.m. UTC | #2
On 8/26/2022 16:54, Ceraolo Spurio, Daniele wrote:
> On 8/25/2022 8:05 PM, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@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@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 */
>
diff mbox series

Patch

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))
+
 #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)
 
 #endif /* _INTEL_UC_FW_ABI_H */