[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:51:35 UTC 2019
On 7/26/19 10:49 AM, Michal Wajdeczko wrote:
> 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.
>
If the commitment from the uC team is that the size of this will be
stable then I'm ok with keeping the check.
Daniele
>>
>> 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