[Intel-gfx] [PATCH 1/3] drm/i915/uc: Remove redundant header_offset/size definitions
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Fri Jul 26 17:33:18 UTC 2019
On 7/26/19 8:58 AM, Michal Wajdeczko wrote:
> According to Firmware layout definition, CSS header is located
> in front of the firmware blob, so header offset is always 0.
> Similarly, size of the CSS header is constant and is 128.
>
> While here, move type/status enums up and keep them together.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 23 ++++++++------------
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 9 ++++----
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h | 2 ++
> 3 files changed, 15 insertions(+), 19 deletions(-)
>
> 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 5fbdd17a864b..66bda0c514a3 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -218,21 +218,18 @@ void intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
>
> css = (struct uc_css_header *)fw->data;
>
> - /* Firmware bits always start from header */
> - uc_fw->header_offset = 0;
> - uc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
> - css->key_size_dw - css->exponent_size_dw) *
> - sizeof(u32);
> -
> - if (uc_fw->header_size != sizeof(struct uc_css_header)) {
> + /* Check integrity of size values inside CSS header */
> + size = (css->header_size_dw - css->key_size_dw - css->modulus_size_dw -
> + css->exponent_size_dw) * sizeof(u32);
> + if (size != sizeof(struct uc_css_header)) {
> DRM_WARN("%s: Mismatched firmware header definition\n",
> intel_uc_fw_type_repr(uc_fw->type));
> err = -ENOEXEC;
> goto fail;
> }
>
> - /* then, uCode */
> - uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
> + /* Firmware blob always starts with the header, then uCode */
> + uc_fw->ucode_offset = sizeof(struct uc_css_header);
> uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
>
> /* now RSA */
> @@ -246,7 +243,7 @@ void intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
> uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
>
> /* At least, it should have header, uCode and RSA. Size of all three. */
> - size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
> + size = sizeof(struct uc_css_header) + uc_fw->ucode_size + uc_fw->rsa_size;
> if (fw->size < size) {
> DRM_WARN("%s: Truncated firmware (%zu, expected %zu)\n",
> intel_uc_fw_type_repr(uc_fw->type), fw->size, size);
> @@ -371,7 +368,7 @@ static int uc_fw_xfer(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
> intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
>
> /* Set the source address for the uCode */
> - offset = uc_fw_ggtt_offset(uc_fw, gt->ggtt) + uc_fw->header_offset;
> + offset = uc_fw_ggtt_offset(uc_fw, gt->ggtt);
> GEM_BUG_ON(upper_32_bits(offset) & 0xFFFF0000);
> intel_uncore_write_fw(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset));
> intel_uncore_write_fw(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset));
> @@ -385,7 +382,7 @@ static int uc_fw_xfer(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
> * via DMA, excluding any other components
> */
> intel_uncore_write_fw(uncore, DMA_COPY_SIZE,
> - uc_fw->header_size + uc_fw->ucode_size);
> + uc_fw->ucode_offset + uc_fw->ucode_size);
in other places you've replaced uc_fw->header_size with sizeof(struct
uc_css_header), I'd do the same here for consistency.
>
> /* Start the DMA */
> intel_uncore_write_fw(uncore, DMA_CTRL,
> @@ -539,8 +536,6 @@ void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p)
> drm_printf(p, "\tversion: wanted %u.%u, found %u.%u\n",
> uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted,
> uc_fw->major_ver_found, uc_fw->minor_ver_found);
> - drm_printf(p, "\theader: offset %u, size %u\n",
> - uc_fw->header_offset, uc_fw->header_size);
> drm_printf(p, "\tuCode: offset %u, size %u\n",
> uc_fw->ucode_offset, uc_fw->ucode_size);
> drm_printf(p, "\tRSA: offset %u, size %u\n",
> 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 eddbb237fabe..a8048f91f0da 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -26,6 +26,7 @@
> #define _INTEL_UC_FW_H_
>
> #include <linux/types.h>
> +#include "intel_uc_fw_abi.h"
> #include "i915_gem.h"
>
> struct drm_printer;
> @@ -57,10 +58,11 @@ enum intel_uc_fw_type {
> * of fetching, caching, and loading the firmware image into the uC.
> */
> struct intel_uc_fw {
> + enum intel_uc_fw_type type;
> + enum intel_uc_fw_status status;
> const char *path;
> size_t size;
> struct drm_i915_gem_object *obj;
> - enum intel_uc_fw_status status;
>
> /*
> * The firmware build process will generate a version header file with major and
> @@ -72,9 +74,6 @@ struct intel_uc_fw {
> u16 major_ver_found;
> u16 minor_ver_found;
>
> - enum intel_uc_fw_type type;
> - u32 header_size;
> - u32 header_offset;
> u32 rsa_size;
> u32 rsa_offset;
> u32 ucode_size;
> @@ -163,7 +162,7 @@ static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
> if (!intel_uc_fw_is_available(uc_fw))
> return 0;
>
> - return uc_fw->header_size + uc_fw->ucode_size;
> + return sizeof(struct uc_css_header) + uc_fw->ucode_size;
> }
>
> void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
> 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 545e86c52a9e..ae58e8a8c53b 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
> @@ -7,6 +7,7 @@
> #define _INTEL_UC_FW_ABI_H
>
> #include <linux/types.h>
> +#include <linux/build_bug.h>
>
> /**
> * DOC: Firmware Layout
> @@ -76,5 +77,6 @@ struct uc_css_header {
> u32 reserved[14];
> u32 header_info;
> } __packed;
> +static_assert(sizeof(struct uc_css_header) == 128);
Do we really care that the size of this is 128? the important thing is
that it matches what the blob reports and you have a check for that
already so we should be able to avoid this check.
With the 2 nits addressed:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
Daniele
>
> #endif /* _INTEL_UC_FW_ABI_H */
>
More information about the Intel-gfx
mailing list