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

From: Jeff McGee <jeff.mcgee@intel.com>
To: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/8] drm/i915/huc: Unified css_header struct for GuC and HuC
Date: Thu, 13 Oct 2016 21:24:43 -0700	[thread overview]
Message-ID: <20161014042442.GL18646@jeffdesk> (raw)
In-Reply-To: <20161013154545.GD18646@jeffdesk>

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.antoine@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.xiang@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Alex Dai <yu.dai@intel.com>
> > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > Reviewed-by: Dave Gordon <david.s.gordon@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