On Thu, Oct 13, 2016 at 08:45:45AM -0700, Jeff McGee wrote:
> On Mon, Oct 03, 2016 at 11:42:56AM -0700, Anusha Srivatsa wrote:
> > From: Peter Antoine <peter.anto...@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.
> >
> > v2: rebased on-top of drm-intel-nightly
> > v3: rebased on-top of drm-intel-nightly (again).
> > v4: rebased + spaces.
> > v7: rebased.
> > v8: rebased.
> >
> > Tested-by: Xiang Haihao <haihao.xi...@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.sriva...@intel.com>
> > Signed-off-by: Alex Dai <yu....@intel.com>
> > Signed-off-by: Peter Antoine <peter.anto...@intel.com>
> > Reviewed-by: Dave Gordon <david.s.gor...@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 | 41
> > ++++++++++++++++++++++-----------
> > 3 files changed, 45 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_guc.h
> > b/drivers/gpu/drm/i915/intel_guc.h
> > index b134a41..812e4ca 100644
> > --- a/drivers/gpu/drm/i915/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/intel_guc.h
> > @@ -98,6 +98,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.
> > @@ -115,6 +118,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 e40db2d..b38b6b4 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 |
> > * +-------------------------------+
> > @@ -181,9 +181,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. */
> > @@ -214,7 +221,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];
>
> I propose renaming the device_id field in this union to
> 'guc_client_branch_version'. GuC uses this position to store a client
> version and branch version. I'm not sure where the 'device_id' term came
> from. We don't reference this currently but may need to in the future
> so might as well name it properly. At the very least we should probably
> make it guc_device_id, to help indicate that it applies to guc fw only.
>
> In that same vein, can we make guc_sw_version into a union as below to
> reinforce the difference to huc?
>
> union {
> uint32_t guc_sw_version;
> uint32_t huc_reserved;
> };
>
> Jeff
>
One correction. The branch version is considered the "major" and the
client version the "minor", so the better name for this field is
'guc_branch_client_version'.
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> > b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index 493295d..0b863a1 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -586,7 +586,7 @@ void intel_uc_fw_fetch(struct drm_device *dev, struct
> > intel_uc_fw *uc_fw)
> > struct pci_dev *pdev = dev->pdev;
> > 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;
> >
> > @@ -603,19 +603,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_NOTE("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_NOTE("CSS header definition mismatch\n");
> > goto fail;
> > }
> > @@ -639,21 +639,36 @@ 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_NOTE("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) {
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
