Re: [PATCH v3 2/6] drm/i915/huc: Unified css_header struct for GuC and HuC

Peter Antoine
2016-07-07T10:09:36Z

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7207 bytes --]

On Wed, 6 Jul 2016, Vivi, Rodrigo wrote:

> On Wed, 2016-07-06 at 15:24 +0100, Peter Antoine wrote:
>> HuC firmware css header has almost exactly same definition as GuC
>> firmware except for the sw_version. Also, add a new member fw_type
>> into intel_uc_fw to indicate what kind of fw it is. So, the loader
>> will pull right sw_version from header.
>>
>> v2: rebased on-top of drm-intel-nightly
>> v3: rebased on-top of drm-intel-nightly (again).
>>
>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc.h        |  4 ++++
>>  drivers/gpu/drm/i915/intel_guc_fwif.h   | 16 ++++++++++---
>>  drivers/gpu/drm/i915/intel_guc_loader.c | 40 ++++++++++++++++++++++-
>> ----------
>>  3 files changed, 44 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 02adcfc..ebf9c8d 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -97,6 +97,9 @@ enum intel_uc_fw_status {
>>   UC_FIRMWARE_SUCCESS
>>  };
>>
>> +#define UC_FW_TYPE_GUC 0
>> +#define UC_FW_TYPE_HUC 1
>> +
Yes.
I'll change it.

Peter.

>
> couldn't this be an enum?
>
>>  /*
>>   * This structure encapsulates all the data needed during the
>> process
>>   * of fetching, caching, and loading the firmware image into the
>> GuC.
>> @@ -114,6 +117,7 @@ struct intel_uc_fw {
>>   uint16_t major_ver_found;
>>   uint16_t minor_ver_found;
>>
>> + uint32_t fw_type;
>>   uint32_t header_size;
>>   uint32_t header_offset;
>>   uint32_t rsa_size;
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h
>> b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> index 944786d..a69ee36 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> @@ -154,7 +154,7 @@
>>   * The GuC firmware layout looks like this:
>>   *
>>   *     +-------------------------------+
>> - *     |        guc_css_header         |
>> + *     |         uc_css_header         |
>>   *     | contains major/minor version  |
>>   *     +-------------------------------+
>>   *     |             uCode             |
>> @@ -180,9 +180,16 @@
>>   * 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.
>> + *
>> + * 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.
>>   */
>>
>> -struct guc_css_header {
>> +struct uc_css_header {
>>   uint32_t module_type;
>>   /* header_size includes all non-uCode bits, including
>> css_header, rsa
>>    * key, modulus key and exponent data. */
>> @@ -213,7 +220,10 @@ struct guc_css_header {
>>
>>   char username[8];
>>   char buildnumber[12];
>> - uint32_t device_id;
>> + union {
>> + uint32_t device_id;
>> + uint32_t huc_sw_version;
>> + };
>>   uint32_t guc_sw_version;
>>   uint32_t prod_preprod_fw;
>>   uint32_t reserved[12];
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index 1afa49b..42b6509 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -553,7 +553,7 @@ void intel_uc_fw_fetch(struct drm_device *dev,
>> struct intel_uc_fw *uc_fw)
>>  {
>>   struct drm_i915_gem_object *obj;
>>   const struct firmware *fw;
>> - struct guc_css_header *css;
>> + struct uc_css_header *css;
>>   size_t size;
>>   int err;
>>
>> @@ -570,19 +570,19 @@ void intel_uc_fw_fetch(struct drm_device *dev,
>> struct intel_uc_fw *uc_fw)
>>   uc_fw->uc_fw_path, fw);
>>
>>   /* Check the size of the blob before examining buffer
>> contents */
>> - if (fw->size < sizeof(struct guc_css_header)) {
>> + if (fw->size < sizeof(struct uc_css_header)) {
>>   DRM_ERROR("Firmware header is missing\n");
>>   goto fail;
>>   }
>>
>> - css = (struct guc_css_header *)fw->data;
>> + 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 guc_css_header)) {
>> + if (uc_fw->header_size != sizeof(struct uc_css_header)) {
>>   DRM_ERROR("CSS header definition mismatch\n");
>>   goto fail;
>>   }
>> @@ -606,21 +606,35 @@ void intel_uc_fw_fetch(struct drm_device *dev,
>> struct intel_uc_fw *uc_fw)
>>   goto fail;
>>   }
>>
>> - /* Header and uCode will be loaded to WOPCM. Size of the
>> two. */
>> - size = uc_fw->header_size + uc_fw->ucode_size;
>> - if (size > guc_wopcm_size(to_i915(dev))) {
>> - DRM_ERROR("Firmware is too large to fit in
>> WOPCM\n");
>> - 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).
>>    */
>> - uc_fw->major_ver_found = css->guc_sw_version >> 16;
>> - uc_fw->minor_ver_found = css->guc_sw_version & 0xFFFF;
>> + switch (uc_fw->fw_type) {
>> + case UC_FW_TYPE_GUC:
>> + /* Header and uCode will be loaded to WOPCM. Size of
>> the two. */
>> + size = uc_fw->header_size + uc_fw->ucode_size;
>> +
>> + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6
>> context). */
>> + if (size > guc_wopcm_size(to_i915(dev))) {
>> + DRM_ERROR("Firmware is too large to fit in
>> WOPCM\n");
>> + goto fail;
>> + }
>> +
>> + uc_fw->major_ver_found = css->guc_sw_version >> 16;
>> + uc_fw->minor_ver_found = css->guc_sw_version &
>> 0xFFFF;
>> + break;
>> + case UC_FW_TYPE_HUC:
>> + uc_fw->major_ver_found = css->huc_sw_version >> 16;
>> + uc_fw->minor_ver_found = css->huc_sw_version &
>> 0xFFFF;
>> + break;
>> + default:
>> + DRM_ERROR("Unknown firmware type %d\n", uc_fw-
>>> fw_type);
>> + err = -ENOEXEC;
>> + goto fail;
>> + }
>>
>>   if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
>>       uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {

--
Peter Antoine (Android Graphics Driver Software Engineer)
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]