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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: Hiler, Arkadiusz
>Sent: Thursday, December 1, 2016 4:23 AM
>To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Mcgee, Jeff <jeff.mcgee@xxxxxxxxx>;
>Kamble, Sagar A <sagar.a.kamble@xxxxxxxxx>
>Subject: Re:  [PATCH 2/8] drm/i915/huc: Unified css_header struct for
>GuC and HuC
>
>On Wed, Nov 30, 2016 at 03:31:28PM -0800, Anusha Srivatsa 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.
>>
>> 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>
>> Reviewed-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
>> Reviewed-by: Jeff McGee <jeff.mcgee@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 00ca0df..c07d9da 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 {
>
>I think we should either move most of this stuff to intel_uc.{c,h} or rename the
>file to intel_uc_fwif.h.
>
>Anyway, this file contains information on top that this is automatically generated
>and your changes might be lost...
>
>The file was introduced and then *manually edited* *multiple times* by *many
>people*, without any signs of changes lost or file being regenerated.
>
>I think we can, at least, drop the warning. I am in favor of drooping the file
>completely though.
So, basically move all these struct and union declarations to intel_uc.h?
>If something would change, we can assume it would be done by hand anyway.
>
>Jeff, Sagar: any thought on that?
>
>>  	uint32_t module_type;
>>  	/* header_size includes all non-uCode bits, including css_header, rsa
>>  	 * key, modulus key and exponent data. */ @@ -214,8 +221,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;
>> +	};
>>  	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 e55ec2c..557d4b4 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_device *dev, struct
>intel_uc_fw *uc_fw)
>>  	struct pci_dev *pdev = dev->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_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;
>>  	}
>> @@ -646,21 +646,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) { diff --git
>> a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
>> index a1c771f..1616cac 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
>> +};
>> +
>>  /*
>>   * 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
>
>--
>Cheers,
>Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux