| Message ID | 1466532685-5101-3-git-send-email-peter.antoine@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21/06/16 19:11, Peter Antoine wrote: > From: Alex Dai <yu.dai@intel.com> > > 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. > > 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 | 42 +++++++++++++++++++++------------ > 3 files changed, 44 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index 836420b..0b4ed88 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 > + > /* > * 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 { This name change could have been part of the previous global search-and-replace. > 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]; This can't really be right; the HuC f/w header doesn't contain a guc_sw_version! So the union must be more like: union { struct { uint32_t device_id; uint32_t guc_sw_version; uint32_t prod_preprod_fw; } guc_data; struct { uint32_t huc_sw_version; uint32_t prod_preprod_fw; uint32_t dummy; } huc_data; }; uint32_t reserved[12]; ... or something - I don't know what the actual layout is here. Otherwise generally OK. .Dave. > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index ff054f5..70575bd 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -574,7 +574,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; > > @@ -591,19 +591,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; > } > @@ -627,23 +627,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; > - > - /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ > - if (size > guc_wopcm_size(dev->dev_private)) { > - DRM_ERROR("Firmware is too large to fit in WOPCM\n"); > - goto fail; > - } > - > /* > * The uC 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(dev->dev_private)) { > + 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) { >
-----Original Message----- From: Gordon, David S Sent: Tuesday, June 28, 2016 3:33 PM To: Antoine, Peter <peter.antoine@intel.com>; intel-gfx@lists.freedesktop.org Cc: Prigent, Christophe <christophe.prigent@intel.com>; Kelley, Sean V <sean.v.kelley@intel.com>; Li, Lawrence T <lawrence.t.li@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Dai, Yu <yu.dai@intel.com> Subject: Re: [PATCH 2/6] drm/i915/huc: Unified css_header struct for GuC and HuC On 21/06/16 19:11, Peter Antoine wrote: > From: Alex Dai <yu.dai@intel.com> > > 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. > > 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 | 42 +++++++++++++++++++++------------ > 3 files changed, 44 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.h > b/drivers/gpu/drm/i915/intel_guc.h > index 836420b..0b4ed88 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 > + > /* > * 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 { This name change could have been part of the previous global search-and-replace. > 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]; This can't really be right; the HuC f/w header doesn't contain a guc_sw_version! So the union must be more like: union { struct { uint32_t device_id; uint32_t guc_sw_version; uint32_t prod_preprod_fw; } guc_data; struct { uint32_t huc_sw_version; uint32_t prod_preprod_fw; uint32_t dummy; } huc_data; }; uint32_t reserved[12]; ... or something - I don't know what the actual layout is here. Otherwise generally OK. .Dave. Not really, It's not wrong. It just could be clearer. But adding other structs just muddies the water. I think Alex's code is easier to read (and correct). Guc_sw_version is simply not used on the huc it is a bad design by the firmware designers not to use a fix format for all fw but just make the word we need a union is simplest. Probably could change it to: Union { Struct { Uint32_t device_id; Unit32_t guc_sw_version; }; Struct { Uint32_t huc_sw_version; Uint32_t filler; }; } Just looking at the code. But I am going to leave it alone unless there is real objection to it. Peter. > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > b/drivers/gpu/drm/i915/intel_guc_loader.c > index ff054f5..70575bd 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -574,7 +574,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; > > @@ -591,19 +591,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; > } > @@ -627,23 +627,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; > - > - /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ > - if (size > guc_wopcm_size(dev->dev_private)) { > - DRM_ERROR("Firmware is too large to fit in WOPCM\n"); > - goto fail; > - } > - > /* > * The uC 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(dev->dev_private)) { > + 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) { >
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 836420b..0b4ed88 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 + /* * 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 ff054f5..70575bd 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -574,7 +574,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; @@ -591,19 +591,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; } @@ -627,23 +627,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; - - /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ - if (size > guc_wopcm_size(dev->dev_private)) { - DRM_ERROR("Firmware is too large to fit in WOPCM\n"); - goto fail; - } - /* * The uC 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(dev->dev_private)) { + 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) {