Re: [PATCH v3 04/19] drm/i915/guc: Update GuC firmware CSS header

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
To: Michal Wajdeczko <michal.wajdeczko@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 04/19] drm/i915/guc: Update GuC firmware CSS header
Date: Wed, 17 Apr 2019 10:35:17 -0700	[thread overview]
Message-ID: <663d36b8-20d5-8868-e5d9-2cb74b1c57c6@intel.com> (raw)
In-Reply-To: <20190417054004.28176-5-michal.wajdeczko@intel.com>


On 4/16/19 10:39 PM, Michal Wajdeczko wrote:
> There are few minor changes in the CSS header related to the version
> numbering in new GuC firmwares. Update our definition and start using
> common tools for extracting bitfields.
> 
> v2: drop deprecated prod_preprod_fw field, replace unions with bit defs
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: John Spotswood <john.a.spotswood@intel.com>
> Cc: Jeff Mcgee <jeff.mcgee@intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

> ---
>   drivers/gpu/drm/i915/intel_guc_fwif.h | 50 +++++++++------------------
>   drivers/gpu/drm/i915/intel_uc_fw.c    | 20 +++++------
>   2 files changed, 26 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index b2f5148f4f17..4528e098d3a5 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -168,11 +168,7 @@
>    *    in fw. So driver will load a truncated firmware in this case.
>    *
>    * HuC firmware layout is same as GuC firmware.
> - *
> - * HuC firmware css header is different. However, the only difference is where
> - * the version information is saved. The uc_css_header is unified to support
> - * both. Driver should get HuC version from uc_css_header.huc_sw_version, while
> - * uc_css_header.guc_sw_version for GuC.
> + * Only HuC version information is saved in a different way.
>    */
>   
>   struct uc_css_header {
> @@ -183,41 +179,27 @@ struct uc_css_header {
>   	u32 header_version;
>   	u32 module_id;
>   	u32 module_vendor;
> -	union {
> -		struct {
> -			u8 day;
> -			u8 month;
> -			u16 year;
> -		};
> -		u32 date;
> -	};
> +	u32 date;
> +#define CSS_DATE_DAY			(0xFF << 0)
> +#define CSS_DATE_MONTH			(0xFF << 8)
> +#define CSS_DATE_YEAR			(0xFFFF << 16)
>   	u32 size_dw; /* uCode plus header_size_dw */
>   	u32 key_size_dw;
>   	u32 modulus_size_dw;
>   	u32 exponent_size_dw;
> -	union {
> -		struct {
> -			u8 hour;
> -			u8 min;
> -			u16 sec;
> -		};
> -		u32 time;
> -	};
> -
> +	u32 time;
> +#define CSS_TIME_HOUR			(0xFF << 0)
> +#define CSS_DATE_MIN			(0xFF << 8)
> +#define CSS_DATE_SEC			(0xFFFF << 16)
>   	char username[8];
>   	char buildnumber[12];
> -	union {
> -		struct {
> -			u32 branch_client_version;
> -			u32 sw_version;
> -	} guc;
> -		struct {
> -			u32 sw_version;
> -			u32 reserved;
> -	} huc;
> -	};
> -	u32 prod_preprod_fw;
> -	u32 reserved[12];
> +	u32 sw_version;
> +#define CSS_SW_VERSION_GUC_MAJOR	(0xFF << 16)
> +#define CSS_SW_VERSION_GUC_MINOR	(0xFF << 8)
> +#define CSS_SW_VERSION_GUC_PATCH	(0xFF << 0)
> +#define CSS_SW_VERSION_HUC_MAJOR	(0xFFFF << 16)
> +#define CSS_SW_VERSION_HUC_MINOR	(0xFFFF << 0)
> +	u32 reserved[14];
>   	u32 header_info;
>   } __packed;
>   
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
> index becf05ebae4d..957c1feb30d3 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -22,6 +22,7 @@
>    *
>    */
>   
> +#include <linux/bitfield.h>
>   #include <linux/firmware.h>
>   #include <drm/drm_print.h>
>   
> @@ -119,21 +120,20 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>   		goto fail;
>   	}
>   
> -	/*
> -	 * The GuC firmware image has the version number embedded at a
> -	 * well-known offset within the firmware blob; note that major / minor
> -	 * version are TWO bytes each (i.e. u16), although all pointers and
> -	 * offsets are defined in terms of bytes (u8).
> -	 */
> +	/* Get version numbers from the CSS header */
>   	switch (uc_fw->type) {
>   	case INTEL_UC_FW_TYPE_GUC:
> -		uc_fw->major_ver_found = css->guc.sw_version >> 16;
> -		uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF;
> +		uc_fw->major_ver_found = FIELD_GET(CSS_SW_VERSION_GUC_MAJOR,
> +						   css->sw_version);
> +		uc_fw->minor_ver_found = FIELD_GET(CSS_SW_VERSION_GUC_MINOR,
> +						   css->sw_version);
>   		break;
>   
>   	case INTEL_UC_FW_TYPE_HUC:
> -		uc_fw->major_ver_found = css->huc.sw_version >> 16;
> -		uc_fw->minor_ver_found = css->huc.sw_version & 0xFFFF;
> +		uc_fw->major_ver_found = FIELD_GET(CSS_SW_VERSION_HUC_MAJOR,
> +						   css->sw_version);
> +		uc_fw->minor_ver_found = FIELD_GET(CSS_SW_VERSION_HUC_MINOR,
> +						   css->sw_version);
>   		break;
>   
>   	default:
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx