[Intel-gfx] [PATCH 1/3] drm/i915/uc: Remove redundant header_offset/size definitions
Michal Wajdeczko
michal.wajdeczko at intel.com
Fri Jul 26 17:49:29 UTC 2019
On Fri, 26 Jul 2019 19:33:18 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio at intel.com> wrote:
>
>
> 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.
ha! oops, this sneaked into patch 2 instead, will fix
>
>> /* 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.
Blob does not report header size directly, instead it provides set of
sizes of other blob components and we are just verifying if that sums
correctly, but we are using sizeof our definition of struct css.
Since CSS header is part of the fw ABI we should make sure that our
definition is stable and its size matches fw definition (whis is 128)
I'm for keeping 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