Re: [PATCH 2/3] drm/i915/uc: Move uc firmware layout definitions to dedicated file

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 7/24/19 10:34 AM, Michal Wajdeczko wrote:
> Generic uc firmware layout definitions are unlikely to change and
> are separate to other GuC specific definitions.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Keeping things that apply to HuC as well in a generic file seems 
sensible to me.

> ---
>   Documentation/gpu/i915.rst                   |  2 +-
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h  | 70 -----------------
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c     |  1 +
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h | 81 ++++++++++++++++++++
>   4 files changed, 83 insertions(+), 71 deletions(-)
>   create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
> 
> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> index c2173d120492..366cb7f46d17 100644
> --- a/Documentation/gpu/i915.rst
> +++ b/Documentation/gpu/i915.rst
> @@ -448,7 +448,7 @@ GuC-based command submission
>   GuC Firmware Layout
>   -------------------
>   
> -.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>      :doc: GuC Firmware Layout

This is now generic uC firmware layout

>   
>   GuC Address Space
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 30cca3a29323..06a9bdfb0faf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -121,76 +121,6 @@
>   
>   #define GUC_CTL_MAX_DWORDS		(SOFT_SCRATCH_COUNT - 2) /* [1..14] */
>   
> -/**
> - * DOC: GuC Firmware Layout
> - *
> - * The GuC firmware layout looks like this:
> - *
> - *     +-------------------------------+
> - *     |         uc_css_header         |
> - *     |                               |
> - *     | contains major/minor version  |
> - *     +-------------------------------+
> - *     |             uCode             |
> - *     +-------------------------------+
> - *     |         RSA signature         |
> - *     +-------------------------------+
> - *     |          modulus key          |
> - *     +-------------------------------+
> - *     |          exponent val         |
> - *     +-------------------------------+
> - *
> - * The firmware may or may not have modulus key and exponent data. The header,
> - * uCode and RSA signature are must-have components that will be used by driver.
> - * Length of each components, which is all in dwords, can be found in header.
> - * In the case that modulus and exponent are not present in fw, a.k.a truncated
> - * image, the length value still appears in header.
> - *
> - * Driver will do some basic fw size validation based on the following rules:
> - *
> - * 1. Header, uCode and RSA are must-have components.
> - * 2. All firmware components, if they present, are in the sequence illustrated
> - *    in the layout table above.
> - * 3. Length info of each component can be found in header, in dwords.
> - * 4. Modulus and exponent key are not required by driver. They may not appear
> - *    in fw. So driver will load a truncated firmware in this case.
> - *
> - * HuC firmware layout is same as GuC firmware.
> - * Only HuC version information is saved in a different way.
> - */
> -
> -struct uc_css_header {
> -	u32 module_type;
> -	/* header_size includes all non-uCode bits, including css_header, rsa
> -	 * key, modulus key and exponent data. */
> -	u32 header_size_dw;
> -	u32 header_version;
> -	u32 module_id;
> -	u32 module_vendor;
> -	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;
> -	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];
> -	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;
> -
>   /* Work item for submitting workloads into work queue of GuC. */
>   struct guc_wq_item {
>   	u32 header;
> 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 8ce7210907c0..d5cb19b4e5c1 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -27,6 +27,7 @@
>   #include <drm/drm_print.h>
>   
>   #include "intel_uc_fw.h"
> +#include "intel_uc_fw_abi.h"
>   #include "i915_drv.h"
>   
>   /**
> 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
> new file mode 100644
> index 000000000000..3ca535534151
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef _INTEL_UC_FW_ABI_H
> +#define _INTEL_UC_FW_ABI_H
> +
> +#include <linux/types.h>
> +
> +/**
> + * DOC: GuC Firmware Layout
> + *
> + * The GuC firmware layout looks like this:

same here, s/GuC/uC.

> + *
> + *     +-------------------------------+
> + *     |         uc_css_header         |
> + *     |                               |
> + *     | contains major/minor version  |
> + *     +-------------------------------+
> + *     |             uCode             |
> + *     +-------------------------------+
> + *     |         RSA signature         |
> + *     +-------------------------------+
> + *     |          modulus key          |
> + *     +-------------------------------+
> + *     |          exponent val         |
> + *     +-------------------------------+
> + *
> + * The firmware may or may not have modulus key and exponent data. The header,
> + * uCode and RSA signature are must-have components that will be used by driver.
> + * Length of each components, which is all in dwords, can be found in header.
> + * In the case that modulus and exponent are not present in fw, a.k.a truncated
> + * image, the length value still appears in header.
> + *
> + * Driver will do some basic fw size validation based on the following rules:
> + *
> + * 1. Header, uCode and RSA are must-have components.
> + * 2. All firmware components, if they present, are in the sequence illustrated
> + *    in the layout table above.
> + * 3. Length info of each component can be found in header, in dwords.
> + * 4. Modulus and exponent key are not required by driver. They may not appear
> + *    in fw. So driver will load a truncated firmware in this case.
> + *
> + * HuC firmware layout is same as GuC firmware.
> + * Only HuC version information is saved in a different way.

And reword this as something like: "The only difference between GuC and 
HuC firmwares is how the version information is saved"

With that:

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

Daniele

> + */
> +
> +struct uc_css_header {
> +	u32 module_type;
> +	/* header_size includes all non-uCode bits, including css_header, rsa
> +	 * key, modulus key and exponent data. */
> +	u32 header_size_dw;
> +	u32 header_version;
> +	u32 module_id;
> +	u32 module_vendor;
> +	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;
> +	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];
> +	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;
> +
> +#endif /* _INTEL_UC_FW_ABI_H */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx