[Intel-gfx] [PATCH 2/8] drm/i915/huc: Unified css_header struct for GuC and HuC
Srivatsa, Anusha
anusha.srivatsa at intel.com
Fri Dec 9 21:42:06 UTC 2016
>-----Original Message-----
>From: Michal Wajdeczko [mailto:michal.wajdeczko at linux.intel.com]
>Sent: Friday, December 9, 2016 3:56 AM
>To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Alex Dai <yu.dai at intel.com>; Peter Antoine
><peter.antoine at intel.com>
>Subject: Re: [Intel-gfx] [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 at 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.
>> 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 at intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> Signed-off-by: Alex Dai <yu.dai at intel.com>
>> Signed-off-by: Peter Antoine <peter.antoine at intel.com>
>> ---
>> 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?
>> uint32_t module_type;
>
>What values are used here? Are they the same as used in fw_type?
>
>
>> /* 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list