On Fri, Dec 09, 2016 at 09:42:06PM +0000, Srivatsa, Anusha wrote: > > > >-----Original Message----- > >From: Michal Wajdeczko [mailto:michal.wajdeczko@xxxxxxxxxxxxxxx] > >Sent: Friday, December 9, 2016 3:56 AM > >To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx> > >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Alex Dai <yu.dai@xxxxxxxxx>; Peter Antoine > ><peter.antoine@xxxxxxxxx> > >Subject: Re: [PATCH 2/8] drm/i915/huc: Unified css_header struct for > >GuC and HuC > > > >On Thu, Dec 08, 2016 at 03:02:13PM -0800, anushasr wrote: > >> From: Peter Antoine <peter.antoine@xxxxxxxxx> > >> > >> 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. > >> v9: rebased. Rename device_id to guc_branch_client_version, make > >> guc_sw_version a union. <Jeff Mcgee>. Put UC_FW_TYPE_GUC and > >> UC_FW_TYPE_HUC into an enum. > >> v10: rebased. > >> v11: rebased. > >> v12: rebased on top of drm-tip. > >> v13: rebased.Update dev to dev_priv in intel_uc_fw_fetch > >> > >> Tested-by: Xiang Haihao <haihao.xiang@xxxxxxxxx> > >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> > >> Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx> > >> Signed-off-by: Peter Antoine <peter.antoine@xxxxxxxxx> > >> --- > >> drivers/gpu/drm/i915/intel_guc_fwif.h | 21 +++++++++++++---- > >> drivers/gpu/drm/i915/intel_guc_loader.c | 41 ++++++++++++++++++++++------- > >---- > >> drivers/gpu/drm/i915/intel_uc.h | 5 ++++ > >> 3 files changed, 50 insertions(+), 17 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h > >> b/drivers/gpu/drm/i915/intel_guc_fwif.h > >> index 3202b32..c1e7faf 100644 > >> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > >> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > >> @@ -145,7 +145,7 @@ > >> * The GuC firmware layout looks like this: > >> * > >> * +-------------------------------+ > >> - * | guc_css_header | > >> + * | uc_css_header | > >> * | | > >> * | contains major/minor version | > >> * +-------------------------------+ > >> @@ -172,9 +172,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 { > > > >Hmm, I'm wondering why we don't use "intel_uc_" prefix for structs defined here. > >It seems that only enums are defined with intel_ prefix. > If we rename it to intel_uc_css_header, wont it be confused with intel_css_header in intel_csr.c? Unles we want to combine them.... > > >Also, it looks that this struct definition is very similar to the intel_css_header > >defined in intel_csr.c. Are there any plans to unify them all ? > > > No idea about any plans regarding unifying the struct in intel_csr.c and this struct. Arek, any idea? Currently not, but that seems like idea worth exploring. I'll look into it this week and if it turns sensible to do, I'll add it to my GuC cleanup series. > >> uint32_t module_type; > > > >What values are used here? Are they the same as used in fw_type? module_type is just a part of HuC/GuC's firmware header (css). You can read it from the blobs: module_type for guc == 0006 0000 00a1 0000 (v6, v4) module_type for huc == 0006 0000 00a1 0000 (v1) It's kind of magic value to determine it's a firmware (I am not aware of other values than the one above, but that might change in the future). We do not check for that exact field though. We do not use it at all, but that, as well, might change in the future. fw_type provides means to distingush between HuC/GuC FW. > > > >> /* header_size includes all non-uCode bits, including css_header, rsa > >> * key, modulus key and exponent data. */ @@ -205,8 +212,14 @@ > >> struct guc_css_header { > >> > >> char username[8]; > >> char buildnumber[12]; > >> - uint32_t device_id; > >> - uint32_t guc_sw_version; > >> + union { > >> + uint32_t guc_branch_client_version; > >> + uint32_t huc_sw_version; > >> + }; > >> + union { > >> + uint32_t guc_sw_version; > >> + uint32_t huc_reserved; > >> + }; > > > >Maybe to make this a little easier to read we can use: > > > >union { > > struct { > > uint32_t branch_client_version; > > uint32_t sw_version; > > } guc; > > struct { > > uint32_t sw_version; > > unit32_t reserved; > > } huc; > >}; > Yes. Will do. > > > >> uint32_t prod_preprod_fw; > >> uint32_t reserved[12]; > >> uint32_t header_info; > >> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > >> b/drivers/gpu/drm/i915/intel_guc_loader.c > >> index 8f04f6e..26a184f 100644 > >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c > >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > >> @@ -593,7 +593,7 @@ void intel_uc_fw_fetch(struct drm_i915_private > >*dev_priv, > >> struct pci_dev *pdev = dev_priv->drm.pdev; > >> struct drm_i915_gem_object *obj; > >> const struct firmware *fw = NULL; > >> - struct guc_css_header *css; > >> + struct uc_css_header *css; > >> size_t size; > >> int err; > >> > >> @@ -610,19 +610,19 @@ void intel_uc_fw_fetch(struct drm_i915_private > >*dev_priv, > >> 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; > >> } > >> @@ -646,21 +646,36 @@ void intel_uc_fw_fetch(struct drm_i915_private > >*dev_priv, > >> 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(dev_priv)) { > >> - 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(dev_priv)) { > >> + 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_uc.h b/drivers/gpu/drm/i915/intel_uc.h > >> index f9f598d..be89f0b 100644 > >> --- a/drivers/gpu/drm/i915/intel_uc.h > >> +++ b/drivers/gpu/drm/i915/intel_uc.h > >> @@ -98,6 +98,11 @@ enum intel_uc_fw_status { > >> UC_FIRMWARE_SUCCESS > >> }; > >> > >> +enum { > >> + UC_FW_TYPE_GUC, > >> + UC_FW_TYPE_HUC > > > >Can we have INTEL_ prefix here? > > > >> +}; > >> Yes. Thanks Michal. > > > Anusha > >> /* > >> * This structure encapsulates all the data needed during the process > >> * of fetching, caching, and loading the firmware image into the GuC. > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Cheers, Arek _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
