[1/8] drm/i915/guc: Make the GuC fw loading helper functions general

Commit Message

Srivatsa, Anusha Dec. 8, 2016, 11:02 p.m. UTC

From: Peter Antoine <[e-mail]>

Rename some of the GuC fw loading code to make them more general. We
will utilise them for HuC loading as well.
     s/intel_guc_fw/intel_uc_fw/g
     s/GUC_FIRMWARE/UC_FIRMWARE/g

Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members,
such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for
same purpose.

v2: rebased on top of nightly.
    reapplied the search/replace as upstream code as changed.
v3: rebased again on drm-nightly.
v4: removed G from messages in shared fw fetch function.
v5: rebased.
v7: rebased.
v8: rebased.
v9: rebased.
v10: rebased.
v11: rebased.
v12: rebased on top of drm-tip
v13: rebased.Updated dev to dev_priv in intel_guc_setup(), guc_fw_getch()
and intel_guc_init().

Signed-off-by: Anusha Srivatsa <[e-mail]>
Signed-off-by: Alex Dai <[e-mail]>
Signed-off-by: Peter Antoine <[e-mail]>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  12 +--
 drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    | 157 +++++++++++++++--------------
 drivers/gpu/drm/i915/intel_uc.h            |  40 ++++----
 4 files changed, 108 insertions(+), 105 deletions(-)

Comments

Arkadiusz Hiler Dec. 9, 2016, 8:58 a.m. UTC | #1

On Thu, Dec 08, 2016 at 03:02:12PM -0800, anushasr wrote:
> From: Peter Antoine <[e-mail]>
> 
> Rename some of the GuC fw loading code to make them more general. We
> will utilise them for HuC loading as well.
>      s/intel_guc_fw/intel_uc_fw/g
>      s/GUC_FIRMWARE/UC_FIRMWARE/g
> 
> Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members,
> such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for
> same purpose.
> 
> v2: rebased on top of nightly.
>     reapplied the search/replace as upstream code as changed.
> v3: rebased again on drm-nightly.
> v4: removed G from messages in shared fw fetch function.
> v5: rebased.
> v7: rebased.
> v8: rebased.
> v9: rebased.
> v10: rebased.
> v11: rebased.
> v12: rebased on top of drm-tip
> v13: rebased.Updated dev to dev_priv in intel_guc_setup(), guc_fw_getch()
> and intel_guc_init().

This is little bit misleading. It suggests that you did the change from
dev to dev_priv, while it really happened externally. You just had to
rebase and accomodate for that.

nontheless...

> Signed-off-by: Anusha Srivatsa <[e-mail]>
> Signed-off-by: Alex Dai <[e-mail]>
> Signed-off-by: Peter Antoine <[e-mail]>
Reviewed-by: Arkadiusz Hiler <[e-mail]>

Michal Wajdeczko Dec. 9, 2016, 11:28 a.m. UTC | #2

On Thu, Dec 08, 2016 at 03:02:12PM -0800, anushasr wrote:
> From: Peter Antoine <[e-mail]>
> 
> Rename some of the GuC fw loading code to make them more general. We
> will utilise them for HuC loading as well.
>      s/intel_guc_fw/intel_uc_fw/g
>      s/GUC_FIRMWARE/UC_FIRMWARE/g
> 
> Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members,
> such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for
> same purpose.
> 
> v2: rebased on top of nightly.
>     reapplied the search/replace as upstream code as changed.
> v3: rebased again on drm-nightly.
> v4: removed G from messages in shared fw fetch function.
> v5: rebased.
> v7: rebased.
> v8: rebased.
> v9: rebased.
> v10: rebased.
> v11: rebased.
> v12: rebased on top of drm-tip
> v13: rebased.Updated dev to dev_priv in intel_guc_setup(), guc_fw_getch()
> and intel_guc_init().
> 
> Signed-off-by: Anusha Srivatsa <[e-mail]>
> Signed-off-by: Alex Dai <[e-mail]>
> Signed-off-by: Peter Antoine <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |  12 +--
>  drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c    | 157 +++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_uc.h            |  40 ++++----
>  4 files changed, 108 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a746130..0e5ef62 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2338,7 +2338,7 @@ static int i915_llc(struct seq_file *m, void *data)
>  static int i915_guc_load_status_info(struct seq_file *m, void *data)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	u32 tmp, i;
>  
>  	if (!HAS_GUC_UCODE(dev_priv))
> @@ -2346,15 +2346,15 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
>  
>  	seq_printf(m, "GuC firmware status:\n");
>  	seq_printf(m, "\tpath: %s\n",
> -		guc_fw->guc_fw_path);
> +		guc_fw->uc_fw_path);
>  	seq_printf(m, "\tfetch: %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> +		intel_uc_fw_status_repr(guc_fw->fetch_status));
>  	seq_printf(m, "\tload: %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> +		intel_uc_fw_status_repr(guc_fw->load_status));
>  	seq_printf(m, "\tversion wanted: %d.%d\n",
> -		guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> +		guc_fw->major_ver_wanted, guc_fw->minor_ver_wanted);
>  	seq_printf(m, "\tversion found: %d.%d\n",
> -		guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found);
> +		guc_fw->major_ver_found, guc_fw->minor_ver_found);
>  	seq_printf(m, "\theader: offset is %d; size = %d\n",
>  		guc_fw->header_offset, guc_fw->header_size);
>  	seq_printf(m, "\tuCode: offset is %d; size = %d\n",
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 7fa4e74..e156a4b 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1493,7 +1493,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
>  	struct i915_gem_context *ctx;
>  	u32 data[3];
>  
> -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
> +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
>  		return 0;
>  
>  	gen9_disable_guc_interrupts(dev_priv);
> @@ -1520,7 +1520,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
>  	struct i915_gem_context *ctx;
>  	u32 data[3];
>  
> -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
> +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
>  		return 0;
>  
>  	if (i915.guc_log_level >= 0)
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 21db697..8f04f6e 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -81,16 +81,16 @@ MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
>  MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
>  
>  /* User-friendly representation of an enum */
> -const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
> +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
>  {
>  	switch (status) {
> -	case GUC_FIRMWARE_FAIL:
> +	case UC_FIRMWARE_FAIL:
>  		return "FAIL";
> -	case GUC_FIRMWARE_NONE:
> +	case UC_FIRMWARE_NONE:
>  		return "NONE";
> -	case GUC_FIRMWARE_PENDING:
> +	case UC_FIRMWARE_PENDING:
>  		return "PENDING";
> -	case GUC_FIRMWARE_SUCCESS:
> +	case UC_FIRMWARE_SUCCESS:
>  		return "SUCCESS";
>  	default:
>  		return "UNKNOWN!";
> @@ -278,7 +278,7 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
>  static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  			      struct i915_vma *vma)
>  {
> -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	unsigned long offset;
>  	struct sg_table *sg = vma->pages;
>  	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
> @@ -350,17 +350,17 @@ static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
>   */
>  static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  {
> -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	struct i915_vma *vma;
>  	int ret;
>  
> -	ret = i915_gem_object_set_to_gtt_domain(guc_fw->guc_fw_obj, false);
> +	ret = i915_gem_object_set_to_gtt_domain(guc_fw->uc_fw_obj, false);
>  	if (ret) {
>  		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
>  		return ret;
>  	}
>  
> -	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);
> +	vma = i915_gem_object_ggtt_pin(guc_fw->uc_fw_obj, NULL, 0, 0, 0);
>  	if (IS_ERR(vma)) {
>  		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
>  		return PTR_ERR(vma);
> @@ -450,14 +450,14 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
>   */
>  int intel_guc_setup(struct drm_i915_private *dev_priv)
>  {
> -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> -	const char *fw_path = guc_fw->guc_fw_path;
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	const char *fw_path = guc_fw->uc_fw_path;
>  	int retries, ret, err;
>  
>  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
>  		fw_path,
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> +		intel_uc_fw_status_repr(guc_fw->load_status));
>  
>  	/* Loading forbidden, or no firmware to load? */
>  	if (!i915.enable_guc_loading) {
> @@ -475,10 +475,10 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  	}
>  
>  	/* Fetch failed, or already fetched but failed to load? */
> -	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> +	if (guc_fw->fetch_status != UC_FIRMWARE_SUCCESS) {
>  		err = -EIO;
>  		goto fail;
> -	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> +	} else if (guc_fw->load_status == UC_FIRMWARE_FAIL) {
>  		err = -ENOEXEC;
>  		goto fail;
>  	}
> @@ -486,11 +486,11 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  	guc_interrupts_release(dev_priv);
>  	gen9_reset_guc_interrupts(dev_priv);
>  
> -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> +	guc_fw->load_status = UC_FIRMWARE_PENDING;
>  
>  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> +		intel_uc_fw_status_repr(guc_fw->load_status));
>  
>  	err = i915_guc_submission_init(dev_priv);
>  	if (err)
> @@ -522,11 +522,11 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  			 "retry %d more time(s)\n", err, retries);
>  	}
>  
> -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
> +	guc_fw->load_status = UC_FIRMWARE_SUCCESS;
>  
>  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> +		intel_uc_fw_status_repr(guc_fw->load_status));
>  
>  	if (i915.enable_guc_submission) {
>  		if (i915.guc_log_level >= 0)
> @@ -541,8 +541,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  	return 0;
>  
>  fail:
> -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
> +	if (guc_fw->load_status == UC_FIRMWARE_PENDING)
> +		guc_fw->load_status = UC_FIRMWARE_FAIL;
>  
>  	guc_interrupts_release(dev_priv);
>  	i915_guc_submission_disable(dev_priv);
> @@ -587,8 +587,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> -			 struct intel_guc_fw *guc_fw)
> +void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> +			 struct intel_uc_fw *uc_fw)
>  {
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	struct drm_i915_gem_object *obj;
> @@ -597,17 +597,17 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
>  	size_t size;
>  	int err;
>  
> -	DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> +	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
> +		intel_uc_fw_status_repr(uc_fw->fetch_status));
>  
> -	err = request_firmware(&fw, guc_fw->guc_fw_path, &pdev->dev);
> +	err = request_firmware(&fw, uc_fw->uc_fw_path, &pdev->dev);
>  	if (err)
>  		goto fail;
>  	if (!fw)
>  		goto fail;
>  
> -	DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n",
> -		guc_fw->guc_fw_path, fw);
> +	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
> +		uc_fw->uc_fw_path, fw);
>  
>  	/* Check the size of the blob before examining buffer contents */
>  	if (fw->size < sizeof(struct guc_css_header)) {
> @@ -618,36 +618,36 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
>  	css = (struct guc_css_header *)fw->data;
>  
>  	/* Firmware bits always start from header */
> -	guc_fw->header_offset = 0;
> -	guc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
> +	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 (guc_fw->header_size != sizeof(struct guc_css_header)) {
> +	if (uc_fw->header_size != sizeof(struct guc_css_header)) {
>  		DRM_NOTE("CSS header definition mismatch\n");
>  		goto fail;
>  	}
>  
>  	/* then, uCode */
> -	guc_fw->ucode_offset = guc_fw->header_offset + guc_fw->header_size;
> -	guc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
> +	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
> +	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
>  
>  	/* now RSA */
>  	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
>  		DRM_NOTE("RSA key size is bad\n");
>  		goto fail;
>  	}
> -	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
> -	guc_fw->rsa_size = css->key_size_dw * sizeof(u32);
> +	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
> +	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
>  
>  	/* At least, it should have header, uCode and RSA. Size of all three. */
> -	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
> +	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
>  	if (fw->size < size) {
>  		DRM_NOTE("Missing firmware components\n");
>  		goto fail;
>  	}
>  
>  	/* Header and uCode will be loaded to WOPCM. Size of the two. */
> -	size = guc_fw->header_size + guc_fw->ucode_size;
> +	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;
> @@ -659,21 +659,21 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
>  	 * TWO bytes each (i.e. u16), although all pointers and offsets are defined
>  	 * in terms of bytes (u8).
>  	 */
> -	guc_fw->guc_fw_major_found = css->guc_sw_version >> 16;
> -	guc_fw->guc_fw_minor_found = css->guc_sw_version & 0xFFFF;
> -
> -	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
> -	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
> -		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
> -			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> -			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> +	uc_fw->major_ver_found = css->guc_sw_version >> 16;
> +	uc_fw->minor_ver_found = css->guc_sw_version & 0xFFFF;
> +
> +	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> +	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> +		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
> +			uc_fw->major_ver_found, uc_fw->minor_ver_found,
> +			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
>  		err = -ENOEXEC;
>  		goto fail;
>  	}
>  
>  	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
> -			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> -			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> +			uc_fw->major_ver_found, uc_fw->minor_ver_found,
> +			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
>  	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
> @@ -683,31 +683,31 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
>  		goto fail;
>  	}
>  
> -	guc_fw->guc_fw_obj = obj;
> -	guc_fw->guc_fw_size = fw->size;
> +	uc_fw->uc_fw_obj = obj;
> +	uc_fw->uc_fw_size = fw->size;
>  
> -	DRM_DEBUG_DRIVER("GuC fw fetch status SUCCESS, obj %p\n",
> -			guc_fw->guc_fw_obj);
> +	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
> +			uc_fw->uc_fw_obj);
>  
>  	release_firmware(fw);
> -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_SUCCESS;
> +	uc_fw->fetch_status = UC_FIRMWARE_SUCCESS;
>  	return;
>  
>  fail:
> -	DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",
> -		 guc_fw->guc_fw_path, err);
> -	DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
> -		err, fw, guc_fw->guc_fw_obj);
> +	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
> +		 uc_fw->uc_fw_path, err);
> +	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
> +		err, fw, uc_fw->uc_fw_obj);
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
> -	obj = guc_fw->guc_fw_obj;
> +	obj = uc_fw->uc_fw_obj;
>  	if (obj)
>  		i915_gem_object_put(obj);
> -	guc_fw->guc_fw_obj = NULL;
> +	uc_fw->uc_fw_obj = NULL;
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  
>  	release_firmware(fw);		/* OK even if fw is NULL */
> -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
> +	uc_fw->fetch_status = UC_FIRMWARE_FAIL;
>  }
>  
>  /**
> @@ -721,7 +721,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
>   */
>  void intel_guc_init(struct drm_i915_private *dev_priv)
>  {
> -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	const char *fw_path;
>  
>  	if (!HAS_GUC(dev_priv)) {
> @@ -739,23 +739,24 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  		fw_path = NULL;
>  	} else if (IS_SKYLAKE(dev_priv)) {
>  		fw_path = I915_SKL_GUC_UCODE;
> -		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
> -		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
> +		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
> +		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
>  	} else if (IS_BROXTON(dev_priv)) {
>  		fw_path = I915_BXT_GUC_UCODE;
> -		guc_fw->guc_fw_major_wanted = BXT_FW_MAJOR;
> -		guc_fw->guc_fw_minor_wanted = BXT_FW_MINOR;
> +		guc_fw->major_ver_wanted = BXT_FW_MAJOR;
> +		guc_fw->minor_ver_wanted = BXT_FW_MINOR;
>  	} else if (IS_KABYLAKE(dev_priv)) {
>  		fw_path = I915_KBL_GUC_UCODE;
> -		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
> -		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
> +		guc_fw->major_ver_wanted = KBL_FW_MAJOR;
> +		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
>  	} else {
>  		fw_path = "";	/* unknown device */
>  	}
>  
> -	guc_fw->guc_fw_path = fw_path;
> -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
> +	guc_fw->uc_dev = &dev_priv->drm;
> +	guc_fw->uc_fw_path = fw_path;
> +	guc_fw->fetch_status = UC_FIRMWARE_NONE;
> +	guc_fw->load_status = UC_FIRMWARE_NONE;
>  
>  	/* Early (and silent) return if GuC loading is disabled */
>  	if (!i915.enable_guc_loading)
> @@ -765,9 +766,9 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	if (*fw_path == '\0')
>  		return;
>  
> -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
> +	guc_fw->fetch_status = UC_FIRMWARE_PENDING;
>  	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
> -	guc_fw_fetch(dev_priv, guc_fw);
> +	intel_uc_fw_fetch(dev_priv, guc_fw);
>  	/* status must now be FAIL or SUCCESS */
>  }
>  
> @@ -777,17 +778,17 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>   */
>  void intel_guc_fini(struct drm_i915_private *dev_priv)
>  {
> -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
>  	guc_interrupts_release(dev_priv);
>  	i915_guc_submission_disable(dev_priv);
>  	i915_guc_submission_fini(dev_priv);
>  
> -	if (guc_fw->guc_fw_obj)
> -		i915_gem_object_put(guc_fw->guc_fw_obj);
> -	guc_fw->guc_fw_obj = NULL;
> +	if (guc_fw->uc_fw_obj)
> +		i915_gem_object_put(guc_fw->uc_fw_obj);
> +	guc_fw->uc_fw_obj = NULL;
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  
> -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> +	guc_fw->fetch_status = UC_FIRMWARE_NONE;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 11f5608..f9f598d 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -91,29 +91,31 @@ struct i915_guc_client {
>  	uint64_t submissions[I915_NUM_ENGINES];
>  };
>  
> -enum intel_guc_fw_status {
> -	GUC_FIRMWARE_FAIL = -1,
> -	GUC_FIRMWARE_NONE = 0,
> -	GUC_FIRMWARE_PENDING,
> -	GUC_FIRMWARE_SUCCESS
> +enum intel_uc_fw_status {
> +	UC_FIRMWARE_FAIL = -1,
> +	UC_FIRMWARE_NONE = 0,
> +	UC_FIRMWARE_PENDING,
> +	UC_FIRMWARE_SUCCESS

Shouldn't we use INTEL_UC_ prefix here (in similar way as in GUC_ACTION)?


>  };
>  
>  /*
>   * This structure encapsulates all the data needed during the process
>   * of fetching, caching, and loading the firmware image into the GuC.
>   */
> -struct intel_guc_fw {
> -	const char *			guc_fw_path;
> -	size_t				guc_fw_size;
> -	struct drm_i915_gem_object *	guc_fw_obj;
> -	enum intel_guc_fw_status	guc_fw_fetch_status;
> -	enum intel_guc_fw_status	guc_fw_load_status;
> -
> -	uint16_t			guc_fw_major_wanted;
> -	uint16_t			guc_fw_minor_wanted;
> -	uint16_t			guc_fw_major_found;
> -	uint16_t			guc_fw_minor_found;
> -
> +struct intel_uc_fw {
> +	struct drm_device *uc_dev;

Why do you need this back pointer to drm_dev ?
More likely all you need is pointer to dev_priv.
But for dev_priv you can always use guc_to_i915().


> +	const char *uc_fw_path;
> +	size_t uc_fw_size;
> +	struct drm_i915_gem_object *uc_fw_obj;

All these uc_fw_ prefixes seem to be redundant as this struct is just about uc_fw.


> +	enum intel_uc_fw_status fetch_status;
> +	enum intel_uc_fw_status load_status;
> +
> +	uint16_t major_ver_wanted;
> +	uint16_t minor_ver_wanted;
> +	uint16_t major_ver_found;
> +	uint16_t minor_ver_found;
> +
> +	uint32_t fw_type;

What is the purpose of this field ?


>  	uint32_t header_size;
>  	uint32_t header_offset;
>  	uint32_t rsa_size;
> @@ -139,7 +141,7 @@ struct intel_guc_log {
>  };
>  
>  struct intel_guc {
> -	struct intel_guc_fw guc_fw;
> +	struct intel_uc_fw guc_fw;

Btw, guc_ prefix in field name here is also redundant.


>  	struct intel_guc_log log;
>  
>  	/* intel_guc_recv interrupt related state */
> @@ -181,7 +183,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val);
>  extern void intel_guc_init(struct drm_i915_private *dev_priv);
>  extern int intel_guc_setup(struct drm_i915_private *dev_priv);
>  extern void intel_guc_fini(struct drm_i915_private *dev_priv);
> -extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
> +extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
>  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Arkadiusz Hiler Dec. 9, 2016, 11:49 a.m. UTC | #3

On Fri, Dec 09, 2016 at 12:28:52PM +0100, Michal Wajdeczko wrote:
> On Thu, Dec 08, 2016 at 03:02:12PM -0800, anushasr wrote:
> > From: Peter Antoine <[e-mail]>
> > 
> > Rename some of the GuC fw loading code to make them more general. We
> > will utilise them for HuC loading as well.
> >      s/intel_guc_fw/intel_uc_fw/g
> >      s/GUC_FIRMWARE/UC_FIRMWARE/g
> > 
> > Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members,
> > such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for
> > same purpose.
> > 
> > v2: rebased on top of nightly.
> >     reapplied the search/replace as upstream code as changed.
> > v3: rebased again on drm-nightly.
> > v4: removed G from messages in shared fw fetch function.
> > v5: rebased.
> > v7: rebased.
> > v8: rebased.
> > v9: rebased.
> > v10: rebased.
> > v11: rebased.
> > v12: rebased on top of drm-tip
> > v13: rebased.Updated dev to dev_priv in intel_guc_setup(), guc_fw_getch()
> > and intel_guc_init().
> > 
> > Signed-off-by: Anusha Srivatsa <[e-mail]>
> > Signed-off-by: Alex Dai <[e-mail]>
> > Signed-off-by: Peter Antoine <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c        |  12 +--
> >  drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
> >  drivers/gpu/drm/i915/intel_guc_loader.c    | 157 +++++++++++++++--------------
> >  drivers/gpu/drm/i915/intel_uc.h            |  40 ++++----
> >  4 files changed, 108 insertions(+), 105 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index a746130..0e5ef62 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2338,7 +2338,7 @@ static int i915_llc(struct seq_file *m, void *data)
> >  static int i915_guc_load_status_info(struct seq_file *m, void *data)
> >  {
> >  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  	u32 tmp, i;
> >  
> >  	if (!HAS_GUC_UCODE(dev_priv))
> > @@ -2346,15 +2346,15 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
> >  
> >  	seq_printf(m, "GuC firmware status:\n");
> >  	seq_printf(m, "\tpath: %s\n",
> > -		guc_fw->guc_fw_path);
> > +		guc_fw->uc_fw_path);
> >  	seq_printf(m, "\tfetch: %s\n",
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> > +		intel_uc_fw_status_repr(guc_fw->fetch_status));
> >  	seq_printf(m, "\tload: %s\n",
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > +		intel_uc_fw_status_repr(guc_fw->load_status));
> >  	seq_printf(m, "\tversion wanted: %d.%d\n",
> > -		guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> > +		guc_fw->major_ver_wanted, guc_fw->minor_ver_wanted);
> >  	seq_printf(m, "\tversion found: %d.%d\n",
> > -		guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found);
> > +		guc_fw->major_ver_found, guc_fw->minor_ver_found);
> >  	seq_printf(m, "\theader: offset is %d; size = %d\n",
> >  		guc_fw->header_offset, guc_fw->header_size);
> >  	seq_printf(m, "\tuCode: offset is %d; size = %d\n",
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 7fa4e74..e156a4b 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -1493,7 +1493,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
> >  	struct i915_gem_context *ctx;
> >  	u32 data[3];
> >  
> > -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
> > +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
> >  		return 0;
> >  
> >  	gen9_disable_guc_interrupts(dev_priv);
> > @@ -1520,7 +1520,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
> >  	struct i915_gem_context *ctx;
> >  	u32 data[3];
> >  
> > -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
> > +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
> >  		return 0;
> >  
> >  	if (i915.guc_log_level >= 0)
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index 21db697..8f04f6e 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -81,16 +81,16 @@ MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
> >  MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
> >  
> >  /* User-friendly representation of an enum */
> > -const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
> > +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
> >  {
> >  	switch (status) {
> > -	case GUC_FIRMWARE_FAIL:
> > +	case UC_FIRMWARE_FAIL:
> >  		return "FAIL";
> > -	case GUC_FIRMWARE_NONE:
> > +	case UC_FIRMWARE_NONE:
> >  		return "NONE";
> > -	case GUC_FIRMWARE_PENDING:
> > +	case UC_FIRMWARE_PENDING:
> >  		return "PENDING";
> > -	case GUC_FIRMWARE_SUCCESS:
> > +	case UC_FIRMWARE_SUCCESS:
> >  		return "SUCCESS";
> >  	default:
> >  		return "UNKNOWN!";
> > @@ -278,7 +278,7 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
> >  static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
> >  			      struct i915_vma *vma)
> >  {
> > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  	unsigned long offset;
> >  	struct sg_table *sg = vma->pages;
> >  	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
> > @@ -350,17 +350,17 @@ static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> >   */
> >  static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
> >  {
> > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  	struct i915_vma *vma;
> >  	int ret;
> >  
> > -	ret = i915_gem_object_set_to_gtt_domain(guc_fw->guc_fw_obj, false);
> > +	ret = i915_gem_object_set_to_gtt_domain(guc_fw->uc_fw_obj, false);
> >  	if (ret) {
> >  		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> >  		return ret;
> >  	}
> >  
> > -	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);
> > +	vma = i915_gem_object_ggtt_pin(guc_fw->uc_fw_obj, NULL, 0, 0, 0);
> >  	if (IS_ERR(vma)) {
> >  		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> >  		return PTR_ERR(vma);
> > @@ -450,14 +450,14 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
> >   */
> >  int intel_guc_setup(struct drm_i915_private *dev_priv)
> >  {
> > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > -	const char *fw_path = guc_fw->guc_fw_path;
> > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > +	const char *fw_path = guc_fw->uc_fw_path;
> >  	int retries, ret, err;
> >  
> >  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> >  		fw_path,
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> > +		intel_uc_fw_status_repr(guc_fw->load_status));
> >  
> >  	/* Loading forbidden, or no firmware to load? */
> >  	if (!i915.enable_guc_loading) {
> > @@ -475,10 +475,10 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> >  	}
> >  
> >  	/* Fetch failed, or already fetched but failed to load? */
> > -	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> > +	if (guc_fw->fetch_status != UC_FIRMWARE_SUCCESS) {
> >  		err = -EIO;
> >  		goto fail;
> > -	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> > +	} else if (guc_fw->load_status == UC_FIRMWARE_FAIL) {
> >  		err = -ENOEXEC;
> >  		goto fail;
> >  	}
> > @@ -486,11 +486,11 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> >  	guc_interrupts_release(dev_priv);
> >  	gen9_reset_guc_interrupts(dev_priv);
> >  
> > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> > +	guc_fw->load_status = UC_FIRMWARE_PENDING;
> >  
> >  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> > +		intel_uc_fw_status_repr(guc_fw->load_status));
> >  
> >  	err = i915_guc_submission_init(dev_priv);
> >  	if (err)
> > @@ -522,11 +522,11 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> >  			 "retry %d more time(s)\n", err, retries);
> >  	}
> >  
> > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
> > +	guc_fw->load_status = UC_FIRMWARE_SUCCESS;
> >  
> >  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> > +		intel_uc_fw_status_repr(guc_fw->load_status));
> >  
> >  	if (i915.enable_guc_submission) {
> >  		if (i915.guc_log_level >= 0)
> > @@ -541,8 +541,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> >  	return 0;
> >  
> >  fail:
> > -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> > -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
> > +	if (guc_fw->load_status == UC_FIRMWARE_PENDING)
> > +		guc_fw->load_status = UC_FIRMWARE_FAIL;
> >  
> >  	guc_interrupts_release(dev_priv);
> >  	i915_guc_submission_disable(dev_priv);
> > @@ -587,8 +587,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> >  	return ret;
> >  }
> >  
> > -static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > -			 struct intel_guc_fw *guc_fw)
> > +void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> > +			 struct intel_uc_fw *uc_fw)
> >  {
> >  	struct pci_dev *pdev = dev_priv->drm.pdev;
> >  	struct drm_i915_gem_object *obj;
> > @@ -597,17 +597,17 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> >  	size_t size;
> >  	int err;
> >  
> > -	DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n",
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> > +	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
> > +		intel_uc_fw_status_repr(uc_fw->fetch_status));
> >  
> > -	err = request_firmware(&fw, guc_fw->guc_fw_path, &pdev->dev);
> > +	err = request_firmware(&fw, uc_fw->uc_fw_path, &pdev->dev);
> >  	if (err)
> >  		goto fail;
> >  	if (!fw)
> >  		goto fail;
> >  
> > -	DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n",
> > -		guc_fw->guc_fw_path, fw);
> > +	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
> > +		uc_fw->uc_fw_path, fw);
> >  
> >  	/* Check the size of the blob before examining buffer contents */
> >  	if (fw->size < sizeof(struct guc_css_header)) {
> > @@ -618,36 +618,36 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> >  	css = (struct guc_css_header *)fw->data;
> >  
> >  	/* Firmware bits always start from header */
> > -	guc_fw->header_offset = 0;
> > -	guc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
> > +	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 (guc_fw->header_size != sizeof(struct guc_css_header)) {
> > +	if (uc_fw->header_size != sizeof(struct guc_css_header)) {
> >  		DRM_NOTE("CSS header definition mismatch\n");
> >  		goto fail;
> >  	}
> >  
> >  	/* then, uCode */
> > -	guc_fw->ucode_offset = guc_fw->header_offset + guc_fw->header_size;
> > -	guc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
> > +	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
> > +	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
> >  
> >  	/* now RSA */
> >  	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
> >  		DRM_NOTE("RSA key size is bad\n");
> >  		goto fail;
> >  	}
> > -	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
> > -	guc_fw->rsa_size = css->key_size_dw * sizeof(u32);
> > +	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
> > +	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
> >  
> >  	/* At least, it should have header, uCode and RSA. Size of all three. */
> > -	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
> > +	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
> >  	if (fw->size < size) {
> >  		DRM_NOTE("Missing firmware components\n");
> >  		goto fail;
> >  	}
> >  
> >  	/* Header and uCode will be loaded to WOPCM. Size of the two. */
> > -	size = guc_fw->header_size + guc_fw->ucode_size;
> > +	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;
> > @@ -659,21 +659,21 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> >  	 * TWO bytes each (i.e. u16), although all pointers and offsets are defined
> >  	 * in terms of bytes (u8).
> >  	 */
> > -	guc_fw->guc_fw_major_found = css->guc_sw_version >> 16;
> > -	guc_fw->guc_fw_minor_found = css->guc_sw_version & 0xFFFF;
> > -
> > -	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
> > -	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
> > -		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
> > -			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> > -			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> > +	uc_fw->major_ver_found = css->guc_sw_version >> 16;
> > +	uc_fw->minor_ver_found = css->guc_sw_version & 0xFFFF;
> > +
> > +	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> > +	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> > +		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
> > +			uc_fw->major_ver_found, uc_fw->minor_ver_found,
> > +			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> >  		err = -ENOEXEC;
> >  		goto fail;
> >  	}
> >  
> >  	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
> > -			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> > -			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> > +			uc_fw->major_ver_found, uc_fw->minor_ver_found,
> > +			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> >  
> >  	mutex_lock(&dev_priv->drm.struct_mutex);
> >  	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
> > @@ -683,31 +683,31 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> >  		goto fail;
> >  	}
> >  
> > -	guc_fw->guc_fw_obj = obj;
> > -	guc_fw->guc_fw_size = fw->size;
> > +	uc_fw->uc_fw_obj = obj;
> > +	uc_fw->uc_fw_size = fw->size;
> >  
> > -	DRM_DEBUG_DRIVER("GuC fw fetch status SUCCESS, obj %p\n",
> > -			guc_fw->guc_fw_obj);
> > +	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
> > +			uc_fw->uc_fw_obj);
> >  
> >  	release_firmware(fw);
> > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_SUCCESS;
> > +	uc_fw->fetch_status = UC_FIRMWARE_SUCCESS;
> >  	return;
> >  
> >  fail:
> > -	DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",
> > -		 guc_fw->guc_fw_path, err);
> > -	DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
> > -		err, fw, guc_fw->guc_fw_obj);
> > +	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
> > +		 uc_fw->uc_fw_path, err);
> > +	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
> > +		err, fw, uc_fw->uc_fw_obj);
> >  
> >  	mutex_lock(&dev_priv->drm.struct_mutex);
> > -	obj = guc_fw->guc_fw_obj;
> > +	obj = uc_fw->uc_fw_obj;
> >  	if (obj)
> >  		i915_gem_object_put(obj);
> > -	guc_fw->guc_fw_obj = NULL;
> > +	uc_fw->uc_fw_obj = NULL;
> >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> >  
> >  	release_firmware(fw);		/* OK even if fw is NULL */
> > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
> > +	uc_fw->fetch_status = UC_FIRMWARE_FAIL;
> >  }
> >  
> >  /**
> > @@ -721,7 +721,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> >   */
> >  void intel_guc_init(struct drm_i915_private *dev_priv)
> >  {
> > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  	const char *fw_path;
> >  
> >  	if (!HAS_GUC(dev_priv)) {
> > @@ -739,23 +739,24 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> >  		fw_path = NULL;
> >  	} else if (IS_SKYLAKE(dev_priv)) {
> >  		fw_path = I915_SKL_GUC_UCODE;
> > -		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
> > -		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
> > +		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
> > +		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
> >  	} else if (IS_BROXTON(dev_priv)) {
> >  		fw_path = I915_BXT_GUC_UCODE;
> > -		guc_fw->guc_fw_major_wanted = BXT_FW_MAJOR;
> > -		guc_fw->guc_fw_minor_wanted = BXT_FW_MINOR;
> > +		guc_fw->major_ver_wanted = BXT_FW_MAJOR;
> > +		guc_fw->minor_ver_wanted = BXT_FW_MINOR;
> >  	} else if (IS_KABYLAKE(dev_priv)) {
> >  		fw_path = I915_KBL_GUC_UCODE;
> > -		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
> > -		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
> > +		guc_fw->major_ver_wanted = KBL_FW_MAJOR;
> > +		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
> >  	} else {
> >  		fw_path = "";	/* unknown device */
> >  	}
> >  
> > -	guc_fw->guc_fw_path = fw_path;
> > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
> > +	guc_fw->uc_dev = &dev_priv->drm;
> > +	guc_fw->uc_fw_path = fw_path;
> > +	guc_fw->fetch_status = UC_FIRMWARE_NONE;
> > +	guc_fw->load_status = UC_FIRMWARE_NONE;
> >  
> >  	/* Early (and silent) return if GuC loading is disabled */
> >  	if (!i915.enable_guc_loading)
> > @@ -765,9 +766,9 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> >  	if (*fw_path == '\0')
> >  		return;
> >  
> > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
> > +	guc_fw->fetch_status = UC_FIRMWARE_PENDING;
> >  	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
> > -	guc_fw_fetch(dev_priv, guc_fw);
> > +	intel_uc_fw_fetch(dev_priv, guc_fw);
> >  	/* status must now be FAIL or SUCCESS */
> >  }
> >  
> > @@ -777,17 +778,17 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> >   */
> >  void intel_guc_fini(struct drm_i915_private *dev_priv)
> >  {
> > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  
> >  	mutex_lock(&dev_priv->drm.struct_mutex);
> >  	guc_interrupts_release(dev_priv);
> >  	i915_guc_submission_disable(dev_priv);
> >  	i915_guc_submission_fini(dev_priv);
> >  
> > -	if (guc_fw->guc_fw_obj)
> > -		i915_gem_object_put(guc_fw->guc_fw_obj);
> > -	guc_fw->guc_fw_obj = NULL;
> > +	if (guc_fw->uc_fw_obj)
> > +		i915_gem_object_put(guc_fw->uc_fw_obj);
> > +	guc_fw->uc_fw_obj = NULL;
> >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> >  
> > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> > +	guc_fw->fetch_status = UC_FIRMWARE_NONE;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > index 11f5608..f9f598d 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -91,29 +91,31 @@ struct i915_guc_client {
> >  	uint64_t submissions[I915_NUM_ENGINES];
> >  };
> >  
> > -enum intel_guc_fw_status {
> > -	GUC_FIRMWARE_FAIL = -1,
> > -	GUC_FIRMWARE_NONE = 0,
> > -	GUC_FIRMWARE_PENDING,
> > -	GUC_FIRMWARE_SUCCESS
> > +enum intel_uc_fw_status {
> > +	UC_FIRMWARE_FAIL = -1,
> > +	UC_FIRMWARE_NONE = 0,
> > +	UC_FIRMWARE_PENDING,
> > +	UC_FIRMWARE_SUCCESS
> 
> Shouldn't we use INTEL_UC_ prefix here (in similar way as in GUC_ACTION)?
> 
> 
> >  };
> >  
> >  /*
> >   * This structure encapsulates all the data needed during the process
> >   * of fetching, caching, and loading the firmware image into the GuC.
> >   */
> > -struct intel_guc_fw {
> > -	const char *			guc_fw_path;
> > -	size_t				guc_fw_size;
> > -	struct drm_i915_gem_object *	guc_fw_obj;
> > -	enum intel_guc_fw_status	guc_fw_fetch_status;
> > -	enum intel_guc_fw_status	guc_fw_load_status;
> > -
> > -	uint16_t			guc_fw_major_wanted;
> > -	uint16_t			guc_fw_minor_wanted;
> > -	uint16_t			guc_fw_major_found;
> > -	uint16_t			guc_fw_minor_found;
> > -
> > +struct intel_uc_fw {
> > +	struct drm_device *uc_dev;
> 
> Why do you need this back pointer to drm_dev ?
> More likely all you need is pointer to dev_priv.
> But for dev_priv you can always use guc_to_i915().

It seems like a left over from the old days. I have a series that
refactors all things GuC that I'll push here soon, including the removal
of this field.

Since this series advertises as a simple renaming I didn't force it
here.

> > +	const char *uc_fw_path;
> > +	size_t uc_fw_size;
> > +	struct drm_i915_gem_object *uc_fw_obj;
> 
> All these uc_fw_ prefixes seem to be redundant as this struct is just about uc_fw.
> 
> 
> > +	enum intel_uc_fw_status fetch_status;
> > +	enum intel_uc_fw_status load_status;
> > +
> > +	uint16_t major_ver_wanted;
> > +	uint16_t minor_ver_wanted;
> > +	uint16_t major_ver_found;
> > +	uint16_t minor_ver_found;
> > +
> > +	uint32_t fw_type;
> 
> What is the purpose of this field ?

It is used in the patch 2, to set {major,minor}_ver_found depending on
which firmware we pass to intel_uc_fw_fetch()

> 
> >  	uint32_t header_size;
> >  	uint32_t header_offset;
> >  	uint32_t rsa_size;
> > @@ -139,7 +141,7 @@ struct intel_guc_log {
> >  };
> >  
> >  struct intel_guc {
> > -	struct intel_guc_fw guc_fw;
> > +	struct intel_uc_fw guc_fw;
> 
> Btw, guc_ prefix in field name here is also redundant.
> 
> 
> >  	struct intel_guc_log log;
> >  
> >  	/* intel_guc_recv interrupt related state */
> > @@ -181,7 +183,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val);
> >  extern void intel_guc_init(struct drm_i915_private *dev_priv);
> >  extern int intel_guc_setup(struct drm_i915_private *dev_priv);
> >  extern void intel_guc_fini(struct drm_i915_private *dev_priv);
> > -extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
> > +extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
> >  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
> >  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> >  
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Michal Wajdeczko Dec. 9, 2016, 1:06 p.m. UTC | #4

On Fri, Dec 09, 2016 at 12:49:02PM +0100, Arkadiusz Hiler wrote:
> On Fri, Dec 09, 2016 at 12:28:52PM +0100, Michal Wajdeczko wrote:
> > On Thu, Dec 08, 2016 at 03:02:12PM -0800, anushasr wrote:
> > > From: Peter Antoine <[e-mail]>
> > > 
> > > Rename some of the GuC fw loading code to make them more general. We
> > > will utilise them for HuC loading as well.
> > >      s/intel_guc_fw/intel_uc_fw/g
> > >      s/GUC_FIRMWARE/UC_FIRMWARE/g
> > > 
> > > Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members,
> > > such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for
> > > same purpose.
> > > 
> > > v2: rebased on top of nightly.
> > >     reapplied the search/replace as upstream code as changed.
> > > v3: rebased again on drm-nightly.
> > > v4: removed G from messages in shared fw fetch function.
> > > v5: rebased.
> > > v7: rebased.
> > > v8: rebased.
> > > v9: rebased.
> > > v10: rebased.
> > > v11: rebased.
> > > v12: rebased on top of drm-tip
> > > v13: rebased.Updated dev to dev_priv in intel_guc_setup(), guc_fw_getch()
> > > and intel_guc_init().
> > > 
> > > Signed-off-by: Anusha Srivatsa <[e-mail]>
> > > Signed-off-by: Alex Dai <[e-mail]>
> > > Signed-off-by: Peter Antoine <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c        |  12 +--
> > >  drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
> > >  drivers/gpu/drm/i915/intel_guc_loader.c    | 157 +++++++++++++++--------------
> > >  drivers/gpu/drm/i915/intel_uc.h            |  40 ++++----
> > >  4 files changed, 108 insertions(+), 105 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index a746130..0e5ef62 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2338,7 +2338,7 @@ static int i915_llc(struct seq_file *m, void *data)
> > >  static int i915_guc_load_status_info(struct seq_file *m, void *data)
> > >  {
> > >  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > >  	u32 tmp, i;
> > >  
> > >  	if (!HAS_GUC_UCODE(dev_priv))
> > > @@ -2346,15 +2346,15 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
> > >  
> > >  	seq_printf(m, "GuC firmware status:\n");
> > >  	seq_printf(m, "\tpath: %s\n",
> > > -		guc_fw->guc_fw_path);
> > > +		guc_fw->uc_fw_path);
> > >  	seq_printf(m, "\tfetch: %s\n",
> > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> > > +		intel_uc_fw_status_repr(guc_fw->fetch_status));
> > >  	seq_printf(m, "\tload: %s\n",
> > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > > +		intel_uc_fw_status_repr(guc_fw->load_status));
> > >  	seq_printf(m, "\tversion wanted: %d.%d\n",
> > > -		guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> > > +		guc_fw->major_ver_wanted, guc_fw->minor_ver_wanted);
> > >  	seq_printf(m, "\tversion found: %d.%d\n",
> > > -		guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found);
> > > +		guc_fw->major_ver_found, guc_fw->minor_ver_found);
> > >  	seq_printf(m, "\theader: offset is %d; size = %d\n",
> > >  		guc_fw->header_offset, guc_fw->header_size);
> > >  	seq_printf(m, "\tuCode: offset is %d; size = %d\n",
> > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > index 7fa4e74..e156a4b 100644
> > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > @@ -1493,7 +1493,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
> > >  	struct i915_gem_context *ctx;
> > >  	u32 data[3];
> > >  
> > > -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
> > > +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
> > >  		return 0;
> > >  
> > >  	gen9_disable_guc_interrupts(dev_priv);
> > > @@ -1520,7 +1520,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
> > >  	struct i915_gem_context *ctx;
> > >  	u32 data[3];
> > >  
> > > -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
> > > +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
> > >  		return 0;
> > >  
> > >  	if (i915.guc_log_level >= 0)
> > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > > index 21db697..8f04f6e 100644
> > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > > @@ -81,16 +81,16 @@ MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
> > >  MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
> > >  
> > >  /* User-friendly representation of an enum */
> > > -const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
> > > +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
> > >  {
> > >  	switch (status) {
> > > -	case GUC_FIRMWARE_FAIL:
> > > +	case UC_FIRMWARE_FAIL:
> > >  		return "FAIL";
> > > -	case GUC_FIRMWARE_NONE:
> > > +	case UC_FIRMWARE_NONE:
> > >  		return "NONE";
> > > -	case GUC_FIRMWARE_PENDING:
> > > +	case UC_FIRMWARE_PENDING:
> > >  		return "PENDING";
> > > -	case GUC_FIRMWARE_SUCCESS:
> > > +	case UC_FIRMWARE_SUCCESS:
> > >  		return "SUCCESS";
> > >  	default:
> > >  		return "UNKNOWN!";
> > > @@ -278,7 +278,7 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
> > >  static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
> > >  			      struct i915_vma *vma)
> > >  {
> > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > >  	unsigned long offset;
> > >  	struct sg_table *sg = vma->pages;
> > >  	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
> > > @@ -350,17 +350,17 @@ static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> > >   */
> > >  static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
> > >  {
> > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > >  	struct i915_vma *vma;
> > >  	int ret;
> > >  
> > > -	ret = i915_gem_object_set_to_gtt_domain(guc_fw->guc_fw_obj, false);
> > > +	ret = i915_gem_object_set_to_gtt_domain(guc_fw->uc_fw_obj, false);
> > >  	if (ret) {
> > >  		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> > >  		return ret;
> > >  	}
> > >  
> > > -	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);
> > > +	vma = i915_gem_object_ggtt_pin(guc_fw->uc_fw_obj, NULL, 0, 0, 0);
> > >  	if (IS_ERR(vma)) {
> > >  		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> > >  		return PTR_ERR(vma);
> > > @@ -450,14 +450,14 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
> > >   */
> > >  int intel_guc_setup(struct drm_i915_private *dev_priv)
> > >  {
> > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > -	const char *fw_path = guc_fw->guc_fw_path;
> > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > +	const char *fw_path = guc_fw->uc_fw_path;
> > >  	int retries, ret, err;
> > >  
> > >  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> > >  		fw_path,
> > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > > +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> > > +		intel_uc_fw_status_repr(guc_fw->load_status));
> > >  
> > >  	/* Loading forbidden, or no firmware to load? */
> > >  	if (!i915.enable_guc_loading) {
> > > @@ -475,10 +475,10 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > >  	}
> > >  
> > >  	/* Fetch failed, or already fetched but failed to load? */
> > > -	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> > > +	if (guc_fw->fetch_status != UC_FIRMWARE_SUCCESS) {
> > >  		err = -EIO;
> > >  		goto fail;
> > > -	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> > > +	} else if (guc_fw->load_status == UC_FIRMWARE_FAIL) {
> > >  		err = -ENOEXEC;
> > >  		goto fail;
> > >  	}
> > > @@ -486,11 +486,11 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > >  	guc_interrupts_release(dev_priv);
> > >  	gen9_reset_guc_interrupts(dev_priv);
> > >  
> > > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> > > +	guc_fw->load_status = UC_FIRMWARE_PENDING;
> > >  
> > >  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > > +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> > > +		intel_uc_fw_status_repr(guc_fw->load_status));
> > >  
> > >  	err = i915_guc_submission_init(dev_priv);
> > >  	if (err)
> > > @@ -522,11 +522,11 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > >  			 "retry %d more time(s)\n", err, retries);
> > >  	}
> > >  
> > > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
> > > +	guc_fw->load_status = UC_FIRMWARE_SUCCESS;
> > >  
> > >  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > > +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> > > +		intel_uc_fw_status_repr(guc_fw->load_status));
> > >  
> > >  	if (i915.enable_guc_submission) {
> > >  		if (i915.guc_log_level >= 0)
> > > @@ -541,8 +541,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > >  	return 0;
> > >  
> > >  fail:
> > > -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> > > -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
> > > +	if (guc_fw->load_status == UC_FIRMWARE_PENDING)
> > > +		guc_fw->load_status = UC_FIRMWARE_FAIL;
> > >  
> > >  	guc_interrupts_release(dev_priv);
> > >  	i915_guc_submission_disable(dev_priv);
> > > @@ -587,8 +587,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > >  	return ret;
> > >  }
> > >  
> > > -static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > > -			 struct intel_guc_fw *guc_fw)
> > > +void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> > > +			 struct intel_uc_fw *uc_fw)
> > >  {
> > >  	struct pci_dev *pdev = dev_priv->drm.pdev;
> > >  	struct drm_i915_gem_object *obj;
> > > @@ -597,17 +597,17 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > >  	size_t size;
> > >  	int err;
> > >  
> > > -	DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n",
> > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> > > +	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
> > > +		intel_uc_fw_status_repr(uc_fw->fetch_status));
> > >  
> > > -	err = request_firmware(&fw, guc_fw->guc_fw_path, &pdev->dev);
> > > +	err = request_firmware(&fw, uc_fw->uc_fw_path, &pdev->dev);
> > >  	if (err)
> > >  		goto fail;
> > >  	if (!fw)
> > >  		goto fail;
> > >  
> > > -	DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n",
> > > -		guc_fw->guc_fw_path, fw);
> > > +	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
> > > +		uc_fw->uc_fw_path, fw);
> > >  
> > >  	/* Check the size of the blob before examining buffer contents */
> > >  	if (fw->size < sizeof(struct guc_css_header)) {
> > > @@ -618,36 +618,36 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > >  	css = (struct guc_css_header *)fw->data;
> > >  
> > >  	/* Firmware bits always start from header */
> > > -	guc_fw->header_offset = 0;
> > > -	guc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
> > > +	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 (guc_fw->header_size != sizeof(struct guc_css_header)) {
> > > +	if (uc_fw->header_size != sizeof(struct guc_css_header)) {
> > >  		DRM_NOTE("CSS header definition mismatch\n");
> > >  		goto fail;
> > >  	}
> > >  
> > >  	/* then, uCode */
> > > -	guc_fw->ucode_offset = guc_fw->header_offset + guc_fw->header_size;
> > > -	guc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
> > > +	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
> > > +	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
> > >  
> > >  	/* now RSA */
> > >  	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
> > >  		DRM_NOTE("RSA key size is bad\n");
> > >  		goto fail;
> > >  	}
> > > -	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
> > > -	guc_fw->rsa_size = css->key_size_dw * sizeof(u32);
> > > +	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
> > > +	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
> > >  
> > >  	/* At least, it should have header, uCode and RSA. Size of all three. */
> > > -	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
> > > +	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
> > >  	if (fw->size < size) {
> > >  		DRM_NOTE("Missing firmware components\n");
> > >  		goto fail;
> > >  	}
> > >  
> > >  	/* Header and uCode will be loaded to WOPCM. Size of the two. */
> > > -	size = guc_fw->header_size + guc_fw->ucode_size;
> > > +	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;
> > > @@ -659,21 +659,21 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > >  	 * TWO bytes each (i.e. u16), although all pointers and offsets are defined
> > >  	 * in terms of bytes (u8).
> > >  	 */
> > > -	guc_fw->guc_fw_major_found = css->guc_sw_version >> 16;
> > > -	guc_fw->guc_fw_minor_found = css->guc_sw_version & 0xFFFF;
> > > -
> > > -	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
> > > -	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
> > > -		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
> > > -			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> > > -			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> > > +	uc_fw->major_ver_found = css->guc_sw_version >> 16;
> > > +	uc_fw->minor_ver_found = css->guc_sw_version & 0xFFFF;
> > > +
> > > +	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> > > +	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> > > +		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
> > > +			uc_fw->major_ver_found, uc_fw->minor_ver_found,
> > > +			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> > >  		err = -ENOEXEC;
> > >  		goto fail;
> > >  	}
> > >  
> > >  	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
> > > -			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> > > -			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> > > +			uc_fw->major_ver_found, uc_fw->minor_ver_found,
> > > +			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> > >  
> > >  	mutex_lock(&dev_priv->drm.struct_mutex);
> > >  	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
> > > @@ -683,31 +683,31 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > >  		goto fail;
> > >  	}
> > >  
> > > -	guc_fw->guc_fw_obj = obj;
> > > -	guc_fw->guc_fw_size = fw->size;
> > > +	uc_fw->uc_fw_obj = obj;
> > > +	uc_fw->uc_fw_size = fw->size;
> > >  
> > > -	DRM_DEBUG_DRIVER("GuC fw fetch status SUCCESS, obj %p\n",
> > > -			guc_fw->guc_fw_obj);
> > > +	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
> > > +			uc_fw->uc_fw_obj);
> > >  
> > >  	release_firmware(fw);
> > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_SUCCESS;
> > > +	uc_fw->fetch_status = UC_FIRMWARE_SUCCESS;
> > >  	return;
> > >  
> > >  fail:
> > > -	DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",
> > > -		 guc_fw->guc_fw_path, err);
> > > -	DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
> > > -		err, fw, guc_fw->guc_fw_obj);
> > > +	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
> > > +		 uc_fw->uc_fw_path, err);
> > > +	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
> > > +		err, fw, uc_fw->uc_fw_obj);
> > >  
> > >  	mutex_lock(&dev_priv->drm.struct_mutex);
> > > -	obj = guc_fw->guc_fw_obj;
> > > +	obj = uc_fw->uc_fw_obj;
> > >  	if (obj)
> > >  		i915_gem_object_put(obj);
> > > -	guc_fw->guc_fw_obj = NULL;
> > > +	uc_fw->uc_fw_obj = NULL;
> > >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> > >  
> > >  	release_firmware(fw);		/* OK even if fw is NULL */
> > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
> > > +	uc_fw->fetch_status = UC_FIRMWARE_FAIL;
> > >  }
> > >  
> > >  /**
> > > @@ -721,7 +721,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > >   */
> > >  void intel_guc_init(struct drm_i915_private *dev_priv)
> > >  {
> > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > >  	const char *fw_path;
> > >  
> > >  	if (!HAS_GUC(dev_priv)) {
> > > @@ -739,23 +739,24 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> > >  		fw_path = NULL;
> > >  	} else if (IS_SKYLAKE(dev_priv)) {
> > >  		fw_path = I915_SKL_GUC_UCODE;
> > > -		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
> > > -		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
> > > +		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
> > > +		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
> > >  	} else if (IS_BROXTON(dev_priv)) {
> > >  		fw_path = I915_BXT_GUC_UCODE;
> > > -		guc_fw->guc_fw_major_wanted = BXT_FW_MAJOR;
> > > -		guc_fw->guc_fw_minor_wanted = BXT_FW_MINOR;
> > > +		guc_fw->major_ver_wanted = BXT_FW_MAJOR;
> > > +		guc_fw->minor_ver_wanted = BXT_FW_MINOR;
> > >  	} else if (IS_KABYLAKE(dev_priv)) {
> > >  		fw_path = I915_KBL_GUC_UCODE;
> > > -		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
> > > -		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
> > > +		guc_fw->major_ver_wanted = KBL_FW_MAJOR;
> > > +		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
> > >  	} else {
> > >  		fw_path = "";	/* unknown device */
> > >  	}
> > >  
> > > -	guc_fw->guc_fw_path = fw_path;
> > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> > > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
> > > +	guc_fw->uc_dev = &dev_priv->drm;
> > > +	guc_fw->uc_fw_path = fw_path;
> > > +	guc_fw->fetch_status = UC_FIRMWARE_NONE;
> > > +	guc_fw->load_status = UC_FIRMWARE_NONE;
> > >  
> > >  	/* Early (and silent) return if GuC loading is disabled */
> > >  	if (!i915.enable_guc_loading)
> > > @@ -765,9 +766,9 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> > >  	if (*fw_path == '\0')
> > >  		return;
> > >  
> > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
> > > +	guc_fw->fetch_status = UC_FIRMWARE_PENDING;
> > >  	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
> > > -	guc_fw_fetch(dev_priv, guc_fw);
> > > +	intel_uc_fw_fetch(dev_priv, guc_fw);
> > >  	/* status must now be FAIL or SUCCESS */
> > >  }
> > >  
> > > @@ -777,17 +778,17 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> > >   */
> > >  void intel_guc_fini(struct drm_i915_private *dev_priv)
> > >  {
> > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > >  
> > >  	mutex_lock(&dev_priv->drm.struct_mutex);
> > >  	guc_interrupts_release(dev_priv);
> > >  	i915_guc_submission_disable(dev_priv);
> > >  	i915_guc_submission_fini(dev_priv);
> > >  
> > > -	if (guc_fw->guc_fw_obj)
> > > -		i915_gem_object_put(guc_fw->guc_fw_obj);
> > > -	guc_fw->guc_fw_obj = NULL;
> > > +	if (guc_fw->uc_fw_obj)
> > > +		i915_gem_object_put(guc_fw->uc_fw_obj);
> > > +	guc_fw->uc_fw_obj = NULL;
> > >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> > >  
> > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> > > +	guc_fw->fetch_status = UC_FIRMWARE_NONE;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > > index 11f5608..f9f598d 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.h
> > > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > > @@ -91,29 +91,31 @@ struct i915_guc_client {
> > >  	uint64_t submissions[I915_NUM_ENGINES];
> > >  };
> > >  
> > > -enum intel_guc_fw_status {
> > > -	GUC_FIRMWARE_FAIL = -1,
> > > -	GUC_FIRMWARE_NONE = 0,
> > > -	GUC_FIRMWARE_PENDING,
> > > -	GUC_FIRMWARE_SUCCESS
> > > +enum intel_uc_fw_status {
> > > +	UC_FIRMWARE_FAIL = -1,
> > > +	UC_FIRMWARE_NONE = 0,
> > > +	UC_FIRMWARE_PENDING,
> > > +	UC_FIRMWARE_SUCCESS
> > 
> > Shouldn't we use INTEL_UC_ prefix here (in similar way as in GUC_ACTION)?
> > 
> > 
> > >  };
> > >  
> > >  /*
> > >   * This structure encapsulates all the data needed during the process
> > >   * of fetching, caching, and loading the firmware image into the GuC.
> > >   */
> > > -struct intel_guc_fw {
> > > -	const char *			guc_fw_path;
> > > -	size_t				guc_fw_size;
> > > -	struct drm_i915_gem_object *	guc_fw_obj;
> > > -	enum intel_guc_fw_status	guc_fw_fetch_status;
> > > -	enum intel_guc_fw_status	guc_fw_load_status;
> > > -
> > > -	uint16_t			guc_fw_major_wanted;
> > > -	uint16_t			guc_fw_minor_wanted;
> > > -	uint16_t			guc_fw_major_found;
> > > -	uint16_t			guc_fw_minor_found;
> > > -
> > > +struct intel_uc_fw {
> > > +	struct drm_device *uc_dev;
> > 
> > Why do you need this back pointer to drm_dev ?
> > More likely all you need is pointer to dev_priv.
> > But for dev_priv you can always use guc_to_i915().
> 
> It seems like a left over from the old days. I have a series that
> refactors all things GuC that I'll push here soon, including the removal
> of this field.
> 
> Since this series advertises as a simple renaming I didn't force it
> here.

If you want to treat this as simple 'renaming' then 'adding' unused field
is even more undesired and shall be avoided. 

> 
> > > +	const char *uc_fw_path;
> > > +	size_t uc_fw_size;
> > > +	struct drm_i915_gem_object *uc_fw_obj;
> > 
> > All these uc_fw_ prefixes seem to be redundant as this struct is just about uc_fw.
> > 
> > 
> > > +	enum intel_uc_fw_status fetch_status;
> > > +	enum intel_uc_fw_status load_status;
> > > +
> > > +	uint16_t major_ver_wanted;
> > > +	uint16_t minor_ver_wanted;
> > > +	uint16_t major_ver_found;
> > > +	uint16_t minor_ver_found;
> > > +
> > > +	uint32_t fw_type;
> > 
> > What is the purpose of this field ?
> 
> It is used in the patch 2, to set {major,minor}_ver_found depending on
> which firmware we pass to intel_uc_fw_fetch()

Then maybe is shall be moved to patch 2 ?


> 
> > 
> > >  	uint32_t header_size;
> > >  	uint32_t header_offset;
> > >  	uint32_t rsa_size;
> > > @@ -139,7 +141,7 @@ struct intel_guc_log {
> > >  };
> > >  
> > >  struct intel_guc {
> > > -	struct intel_guc_fw guc_fw;
> > > +	struct intel_uc_fw guc_fw;
> > 
> > Btw, guc_ prefix in field name here is also redundant.
> > 
> > 
> > >  	struct intel_guc_log log;
> > >  
> > >  	/* intel_guc_recv interrupt related state */
> > > @@ -181,7 +183,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val);
> > >  extern void intel_guc_init(struct drm_i915_private *dev_priv);
> > >  extern int intel_guc_setup(struct drm_i915_private *dev_priv);
> > >  extern void intel_guc_fini(struct drm_i915_private *dev_priv);
> > > -extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
> > > +extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
> > >  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
> > >  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> > >  
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Cheers,
> Arek
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Arkadiusz Hiler Dec. 9, 2016, 1:09 p.m. UTC | #5

On Fri, Dec 09, 2016 at 02:06:29PM +0100, Michal Wajdeczko wrote:
> On Fri, Dec 09, 2016 at 12:49:02PM +0100, Arkadiusz Hiler wrote:
> > On Fri, Dec 09, 2016 at 12:28:52PM +0100, Michal Wajdeczko wrote:
> > > On Thu, Dec 08, 2016 at 03:02:12PM -0800, anushasr wrote:
> > > > From: Peter Antoine <[e-mail]>
> > > > 
> > > > Rename some of the GuC fw loading code to make them more general. We
> > > > will utilise them for HuC loading as well.
> > > >      s/intel_guc_fw/intel_uc_fw/g
> > > >      s/GUC_FIRMWARE/UC_FIRMWARE/g
> > > > 
> > > > Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members,
> > > > such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for
> > > > same purpose.
> > > > 
> > > > v2: rebased on top of nightly.
> > > >     reapplied the search/replace as upstream code as changed.
> > > > v3: rebased again on drm-nightly.
> > > > v4: removed G from messages in shared fw fetch function.
> > > > v5: rebased.
> > > > v7: rebased.
> > > > v8: rebased.
> > > > v9: rebased.
> > > > v10: rebased.
> > > > v11: rebased.
> > > > v12: rebased on top of drm-tip
> > > > v13: rebased.Updated dev to dev_priv in intel_guc_setup(), guc_fw_getch()
> > > > and intel_guc_init().
> > > > 
> > > > Signed-off-by: Anusha Srivatsa <[e-mail]>
> > > > Signed-off-by: Alex Dai <[e-mail]>
> > > > Signed-off-by: Peter Antoine <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c        |  12 +--
> > > >  drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
> > > >  drivers/gpu/drm/i915/intel_guc_loader.c    | 157 +++++++++++++++--------------
> > > >  drivers/gpu/drm/i915/intel_uc.h            |  40 ++++----
> > > >  4 files changed, 108 insertions(+), 105 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index a746130..0e5ef62 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2338,7 +2338,7 @@ static int i915_llc(struct seq_file *m, void *data)
> > > >  static int i915_guc_load_status_info(struct seq_file *m, void *data)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > >  	u32 tmp, i;
> > > >  
> > > >  	if (!HAS_GUC_UCODE(dev_priv))
> > > > @@ -2346,15 +2346,15 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
> > > >  
> > > >  	seq_printf(m, "GuC firmware status:\n");
> > > >  	seq_printf(m, "\tpath: %s\n",
> > > > -		guc_fw->guc_fw_path);
> > > > +		guc_fw->uc_fw_path);
> > > >  	seq_printf(m, "\tfetch: %s\n",
> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> > > > +		intel_uc_fw_status_repr(guc_fw->fetch_status));
> > > >  	seq_printf(m, "\tload: %s\n",
> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > > > +		intel_uc_fw_status_repr(guc_fw->load_status));
> > > >  	seq_printf(m, "\tversion wanted: %d.%d\n",
> > > > -		guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> > > > +		guc_fw->major_ver_wanted, guc_fw->minor_ver_wanted);
> > > >  	seq_printf(m, "\tversion found: %d.%d\n",
> > > > -		guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found);
> > > > +		guc_fw->major_ver_found, guc_fw->minor_ver_found);
> > > >  	seq_printf(m, "\theader: offset is %d; size = %d\n",
> > > >  		guc_fw->header_offset, guc_fw->header_size);
> > > >  	seq_printf(m, "\tuCode: offset is %d; size = %d\n",
> > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > index 7fa4e74..e156a4b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > @@ -1493,7 +1493,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
> > > >  	struct i915_gem_context *ctx;
> > > >  	u32 data[3];
> > > >  
> > > > -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
> > > > +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
> > > >  		return 0;
> > > >  
> > > >  	gen9_disable_guc_interrupts(dev_priv);
> > > > @@ -1520,7 +1520,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
> > > >  	struct i915_gem_context *ctx;
> > > >  	u32 data[3];
> > > >  
> > > > -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
> > > > +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
> > > >  		return 0;
> > > >  
> > > >  	if (i915.guc_log_level >= 0)
> > > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > > > index 21db697..8f04f6e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > > > @@ -81,16 +81,16 @@ MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
> > > >  MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
> > > >  
> > > >  /* User-friendly representation of an enum */
> > > > -const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
> > > > +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
> > > >  {
> > > >  	switch (status) {
> > > > -	case GUC_FIRMWARE_FAIL:
> > > > +	case UC_FIRMWARE_FAIL:
> > > >  		return "FAIL";
> > > > -	case GUC_FIRMWARE_NONE:
> > > > +	case UC_FIRMWARE_NONE:
> > > >  		return "NONE";
> > > > -	case GUC_FIRMWARE_PENDING:
> > > > +	case UC_FIRMWARE_PENDING:
> > > >  		return "PENDING";
> > > > -	case GUC_FIRMWARE_SUCCESS:
> > > > +	case UC_FIRMWARE_SUCCESS:
> > > >  		return "SUCCESS";
> > > >  	default:
> > > >  		return "UNKNOWN!";
> > > > @@ -278,7 +278,7 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
> > > >  static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
> > > >  			      struct i915_vma *vma)
> > > >  {
> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > >  	unsigned long offset;
> > > >  	struct sg_table *sg = vma->pages;
> > > >  	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
> > > > @@ -350,17 +350,17 @@ static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> > > >   */
> > > >  static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
> > > >  {
> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > >  	struct i915_vma *vma;
> > > >  	int ret;
> > > >  
> > > > -	ret = i915_gem_object_set_to_gtt_domain(guc_fw->guc_fw_obj, false);
> > > > +	ret = i915_gem_object_set_to_gtt_domain(guc_fw->uc_fw_obj, false);
> > > >  	if (ret) {
> > > >  		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > -	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);
> > > > +	vma = i915_gem_object_ggtt_pin(guc_fw->uc_fw_obj, NULL, 0, 0, 0);
> > > >  	if (IS_ERR(vma)) {
> > > >  		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> > > >  		return PTR_ERR(vma);
> > > > @@ -450,14 +450,14 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
> > > >   */
> > > >  int intel_guc_setup(struct drm_i915_private *dev_priv)
> > > >  {
> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > > -	const char *fw_path = guc_fw->guc_fw_path;
> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > > +	const char *fw_path = guc_fw->uc_fw_path;
> > > >  	int retries, ret, err;
> > > >  
> > > >  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> > > >  		fw_path,
> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > > > +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> > > > +		intel_uc_fw_status_repr(guc_fw->load_status));
> > > >  
> > > >  	/* Loading forbidden, or no firmware to load? */
> > > >  	if (!i915.enable_guc_loading) {
> > > > @@ -475,10 +475,10 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > > >  	}
> > > >  
> > > >  	/* Fetch failed, or already fetched but failed to load? */
> > > > -	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> > > > +	if (guc_fw->fetch_status != UC_FIRMWARE_SUCCESS) {
> > > >  		err = -EIO;
> > > >  		goto fail;
> > > > -	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> > > > +	} else if (guc_fw->load_status == UC_FIRMWARE_FAIL) {
> > > >  		err = -ENOEXEC;
> > > >  		goto fail;
> > > >  	}
> > > > @@ -486,11 +486,11 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > > >  	guc_interrupts_release(dev_priv);
> > > >  	gen9_reset_guc_interrupts(dev_priv);
> > > >  
> > > > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> > > > +	guc_fw->load_status = UC_FIRMWARE_PENDING;
> > > >  
> > > >  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > > > +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> > > > +		intel_uc_fw_status_repr(guc_fw->load_status));
> > > >  
> > > >  	err = i915_guc_submission_init(dev_priv);
> > > >  	if (err)
> > > > @@ -522,11 +522,11 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > > >  			 "retry %d more time(s)\n", err, retries);
> > > >  	}
> > > >  
> > > > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
> > > > +	guc_fw->load_status = UC_FIRMWARE_SUCCESS;
> > > >  
> > > >  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > > > +		intel_uc_fw_status_repr(guc_fw->fetch_status),
> > > > +		intel_uc_fw_status_repr(guc_fw->load_status));
> > > >  
> > > >  	if (i915.enable_guc_submission) {
> > > >  		if (i915.guc_log_level >= 0)
> > > > @@ -541,8 +541,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > > >  	return 0;
> > > >  
> > > >  fail:
> > > > -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> > > > -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
> > > > +	if (guc_fw->load_status == UC_FIRMWARE_PENDING)
> > > > +		guc_fw->load_status = UC_FIRMWARE_FAIL;
> > > >  
> > > >  	guc_interrupts_release(dev_priv);
> > > >  	i915_guc_submission_disable(dev_priv);
> > > > @@ -587,8 +587,8 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > -static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > > > -			 struct intel_guc_fw *guc_fw)
> > > > +void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> > > > +			 struct intel_uc_fw *uc_fw)
> > > >  {
> > > >  	struct pci_dev *pdev = dev_priv->drm.pdev;
> > > >  	struct drm_i915_gem_object *obj;
> > > > @@ -597,17 +597,17 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > > >  	size_t size;
> > > >  	int err;
> > > >  
> > > > -	DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n",
> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> > > > +	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
> > > > +		intel_uc_fw_status_repr(uc_fw->fetch_status));
> > > >  
> > > > -	err = request_firmware(&fw, guc_fw->guc_fw_path, &pdev->dev);
> > > > +	err = request_firmware(&fw, uc_fw->uc_fw_path, &pdev->dev);
> > > >  	if (err)
> > > >  		goto fail;
> > > >  	if (!fw)
> > > >  		goto fail;
> > > >  
> > > > -	DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n",
> > > > -		guc_fw->guc_fw_path, fw);
> > > > +	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
> > > > +		uc_fw->uc_fw_path, fw);
> > > >  
> > > >  	/* Check the size of the blob before examining buffer contents */
> > > >  	if (fw->size < sizeof(struct guc_css_header)) {
> > > > @@ -618,36 +618,36 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > > >  	css = (struct guc_css_header *)fw->data;
> > > >  
> > > >  	/* Firmware bits always start from header */
> > > > -	guc_fw->header_offset = 0;
> > > > -	guc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
> > > > +	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 (guc_fw->header_size != sizeof(struct guc_css_header)) {
> > > > +	if (uc_fw->header_size != sizeof(struct guc_css_header)) {
> > > >  		DRM_NOTE("CSS header definition mismatch\n");
> > > >  		goto fail;
> > > >  	}
> > > >  
> > > >  	/* then, uCode */
> > > > -	guc_fw->ucode_offset = guc_fw->header_offset + guc_fw->header_size;
> > > > -	guc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
> > > > +	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
> > > > +	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
> > > >  
> > > >  	/* now RSA */
> > > >  	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
> > > >  		DRM_NOTE("RSA key size is bad\n");
> > > >  		goto fail;
> > > >  	}
> > > > -	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
> > > > -	guc_fw->rsa_size = css->key_size_dw * sizeof(u32);
> > > > +	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
> > > > +	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
> > > >  
> > > >  	/* At least, it should have header, uCode and RSA. Size of all three. */
> > > > -	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
> > > > +	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
> > > >  	if (fw->size < size) {
> > > >  		DRM_NOTE("Missing firmware components\n");
> > > >  		goto fail;
> > > >  	}
> > > >  
> > > >  	/* Header and uCode will be loaded to WOPCM. Size of the two. */
> > > > -	size = guc_fw->header_size + guc_fw->ucode_size;
> > > > +	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;
> > > > @@ -659,21 +659,21 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > > >  	 * TWO bytes each (i.e. u16), although all pointers and offsets are defined
> > > >  	 * in terms of bytes (u8).
> > > >  	 */
> > > > -	guc_fw->guc_fw_major_found = css->guc_sw_version >> 16;
> > > > -	guc_fw->guc_fw_minor_found = css->guc_sw_version & 0xFFFF;
> > > > -
> > > > -	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
> > > > -	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
> > > > -		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
> > > > -			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> > > > -			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> > > > +	uc_fw->major_ver_found = css->guc_sw_version >> 16;
> > > > +	uc_fw->minor_ver_found = css->guc_sw_version & 0xFFFF;
> > > > +
> > > > +	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> > > > +	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> > > > +		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
> > > > +			uc_fw->major_ver_found, uc_fw->minor_ver_found,
> > > > +			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> > > >  		err = -ENOEXEC;
> > > >  		goto fail;
> > > >  	}
> > > >  
> > > >  	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
> > > > -			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> > > > -			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> > > > +			uc_fw->major_ver_found, uc_fw->minor_ver_found,
> > > > +			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> > > >  
> > > >  	mutex_lock(&dev_priv->drm.struct_mutex);
> > > >  	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
> > > > @@ -683,31 +683,31 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > > >  		goto fail;
> > > >  	}
> > > >  
> > > > -	guc_fw->guc_fw_obj = obj;
> > > > -	guc_fw->guc_fw_size = fw->size;
> > > > +	uc_fw->uc_fw_obj = obj;
> > > > +	uc_fw->uc_fw_size = fw->size;
> > > >  
> > > > -	DRM_DEBUG_DRIVER("GuC fw fetch status SUCCESS, obj %p\n",
> > > > -			guc_fw->guc_fw_obj);
> > > > +	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
> > > > +			uc_fw->uc_fw_obj);
> > > >  
> > > >  	release_firmware(fw);
> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_SUCCESS;
> > > > +	uc_fw->fetch_status = UC_FIRMWARE_SUCCESS;
> > > >  	return;
> > > >  
> > > >  fail:
> > > > -	DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",
> > > > -		 guc_fw->guc_fw_path, err);
> > > > -	DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
> > > > -		err, fw, guc_fw->guc_fw_obj);
> > > > +	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
> > > > +		 uc_fw->uc_fw_path, err);
> > > > +	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
> > > > +		err, fw, uc_fw->uc_fw_obj);
> > > >  
> > > >  	mutex_lock(&dev_priv->drm.struct_mutex);
> > > > -	obj = guc_fw->guc_fw_obj;
> > > > +	obj = uc_fw->uc_fw_obj;
> > > >  	if (obj)
> > > >  		i915_gem_object_put(obj);
> > > > -	guc_fw->guc_fw_obj = NULL;
> > > > +	uc_fw->uc_fw_obj = NULL;
> > > >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> > > >  
> > > >  	release_firmware(fw);		/* OK even if fw is NULL */
> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
> > > > +	uc_fw->fetch_status = UC_FIRMWARE_FAIL;
> > > >  }
> > > >  
> > > >  /**
> > > > @@ -721,7 +721,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > > >   */
> > > >  void intel_guc_init(struct drm_i915_private *dev_priv)
> > > >  {
> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > >  	const char *fw_path;
> > > >  
> > > >  	if (!HAS_GUC(dev_priv)) {
> > > > @@ -739,23 +739,24 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> > > >  		fw_path = NULL;
> > > >  	} else if (IS_SKYLAKE(dev_priv)) {
> > > >  		fw_path = I915_SKL_GUC_UCODE;
> > > > -		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
> > > > -		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
> > > > +		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
> > > > +		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
> > > >  	} else if (IS_BROXTON(dev_priv)) {
> > > >  		fw_path = I915_BXT_GUC_UCODE;
> > > > -		guc_fw->guc_fw_major_wanted = BXT_FW_MAJOR;
> > > > -		guc_fw->guc_fw_minor_wanted = BXT_FW_MINOR;
> > > > +		guc_fw->major_ver_wanted = BXT_FW_MAJOR;
> > > > +		guc_fw->minor_ver_wanted = BXT_FW_MINOR;
> > > >  	} else if (IS_KABYLAKE(dev_priv)) {
> > > >  		fw_path = I915_KBL_GUC_UCODE;
> > > > -		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
> > > > -		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
> > > > +		guc_fw->major_ver_wanted = KBL_FW_MAJOR;
> > > > +		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
> > > >  	} else {
> > > >  		fw_path = "";	/* unknown device */
> > > >  	}
> > > >  
> > > > -	guc_fw->guc_fw_path = fw_path;
> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> > > > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
> > > > +	guc_fw->uc_dev = &dev_priv->drm;
> > > > +	guc_fw->uc_fw_path = fw_path;
> > > > +	guc_fw->fetch_status = UC_FIRMWARE_NONE;
> > > > +	guc_fw->load_status = UC_FIRMWARE_NONE;
> > > >  
> > > >  	/* Early (and silent) return if GuC loading is disabled */
> > > >  	if (!i915.enable_guc_loading)
> > > > @@ -765,9 +766,9 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> > > >  	if (*fw_path == '\0')
> > > >  		return;
> > > >  
> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
> > > > +	guc_fw->fetch_status = UC_FIRMWARE_PENDING;
> > > >  	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
> > > > -	guc_fw_fetch(dev_priv, guc_fw);
> > > > +	intel_uc_fw_fetch(dev_priv, guc_fw);
> > > >  	/* status must now be FAIL or SUCCESS */
> > > >  }
> > > >  
> > > > @@ -777,17 +778,17 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> > > >   */
> > > >  void intel_guc_fini(struct drm_i915_private *dev_priv)
> > > >  {
> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > > >  
> > > >  	mutex_lock(&dev_priv->drm.struct_mutex);
> > > >  	guc_interrupts_release(dev_priv);
> > > >  	i915_guc_submission_disable(dev_priv);
> > > >  	i915_guc_submission_fini(dev_priv);
> > > >  
> > > > -	if (guc_fw->guc_fw_obj)
> > > > -		i915_gem_object_put(guc_fw->guc_fw_obj);
> > > > -	guc_fw->guc_fw_obj = NULL;
> > > > +	if (guc_fw->uc_fw_obj)
> > > > +		i915_gem_object_put(guc_fw->uc_fw_obj);
> > > > +	guc_fw->uc_fw_obj = NULL;
> > > >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> > > >  
> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> > > > +	guc_fw->fetch_status = UC_FIRMWARE_NONE;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > > > index 11f5608..f9f598d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_uc.h
> > > > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > > > @@ -91,29 +91,31 @@ struct i915_guc_client {
> > > >  	uint64_t submissions[I915_NUM_ENGINES];
> > > >  };
> > > >  
> > > > -enum intel_guc_fw_status {
> > > > -	GUC_FIRMWARE_FAIL = -1,
> > > > -	GUC_FIRMWARE_NONE = 0,
> > > > -	GUC_FIRMWARE_PENDING,
> > > > -	GUC_FIRMWARE_SUCCESS
> > > > +enum intel_uc_fw_status {
> > > > +	UC_FIRMWARE_FAIL = -1,
> > > > +	UC_FIRMWARE_NONE = 0,
> > > > +	UC_FIRMWARE_PENDING,
> > > > +	UC_FIRMWARE_SUCCESS
> > > 
> > > Shouldn't we use INTEL_UC_ prefix here (in similar way as in GUC_ACTION)?
> > > 
> > > 
> > > >  };
> > > >  
> > > >  /*
> > > >   * This structure encapsulates all the data needed during the process
> > > >   * of fetching, caching, and loading the firmware image into the GuC.
> > > >   */
> > > > -struct intel_guc_fw {
> > > > -	const char *			guc_fw_path;
> > > > -	size_t				guc_fw_size;
> > > > -	struct drm_i915_gem_object *	guc_fw_obj;
> > > > -	enum intel_guc_fw_status	guc_fw_fetch_status;
> > > > -	enum intel_guc_fw_status	guc_fw_load_status;
> > > > -
> > > > -	uint16_t			guc_fw_major_wanted;
> > > > -	uint16_t			guc_fw_minor_wanted;
> > > > -	uint16_t			guc_fw_major_found;
> > > > -	uint16_t			guc_fw_minor_found;
> > > > -
> > > > +struct intel_uc_fw {
> > > > +	struct drm_device *uc_dev;
> > > 
> > > Why do you need this back pointer to drm_dev ?
> > > More likely all you need is pointer to dev_priv.
> > > But for dev_priv you can always use guc_to_i915().
> > 
> > It seems like a left over from the old days. I have a series that
> > refactors all things GuC that I'll push here soon, including the removal
> > of this field.
> > 
> > Since this series advertises as a simple renaming I didn't force it
> > here.
> 
> If you want to treat this as simple 'renaming' then 'adding' unused field
> is even more undesired and shall be avoided. 
> 
> > 
> > > > +	const char *uc_fw_path;
> > > > +	size_t uc_fw_size;
> > > > +	struct drm_i915_gem_object *uc_fw_obj;
> > > 
> > > All these uc_fw_ prefixes seem to be redundant as this struct is just about uc_fw.
> > > 
> > > 
> > > > +	enum intel_uc_fw_status fetch_status;
> > > > +	enum intel_uc_fw_status load_status;
> > > > +
> > > > +	uint16_t major_ver_wanted;
> > > > +	uint16_t minor_ver_wanted;
> > > > +	uint16_t major_ver_found;
> > > > +	uint16_t minor_ver_found;
> > > > +
> > > > +	uint32_t fw_type;
> > > 
> > > What is the purpose of this field ?
> > 
> > It is used in the patch 2, to set {major,minor}_ver_found depending on
> > which firmware we pass to intel_uc_fw_fetch()
> 
> Then maybe is shall be moved to patch 2 ?

Sound reasonable as that's the first usage, although is also fits here
along making te structs general. I have no strong opinion on that
though.

> 
> > 
> > > 
> > > >  	uint32_t header_size;
> > > >  	uint32_t header_offset;
> > > >  	uint32_t rsa_size;
> > > > @@ -139,7 +141,7 @@ struct intel_guc_log {
> > > >  };
> > > >  
> > > >  struct intel_guc {
> > > > -	struct intel_guc_fw guc_fw;
> > > > +	struct intel_uc_fw guc_fw;
> > > 
> > > Btw, guc_ prefix in field name here is also redundant.
> > > 
> > > 
> > > >  	struct intel_guc_log log;
> > > >  
> > > >  	/* intel_guc_recv interrupt related state */
> > > > @@ -181,7 +183,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val);
> > > >  extern void intel_guc_init(struct drm_i915_private *dev_priv);
> > > >  extern int intel_guc_setup(struct drm_i915_private *dev_priv);
> > > >  extern void intel_guc_fini(struct drm_i915_private *dev_priv);
> > > > -extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
> > > > +extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
> > > >  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
> > > >  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> > > >  
> > > > -- 
> > > > 2.7.4
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > [e-mail]
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Cheers,
> > Arek
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Srivatsa, Anusha Dec. 9, 2016, 7:34 p.m. UTC | #6

>-----Original Message-----

>From: Intel-gfx [mailto:[e-mail]] On Behalf Of

>Arkadiusz Hiler

>Sent: Friday, December 9, 2016 5:09 AM

>To: Michal Wajdeczko <[e-mail]>

>Cc: [e-mail]; Alex Dai <[e-mail]>; Peter Antoine

><[e-mail]>

>Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915/guc: Make the GuC fw loading

>helper functions general

>

>On Fri, Dec 09, 2016 at 02:06:29PM +0100, Michal Wajdeczko wrote:

>> On Fri, Dec 09, 2016 at 12:49:02PM +0100, Arkadiusz Hiler wrote:

>> > On Fri, Dec 09, 2016 at 12:28:52PM +0100, Michal Wajdeczko wrote:

>> > > On Thu, Dec 08, 2016 at 03:02:12PM -0800, anushasr wrote:

>> > > > From: Peter Antoine <[e-mail]>

>> > > >

>> > > > Rename some of the GuC fw loading code to make them more

>> > > > general. We will utilise them for HuC loading as well.

>> > > >      s/intel_guc_fw/intel_uc_fw/g

>> > > >      s/GUC_FIRMWARE/UC_FIRMWARE/g

>> > > >

>> > > > Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts

>> > > > members, such as 'guc' or 'guc_fw' either is renamed to 'uc' or

>> > > > removed for same purpose.

>> > > >

>> > > > v2: rebased on top of nightly.

>> > > >     reapplied the search/replace as upstream code as changed.

>> > > > v3: rebased again on drm-nightly.

>> > > > v4: removed G from messages in shared fw fetch function.

>> > > > v5: rebased.

>> > > > v7: rebased.

>> > > > v8: rebased.

>> > > > v9: rebased.

>> > > > v10: rebased.

>> > > > v11: rebased.

>> > > > v12: rebased on top of drm-tip

>> > > > v13: rebased.Updated dev to dev_priv in intel_guc_setup(),

>> > > > guc_fw_getch() and intel_guc_init().

>> > > >

>> > > > Signed-off-by: Anusha Srivatsa <[e-mail]>

>> > > > Signed-off-by: Alex Dai <[e-mail]>

>> > > > Signed-off-by: Peter Antoine <[e-mail]>

>> > > > ---

>> > > >  drivers/gpu/drm/i915/i915_debugfs.c        |  12 +--

>> > > >  drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-

>> > > >  drivers/gpu/drm/i915/intel_guc_loader.c    | 157 +++++++++++++++-------

>-------

>> > > >  drivers/gpu/drm/i915/intel_uc.h            |  40 ++++----

>> > > >  4 files changed, 108 insertions(+), 105 deletions(-)

>> > > >

>> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c

>> > > > b/drivers/gpu/drm/i915/i915_debugfs.c

>> > > > index a746130..0e5ef62 100644

>> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c

>> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c

>> > > > @@ -2338,7 +2338,7 @@ static int i915_llc(struct seq_file *m,

>> > > > void *data)  static int i915_guc_load_status_info(struct

>> > > > seq_file *m, void *data)  {

>> > > >  	struct drm_i915_private *dev_priv = node_to_i915(m->private);

>> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > >  	u32 tmp, i;

>> > > >

>> > > >  	if (!HAS_GUC_UCODE(dev_priv))

>> > > > @@ -2346,15 +2346,15 @@ static int

>> > > > i915_guc_load_status_info(struct seq_file *m, void *data)

>> > > >

>> > > >  	seq_printf(m, "GuC firmware status:\n");

>> > > >  	seq_printf(m, "\tpath: %s\n",

>> > > > -		guc_fw->guc_fw_path);

>> > > > +		guc_fw->uc_fw_path);

>> > > >  	seq_printf(m, "\tfetch: %s\n",

>> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));

>> > > > +		intel_uc_fw_status_repr(guc_fw->fetch_status));

>> > > >  	seq_printf(m, "\tload: %s\n",

>> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));

>> > > > +		intel_uc_fw_status_repr(guc_fw->load_status));

>> > > >  	seq_printf(m, "\tversion wanted: %d.%d\n",

>> > > > -		guc_fw->guc_fw_major_wanted, guc_fw-

>>guc_fw_minor_wanted);

>> > > > +		guc_fw->major_ver_wanted, guc_fw-

>>minor_ver_wanted);

>> > > >  	seq_printf(m, "\tversion found: %d.%d\n",

>> > > > -		guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found);

>> > > > +		guc_fw->major_ver_found, guc_fw->minor_ver_found);

>> > > >  	seq_printf(m, "\theader: offset is %d; size = %d\n",

>> > > >  		guc_fw->header_offset, guc_fw->header_size);

>> > > >  	seq_printf(m, "\tuCode: offset is %d; size = %d\n", diff --git

>> > > > a/drivers/gpu/drm/i915/i915_guc_submission.c

>> > > > b/drivers/gpu/drm/i915/i915_guc_submission.c

>> > > > index 7fa4e74..e156a4b 100644

>> > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c

>> > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c

>> > > > @@ -1493,7 +1493,7 @@ int intel_guc_suspend(struct drm_i915_private

>*dev_priv)

>> > > >  	struct i915_gem_context *ctx;

>> > > >  	u32 data[3];

>> > > >

>> > > > -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)

>> > > > +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)

>> > > >  		return 0;

>> > > >

>> > > >  	gen9_disable_guc_interrupts(dev_priv);

>> > > > @@ -1520,7 +1520,7 @@ int intel_guc_resume(struct drm_i915_private

>*dev_priv)

>> > > >  	struct i915_gem_context *ctx;

>> > > >  	u32 data[3];

>> > > >

>> > > > -	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)

>> > > > +	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)

>> > > >  		return 0;

>> > > >

>> > > >  	if (i915.guc_log_level >= 0)

>> > > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c

>> > > > b/drivers/gpu/drm/i915/intel_guc_loader.c

>> > > > index 21db697..8f04f6e 100644

>> > > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c

>> > > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c

>> > > > @@ -81,16 +81,16 @@ MODULE_FIRMWARE(I915_BXT_GUC_UCODE);

>> > > >  MODULE_FIRMWARE(I915_KBL_GUC_UCODE);

>> > > >

>> > > >  /* User-friendly representation of an enum */ -const char

>> > > > *intel_guc_fw_status_repr(enum intel_guc_fw_status status)

>> > > > +const char *intel_uc_fw_status_repr(enum intel_uc_fw_status

>> > > > +status)

>> > > >  {

>> > > >  	switch (status) {

>> > > > -	case GUC_FIRMWARE_FAIL:

>> > > > +	case UC_FIRMWARE_FAIL:

>> > > >  		return "FAIL";

>> > > > -	case GUC_FIRMWARE_NONE:

>> > > > +	case UC_FIRMWARE_NONE:

>> > > >  		return "NONE";

>> > > > -	case GUC_FIRMWARE_PENDING:

>> > > > +	case UC_FIRMWARE_PENDING:

>> > > >  		return "PENDING";

>> > > > -	case GUC_FIRMWARE_SUCCESS:

>> > > > +	case UC_FIRMWARE_SUCCESS:

>> > > >  		return "SUCCESS";

>> > > >  	default:

>> > > >  		return "UNKNOWN!";

>> > > > @@ -278,7 +278,7 @@ static inline bool guc_ucode_response(struct

>> > > > drm_i915_private *dev_priv,  static int guc_ucode_xfer_dma(struct

>drm_i915_private *dev_priv,

>> > > >  			      struct i915_vma *vma)

>> > > >  {

>> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > >  	unsigned long offset;

>> > > >  	struct sg_table *sg = vma->pages;

>> > > >  	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT]; @@ -350,17 +350,17

>> > > > @@ static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)

>> > > >   */

>> > > >  static int guc_ucode_xfer(struct drm_i915_private *dev_priv)  {

>> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > >  	struct i915_vma *vma;

>> > > >  	int ret;

>> > > >

>> > > > -	ret = i915_gem_object_set_to_gtt_domain(guc_fw->guc_fw_obj, false);

>> > > > +	ret = i915_gem_object_set_to_gtt_domain(guc_fw->uc_fw_obj,

>> > > > +false);

>> > > >  	if (ret) {

>> > > >  		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);

>> > > >  		return ret;

>> > > >  	}

>> > > >

>> > > > -	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);

>> > > > +	vma = i915_gem_object_ggtt_pin(guc_fw->uc_fw_obj, NULL, 0,

>0,

>> > > > +0);

>> > > >  	if (IS_ERR(vma)) {

>> > > >  		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));

>> > > >  		return PTR_ERR(vma);

>> > > > @@ -450,14 +450,14 @@ static int guc_hw_reset(struct

>drm_i915_private *dev_priv)

>> > > >   */

>> > > >  int intel_guc_setup(struct drm_i915_private *dev_priv)  {

>> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > > -	const char *fw_path = guc_fw->guc_fw_path;

>> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > > +	const char *fw_path = guc_fw->uc_fw_path;

>> > > >  	int retries, ret, err;

>> > > >

>> > > >  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",

>> > > >  		fw_path,

>> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),

>> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));

>> > > > +		intel_uc_fw_status_repr(guc_fw->fetch_status),

>> > > > +		intel_uc_fw_status_repr(guc_fw->load_status));

>> > > >

>> > > >  	/* Loading forbidden, or no firmware to load? */

>> > > >  	if (!i915.enable_guc_loading) { @@ -475,10 +475,10 @@ int

>> > > > intel_guc_setup(struct drm_i915_private *dev_priv)

>> > > >  	}

>> > > >

>> > > >  	/* Fetch failed, or already fetched but failed to load? */

>> > > > -	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {

>> > > > +	if (guc_fw->fetch_status != UC_FIRMWARE_SUCCESS) {

>> > > >  		err = -EIO;

>> > > >  		goto fail;

>> > > > -	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {

>> > > > +	} else if (guc_fw->load_status == UC_FIRMWARE_FAIL) {

>> > > >  		err = -ENOEXEC;

>> > > >  		goto fail;

>> > > >  	}

>> > > > @@ -486,11 +486,11 @@ int intel_guc_setup(struct drm_i915_private

>*dev_priv)

>> > > >  	guc_interrupts_release(dev_priv);

>> > > >  	gen9_reset_guc_interrupts(dev_priv);

>> > > >

>> > > > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;

>> > > > +	guc_fw->load_status = UC_FIRMWARE_PENDING;

>> > > >

>> > > >  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",

>> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),

>> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));

>> > > > +		intel_uc_fw_status_repr(guc_fw->fetch_status),

>> > > > +		intel_uc_fw_status_repr(guc_fw->load_status));

>> > > >

>> > > >  	err = i915_guc_submission_init(dev_priv);

>> > > >  	if (err)

>> > > > @@ -522,11 +522,11 @@ int intel_guc_setup(struct drm_i915_private

>*dev_priv)

>> > > >  			 "retry %d more time(s)\n", err, retries);

>> > > >  	}

>> > > >

>> > > > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;

>> > > > +	guc_fw->load_status = UC_FIRMWARE_SUCCESS;

>> > > >

>> > > >  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",

>> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),

>> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));

>> > > > +		intel_uc_fw_status_repr(guc_fw->fetch_status),

>> > > > +		intel_uc_fw_status_repr(guc_fw->load_status));

>> > > >

>> > > >  	if (i915.enable_guc_submission) {

>> > > >  		if (i915.guc_log_level >= 0)

>> > > > @@ -541,8 +541,8 @@ int intel_guc_setup(struct drm_i915_private

>*dev_priv)

>> > > >  	return 0;

>> > > >

>> > > >  fail:

>> > > > -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)

>> > > > -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;

>> > > > +	if (guc_fw->load_status == UC_FIRMWARE_PENDING)

>> > > > +		guc_fw->load_status = UC_FIRMWARE_FAIL;

>> > > >

>> > > >  	guc_interrupts_release(dev_priv);

>> > > >  	i915_guc_submission_disable(dev_priv);

>> > > > @@ -587,8 +587,8 @@ int intel_guc_setup(struct drm_i915_private

>*dev_priv)

>> > > >  	return ret;

>> > > >  }

>> > > >

>> > > > -static void guc_fw_fetch(struct drm_i915_private *dev_priv,

>> > > > -			 struct intel_guc_fw *guc_fw)

>> > > > +void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,

>> > > > +			 struct intel_uc_fw *uc_fw)

>> > > >  {

>> > > >  	struct pci_dev *pdev = dev_priv->drm.pdev;

>> > > >  	struct drm_i915_gem_object *obj; @@ -597,17 +597,17 @@ static

>> > > > void guc_fw_fetch(struct drm_i915_private *dev_priv,

>> > > >  	size_t size;

>> > > >  	int err;

>> > > >

>> > > > -	DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status

>%s\n",

>> > > > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));

>> > > > +	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch

>status %s\n",

>> > > > +		intel_uc_fw_status_repr(uc_fw->fetch_status));

>> > > >

>> > > > -	err = request_firmware(&fw, guc_fw->guc_fw_path, &pdev->dev);

>> > > > +	err = request_firmware(&fw, uc_fw->uc_fw_path, &pdev->dev);

>> > > >  	if (err)

>> > > >  		goto fail;

>> > > >  	if (!fw)

>> > > >  		goto fail;

>> > > >

>> > > > -	DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n",

>> > > > -		guc_fw->guc_fw_path, fw);

>> > > > +	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw

>%p\n",

>> > > > +		uc_fw->uc_fw_path, fw);

>> > > >

>> > > >  	/* Check the size of the blob before examining buffer contents */

>> > > >  	if (fw->size < sizeof(struct guc_css_header)) { @@ -618,36

>> > > > +618,36 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,

>> > > >  	css = (struct guc_css_header *)fw->data;

>> > > >

>> > > >  	/* Firmware bits always start from header */

>> > > > -	guc_fw->header_offset = 0;

>> > > > -	guc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -

>> > > > +	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 (guc_fw->header_size != sizeof(struct guc_css_header)) {

>> > > > +	if (uc_fw->header_size != sizeof(struct guc_css_header)) {

>> > > >  		DRM_NOTE("CSS header definition mismatch\n");

>> > > >  		goto fail;

>> > > >  	}

>> > > >

>> > > >  	/* then, uCode */

>> > > > -	guc_fw->ucode_offset = guc_fw->header_offset + guc_fw->header_size;

>> > > > -	guc_fw->ucode_size = (css->size_dw - css->header_size_dw) *

>sizeof(u32);

>> > > > +	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw-

>>header_size;

>> > > > +	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) *

>> > > > +sizeof(u32);

>> > > >

>> > > >  	/* now RSA */

>> > > >  	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {

>> > > >  		DRM_NOTE("RSA key size is bad\n");

>> > > >  		goto fail;

>> > > >  	}

>> > > > -	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;

>> > > > -	guc_fw->rsa_size = css->key_size_dw * sizeof(u32);

>> > > > +	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;

>> > > > +	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);

>> > > >

>> > > >  	/* At least, it should have header, uCode and RSA. Size of all three. */

>> > > > -	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;

>> > > > +	size = uc_fw->header_size + uc_fw->ucode_size +

>> > > > +uc_fw->rsa_size;

>> > > >  	if (fw->size < size) {

>> > > >  		DRM_NOTE("Missing firmware components\n");

>> > > >  		goto fail;

>> > > >  	}

>> > > >

>> > > >  	/* Header and uCode will be loaded to WOPCM. Size of the two. */

>> > > > -	size = guc_fw->header_size + guc_fw->ucode_size;

>> > > > +	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;

>> > > > @@ -659,21 +659,21 @@ static void guc_fw_fetch(struct

>drm_i915_private *dev_priv,

>> > > >  	 * TWO bytes each (i.e. u16), although all pointers and offsets are defined

>> > > >  	 * in terms of bytes (u8).

>> > > >  	 */

>> > > > -	guc_fw->guc_fw_major_found = css->guc_sw_version >> 16;

>> > > > -	guc_fw->guc_fw_minor_found = css->guc_sw_version & 0xFFFF;

>> > > > -

>> > > > -	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||

>> > > > -	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {

>> > > > -		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",

>> > > > -			guc_fw->guc_fw_major_found, guc_fw-

>>guc_fw_minor_found,

>> > > > -			guc_fw->guc_fw_major_wanted, guc_fw-

>>guc_fw_minor_wanted);

>> > > > +	uc_fw->major_ver_found = css->guc_sw_version >> 16;

>> > > > +	uc_fw->minor_ver_found = css->guc_sw_version & 0xFFFF;

>> > > > +

>> > > > +	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||

>> > > > +	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {

>> > > > +		DRM_NOTE("uC firmware version %d.%d, required

>%d.%d\n",

>> > > > +			uc_fw->major_ver_found, uc_fw-

>>minor_ver_found,

>> > > > +			uc_fw->major_ver_wanted, uc_fw-

>>minor_ver_wanted);

>> > > >  		err = -ENOEXEC;

>> > > >  		goto fail;

>> > > >  	}

>> > > >

>> > > >  	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum

>%d.%d)\n",

>> > > > -			guc_fw->guc_fw_major_found, guc_fw-

>>guc_fw_minor_found,

>> > > > -			guc_fw->guc_fw_major_wanted, guc_fw-

>>guc_fw_minor_wanted);

>> > > > +			uc_fw->major_ver_found, uc_fw-

>>minor_ver_found,

>> > > > +			uc_fw->major_ver_wanted, uc_fw-

>>minor_ver_wanted);

>> > > >

>> > > >  	mutex_lock(&dev_priv->drm.struct_mutex);

>> > > >  	obj = i915_gem_object_create_from_data(dev_priv, fw->data,

>> > > > fw->size); @@ -683,31 +683,31 @@ static void guc_fw_fetch(struct

>drm_i915_private *dev_priv,

>> > > >  		goto fail;

>> > > >  	}

>> > > >

>> > > > -	guc_fw->guc_fw_obj = obj;

>> > > > -	guc_fw->guc_fw_size = fw->size;

>> > > > +	uc_fw->uc_fw_obj = obj;

>> > > > +	uc_fw->uc_fw_size = fw->size;

>> > > >

>> > > > -	DRM_DEBUG_DRIVER("GuC fw fetch status SUCCESS, obj %p\n",

>> > > > -			guc_fw->guc_fw_obj);

>> > > > +	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",

>> > > > +			uc_fw->uc_fw_obj);

>> > > >

>> > > >  	release_firmware(fw);

>> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_SUCCESS;

>> > > > +	uc_fw->fetch_status = UC_FIRMWARE_SUCCESS;

>> > > >  	return;

>> > > >

>> > > >  fail:

>> > > > -	DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",

>> > > > -		 guc_fw->guc_fw_path, err);

>> > > > -	DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj

>%p\n",

>> > > > -		err, fw, guc_fw->guc_fw_obj);

>> > > > +	DRM_WARN("Failed to fetch valid uC firmware from %s (error

>%d)\n",

>> > > > +		 uc_fw->uc_fw_path, err);

>> > > > +	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p,

>obj %p\n",

>> > > > +		err, fw, uc_fw->uc_fw_obj);

>> > > >

>> > > >  	mutex_lock(&dev_priv->drm.struct_mutex);

>> > > > -	obj = guc_fw->guc_fw_obj;

>> > > > +	obj = uc_fw->uc_fw_obj;

>> > > >  	if (obj)

>> > > >  		i915_gem_object_put(obj);

>> > > > -	guc_fw->guc_fw_obj = NULL;

>> > > > +	uc_fw->uc_fw_obj = NULL;

>> > > >  	mutex_unlock(&dev_priv->drm.struct_mutex);

>> > > >

>> > > >  	release_firmware(fw);		/* OK even if fw is NULL */

>> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;

>> > > > +	uc_fw->fetch_status = UC_FIRMWARE_FAIL;

>> > > >  }

>> > > >

>> > > >  /**

>> > > > @@ -721,7 +721,7 @@ static void guc_fw_fetch(struct drm_i915_private

>*dev_priv,

>> > > >   */

>> > > >  void intel_guc_init(struct drm_i915_private *dev_priv)  {

>> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > >  	const char *fw_path;

>> > > >

>> > > >  	if (!HAS_GUC(dev_priv)) {

>> > > > @@ -739,23 +739,24 @@ void intel_guc_init(struct drm_i915_private

>*dev_priv)

>> > > >  		fw_path = NULL;

>> > > >  	} else if (IS_SKYLAKE(dev_priv)) {

>> > > >  		fw_path = I915_SKL_GUC_UCODE;

>> > > > -		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;

>> > > > -		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;

>> > > > +		guc_fw->major_ver_wanted = SKL_FW_MAJOR;

>> > > > +		guc_fw->minor_ver_wanted = SKL_FW_MINOR;

>> > > >  	} else if (IS_BROXTON(dev_priv)) {

>> > > >  		fw_path = I915_BXT_GUC_UCODE;

>> > > > -		guc_fw->guc_fw_major_wanted = BXT_FW_MAJOR;

>> > > > -		guc_fw->guc_fw_minor_wanted = BXT_FW_MINOR;

>> > > > +		guc_fw->major_ver_wanted = BXT_FW_MAJOR;

>> > > > +		guc_fw->minor_ver_wanted = BXT_FW_MINOR;

>> > > >  	} else if (IS_KABYLAKE(dev_priv)) {

>> > > >  		fw_path = I915_KBL_GUC_UCODE;

>> > > > -		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;

>> > > > -		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;

>> > > > +		guc_fw->major_ver_wanted = KBL_FW_MAJOR;

>> > > > +		guc_fw->minor_ver_wanted = KBL_FW_MINOR;

>> > > >  	} else {

>> > > >  		fw_path = "";	/* unknown device */

>> > > >  	}

>> > > >

>> > > > -	guc_fw->guc_fw_path = fw_path;

>> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;

>> > > > -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;

>> > > > +	guc_fw->uc_dev = &dev_priv->drm;

>> > > > +	guc_fw->uc_fw_path = fw_path;

>> > > > +	guc_fw->fetch_status = UC_FIRMWARE_NONE;

>> > > > +	guc_fw->load_status = UC_FIRMWARE_NONE;

>> > > >

>> > > >  	/* Early (and silent) return if GuC loading is disabled */

>> > > >  	if (!i915.enable_guc_loading)

>> > > > @@ -765,9 +766,9 @@ void intel_guc_init(struct drm_i915_private

>*dev_priv)

>> > > >  	if (*fw_path == '\0')

>> > > >  		return;

>> > > >

>> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;

>> > > > +	guc_fw->fetch_status = UC_FIRMWARE_PENDING;

>> > > >  	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);

>> > > > -	guc_fw_fetch(dev_priv, guc_fw);

>> > > > +	intel_uc_fw_fetch(dev_priv, guc_fw);

>> > > >  	/* status must now be FAIL or SUCCESS */  }

>> > > >

>> > > > @@ -777,17 +778,17 @@ void intel_guc_init(struct drm_i915_private

>*dev_priv)

>> > > >   */

>> > > >  void intel_guc_fini(struct drm_i915_private *dev_priv)  {

>> > > > -	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > > +	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;

>> > > >

>> > > >  	mutex_lock(&dev_priv->drm.struct_mutex);

>> > > >  	guc_interrupts_release(dev_priv);

>> > > >  	i915_guc_submission_disable(dev_priv);

>> > > >  	i915_guc_submission_fini(dev_priv);

>> > > >

>> > > > -	if (guc_fw->guc_fw_obj)

>> > > > -		i915_gem_object_put(guc_fw->guc_fw_obj);

>> > > > -	guc_fw->guc_fw_obj = NULL;

>> > > > +	if (guc_fw->uc_fw_obj)

>> > > > +		i915_gem_object_put(guc_fw->uc_fw_obj);

>> > > > +	guc_fw->uc_fw_obj = NULL;

>> > > >  	mutex_unlock(&dev_priv->drm.struct_mutex);

>> > > >

>> > > > -	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;

>> > > > +	guc_fw->fetch_status = UC_FIRMWARE_NONE;

>> > > >  }

>> > > > diff --git a/drivers/gpu/drm/i915/intel_uc.h

>> > > > b/drivers/gpu/drm/i915/intel_uc.h index 11f5608..f9f598d 100644

>> > > > --- a/drivers/gpu/drm/i915/intel_uc.h

>> > > > +++ b/drivers/gpu/drm/i915/intel_uc.h

>> > > > @@ -91,29 +91,31 @@ struct i915_guc_client {

>> > > >  	uint64_t submissions[I915_NUM_ENGINES];  };

>> > > >

>> > > > -enum intel_guc_fw_status {

>> > > > -	GUC_FIRMWARE_FAIL = -1,

>> > > > -	GUC_FIRMWARE_NONE = 0,

>> > > > -	GUC_FIRMWARE_PENDING,

>> > > > -	GUC_FIRMWARE_SUCCESS

>> > > > +enum intel_uc_fw_status {

>> > > > +	UC_FIRMWARE_FAIL = -1,

>> > > > +	UC_FIRMWARE_NONE = 0,

>> > > > +	UC_FIRMWARE_PENDING,

>> > > > +	UC_FIRMWARE_SUCCESS

>> > >

>> > > Shouldn't we use INTEL_UC_ prefix here (in similar way as in GUC_ACTION)?

Will do. Thanks for your review.
-Anusha
>> > >

>> > > >  };

>> > > >

>> > > >  /*

>> > > >   * This structure encapsulates all the data needed during the process

>> > > >   * of fetching, caching, and loading the firmware image into the GuC.

>> > > >   */

>> > > > -struct intel_guc_fw {

>> > > > -	const char *			guc_fw_path;

>> > > > -	size_t				guc_fw_size;

>> > > > -	struct drm_i915_gem_object *	guc_fw_obj;

>> > > > -	enum intel_guc_fw_status	guc_fw_fetch_status;

>> > > > -	enum intel_guc_fw_status	guc_fw_load_status;

>> > > > -

>> > > > -	uint16_t			guc_fw_major_wanted;

>> > > > -	uint16_t			guc_fw_minor_wanted;

>> > > > -	uint16_t			guc_fw_major_found;

>> > > > -	uint16_t			guc_fw_minor_found;

>> > > > -

>> > > > +struct intel_uc_fw {

>> > > > +	struct drm_device *uc_dev;

>> > >

>> > > Why do you need this back pointer to drm_dev ?

>> > > More likely all you need is pointer to dev_priv.

>> > > But for dev_priv you can always use guc_to_i915().

>> >

>> > It seems like a left over from the old days. I have a series that

>> > refactors all things GuC that I'll push here soon, including the

>> > removal of this field.

>> >

>> > Since this series advertises as a simple renaming I didn't force it

>> > here.

>>

>> If you want to treat this as simple 'renaming' then 'adding' unused

>> field is even more undesired and shall be avoided.


Michal, I don’t think I follow you completely here. Do you want me to rename it or wait for Arek's series?
>> >

>> > > > +	const char *uc_fw_path;

>> > > > +	size_t uc_fw_size;

>> > > > +	struct drm_i915_gem_object *uc_fw_obj;

>> > >

>> > > All these uc_fw_ prefixes seem to be redundant as this struct is just about

>uc_fw.

>> > >

>> > >

>> > > > +	enum intel_uc_fw_status fetch_status;

>> > > > +	enum intel_uc_fw_status load_status;

>> > > > +

>> > > > +	uint16_t major_ver_wanted;

>> > > > +	uint16_t minor_ver_wanted;

>> > > > +	uint16_t major_ver_found;

>> > > > +	uint16_t minor_ver_found;

>> > > > +

>> > > > +	uint32_t fw_type;

>> > >

>> > > What is the purpose of this field ?

>> >

>> > It is used in the patch 2, to set {major,minor}_ver_found depending

>> > on which firmware we pass to intel_uc_fw_fetch()

>>

>> Then maybe is shall be moved to patch 2 ?

>

>Sound reasonable as that's the first usage, although is also fits here along making

>te structs general. I have no strong opinion on that though.

>

Will put it in patch 2 then. Thanks.

>> >

>> > >

>> > > >  	uint32_t header_size;

>> > > >  	uint32_t header_offset;

>> > > >  	uint32_t rsa_size;

>> > > > @@ -139,7 +141,7 @@ struct intel_guc_log {  };

>> > > >

>> > > >  struct intel_guc {

>> > > > -	struct intel_guc_fw guc_fw;

>> > > > +	struct intel_uc_fw guc_fw;

>> > >

>> > > Btw, guc_ prefix in field name here is also redundant.


>> > >

>> > > >  	struct intel_guc_log log;

>> > > >

>> > > >  	/* intel_guc_recv interrupt related state */ @@ -181,7 +183,7

>> > > > @@ int intel_guc_log_control(struct intel_guc *guc, u32

>> > > > control_val);  extern void intel_guc_init(struct

>> > > > drm_i915_private *dev_priv);  extern int intel_guc_setup(struct

>> > > > drm_i915_private *dev_priv);  extern void intel_guc_fini(struct

>> > > > drm_i915_private *dev_priv); -extern const char

>> > > > *intel_guc_fw_status_repr(enum intel_guc_fw_status status);

>> > > > +extern const char *intel_uc_fw_status_repr(enum

>> > > > +intel_uc_fw_status status);

>> > > >  extern int intel_guc_suspend(struct drm_i915_private

>> > > > *dev_priv);  extern int intel_guc_resume(struct drm_i915_private

>> > > > *dev_priv);

>> > > >

>> > > > --

>> > > > 2.7.4

>> > > >

>> > > > _______________________________________________

>> > > > Intel-gfx mailing list

>> > > > [e-mail]

>> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

>> > > _______________________________________________

>> > > Intel-gfx mailing list

>> > > [e-mail]

>> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

>> >

>> > --

>> > Cheers,

>> > Arek

>> > _______________________________________________

>> > Intel-gfx mailing list

>> > [e-mail]

>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

>

>--

>Cheers,

>Arek

>_______________________________________________

>Intel-gfx mailing list

>[e-mail]

>https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a746130..0e5ef62 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2338,7 +2338,7 @@  static int i915_llc(struct seq_file *m, void *data)
 static int i915_guc_load_status_info(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
-	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	u32 tmp, i;
 
 	if (!HAS_GUC_UCODE(dev_priv))
@@ -2346,15 +2346,15 @@  static int i915_guc_load_status_info(struct seq_file *m, void *data)
 
 	seq_printf(m, "GuC firmware status:\n");
 	seq_printf(m, "\tpath: %s\n",
-		guc_fw->guc_fw_path);
+		guc_fw->uc_fw_path);
 	seq_printf(m, "\tfetch: %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
+		intel_uc_fw_status_repr(guc_fw->fetch_status));
 	seq_printf(m, "\tload: %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
+		intel_uc_fw_status_repr(guc_fw->load_status));
 	seq_printf(m, "\tversion wanted: %d.%d\n",
-		guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
+		guc_fw->major_ver_wanted, guc_fw->minor_ver_wanted);
 	seq_printf(m, "\tversion found: %d.%d\n",
-		guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found);
+		guc_fw->major_ver_found, guc_fw->minor_ver_found);
 	seq_printf(m, "\theader: offset is %d; size = %d\n",
 		guc_fw->header_offset, guc_fw->header_size);
 	seq_printf(m, "\tuCode: offset is %d; size = %d\n",
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 7fa4e74..e156a4b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1493,7 +1493,7 @@  int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	struct i915_gem_context *ctx;
 	u32 data[3];
 
-	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
+	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
 		return 0;
 
 	gen9_disable_guc_interrupts(dev_priv);
@@ -1520,7 +1520,7 @@  int intel_guc_resume(struct drm_i915_private *dev_priv)
 	struct i915_gem_context *ctx;
 	u32 data[3];
 
-	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
+	if (guc->guc_fw.load_status != UC_FIRMWARE_SUCCESS)
 		return 0;
 
 	if (i915.guc_log_level >= 0)
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 21db697..8f04f6e 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -81,16 +81,16 @@  MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
 MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
 
 /* User-friendly representation of an enum */
-const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
+const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 {
 	switch (status) {
-	case GUC_FIRMWARE_FAIL:
+	case UC_FIRMWARE_FAIL:
 		return "FAIL";
-	case GUC_FIRMWARE_NONE:
+	case UC_FIRMWARE_NONE:
 		return "NONE";
-	case GUC_FIRMWARE_PENDING:
+	case UC_FIRMWARE_PENDING:
 		return "PENDING";
-	case GUC_FIRMWARE_SUCCESS:
+	case UC_FIRMWARE_SUCCESS:
 		return "SUCCESS";
 	default:
 		return "UNKNOWN!";
@@ -278,7 +278,7 @@  static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
 static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
 			      struct i915_vma *vma)
 {
-	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	unsigned long offset;
 	struct sg_table *sg = vma->pages;
 	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
@@ -350,17 +350,17 @@  static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
  */
 static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 {
-	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	struct i915_vma *vma;
 	int ret;
 
-	ret = i915_gem_object_set_to_gtt_domain(guc_fw->guc_fw_obj, false);
+	ret = i915_gem_object_set_to_gtt_domain(guc_fw->uc_fw_obj, false);
 	if (ret) {
 		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
 		return ret;
 	}
 
-	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);
+	vma = i915_gem_object_ggtt_pin(guc_fw->uc_fw_obj, NULL, 0, 0, 0);
 	if (IS_ERR(vma)) {
 		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
 		return PTR_ERR(vma);
@@ -450,14 +450,14 @@  static int guc_hw_reset(struct drm_i915_private *dev_priv)
  */
 int intel_guc_setup(struct drm_i915_private *dev_priv)
 {
-	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
-	const char *fw_path = guc_fw->guc_fw_path;
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
+	const char *fw_path = guc_fw->uc_fw_path;
 	int retries, ret, err;
 
 	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
 		fw_path,
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
-		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
+		intel_uc_fw_status_repr(guc_fw->fetch_status),
+		intel_uc_fw_status_repr(guc_fw->load_status));
 
 	/* Loading forbidden, or no firmware to load? */
 	if (!i915.enable_guc_loading) {
@@ -475,10 +475,10 @@  int intel_guc_setup(struct drm_i915_private *dev_priv)
 	}
 
 	/* Fetch failed, or already fetched but failed to load? */
-	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
+	if (guc_fw->fetch_status != UC_FIRMWARE_SUCCESS) {
 		err = -EIO;
 		goto fail;
-	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
+	} else if (guc_fw->load_status == UC_FIRMWARE_FAIL) {
 		err = -ENOEXEC;
 		goto fail;
 	}
@@ -486,11 +486,11 @@  int intel_guc_setup(struct drm_i915_private *dev_priv)
 	guc_interrupts_release(dev_priv);
 	gen9_reset_guc_interrupts(dev_priv);
 
-	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
+	guc_fw->load_status = UC_FIRMWARE_PENDING;
 
 	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
-		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
+		intel_uc_fw_status_repr(guc_fw->fetch_status),
+		intel_uc_fw_status_repr(guc_fw->load_status));
 
 	err = i915_guc_submission_init(dev_priv);
 	if (err)
@@ -522,11 +522,11 @@  int intel_guc_setup(struct drm_i915_private *dev_priv)
 			 "retry %d more time(s)\n", err, retries);
 	}
 
-	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
+	guc_fw->load_status = UC_FIRMWARE_SUCCESS;
 
 	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
-		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
+		intel_uc_fw_status_repr(guc_fw->fetch_status),
+		intel_uc_fw_status_repr(guc_fw->load_status));
 
 	if (i915.enable_guc_submission) {
 		if (i915.guc_log_level >= 0)
@@ -541,8 +541,8 @@  int intel_guc_setup(struct drm_i915_private *dev_priv)
 	return 0;
 
 fail:
-	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
-		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
+	if (guc_fw->load_status == UC_FIRMWARE_PENDING)
+		guc_fw->load_status = UC_FIRMWARE_FAIL;
 
 	guc_interrupts_release(dev_priv);
 	i915_guc_submission_disable(dev_priv);
@@ -587,8 +587,8 @@  int intel_guc_setup(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static void guc_fw_fetch(struct drm_i915_private *dev_priv,
-			 struct intel_guc_fw *guc_fw)
+void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
+			 struct intel_uc_fw *uc_fw)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct drm_i915_gem_object *obj;
@@ -597,17 +597,17 @@  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
 	size_t size;
 	int err;
 
-	DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
+	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
+		intel_uc_fw_status_repr(uc_fw->fetch_status));
 
-	err = request_firmware(&fw, guc_fw->guc_fw_path, &pdev->dev);
+	err = request_firmware(&fw, uc_fw->uc_fw_path, &pdev->dev);
 	if (err)
 		goto fail;
 	if (!fw)
 		goto fail;
 
-	DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n",
-		guc_fw->guc_fw_path, fw);
+	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
+		uc_fw->uc_fw_path, fw);
 
 	/* Check the size of the blob before examining buffer contents */
 	if (fw->size < sizeof(struct guc_css_header)) {
@@ -618,36 +618,36 @@  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
 	css = (struct guc_css_header *)fw->data;
 
 	/* Firmware bits always start from header */
-	guc_fw->header_offset = 0;
-	guc_fw->header_size = (css->header_size_dw - css->modulus_size_dw -
+	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 (guc_fw->header_size != sizeof(struct guc_css_header)) {
+	if (uc_fw->header_size != sizeof(struct guc_css_header)) {
 		DRM_NOTE("CSS header definition mismatch\n");
 		goto fail;
 	}
 
 	/* then, uCode */
-	guc_fw->ucode_offset = guc_fw->header_offset + guc_fw->header_size;
-	guc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
+	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
+	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
 
 	/* now RSA */
 	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
 		DRM_NOTE("RSA key size is bad\n");
 		goto fail;
 	}
-	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
-	guc_fw->rsa_size = css->key_size_dw * sizeof(u32);
+	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
+	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
 
 	/* At least, it should have header, uCode and RSA. Size of all three. */
-	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
+	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
 	if (fw->size < size) {
 		DRM_NOTE("Missing firmware components\n");
 		goto fail;
 	}
 
 	/* Header and uCode will be loaded to WOPCM. Size of the two. */
-	size = guc_fw->header_size + guc_fw->ucode_size;
+	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;
@@ -659,21 +659,21 @@  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
 	 * TWO bytes each (i.e. u16), although all pointers and offsets are defined
 	 * in terms of bytes (u8).
 	 */
-	guc_fw->guc_fw_major_found = css->guc_sw_version >> 16;
-	guc_fw->guc_fw_minor_found = css->guc_sw_version & 0xFFFF;
-
-	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
-	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
-		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
-			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
-			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
+	uc_fw->major_ver_found = css->guc_sw_version >> 16;
+	uc_fw->minor_ver_found = css->guc_sw_version & 0xFFFF;
+
+	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
+	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
+		DRM_NOTE("uC firmware version %d.%d, required %d.%d\n",
+			uc_fw->major_ver_found, uc_fw->minor_ver_found,
+			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
 		err = -ENOEXEC;
 		goto fail;
 	}
 
 	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
-			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
-			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
+			uc_fw->major_ver_found, uc_fw->minor_ver_found,
+			uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
@@ -683,31 +683,31 @@  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
 		goto fail;
 	}
 
-	guc_fw->guc_fw_obj = obj;
-	guc_fw->guc_fw_size = fw->size;
+	uc_fw->uc_fw_obj = obj;
+	uc_fw->uc_fw_size = fw->size;
 
-	DRM_DEBUG_DRIVER("GuC fw fetch status SUCCESS, obj %p\n",
-			guc_fw->guc_fw_obj);
+	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
+			uc_fw->uc_fw_obj);
 
 	release_firmware(fw);
-	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_SUCCESS;
+	uc_fw->fetch_status = UC_FIRMWARE_SUCCESS;
 	return;
 
 fail:
-	DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",
-		 guc_fw->guc_fw_path, err);
-	DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
-		err, fw, guc_fw->guc_fw_obj);
+	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
+		 uc_fw->uc_fw_path, err);
+	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
+		err, fw, uc_fw->uc_fw_obj);
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	obj = guc_fw->guc_fw_obj;
+	obj = uc_fw->uc_fw_obj;
 	if (obj)
 		i915_gem_object_put(obj);
-	guc_fw->guc_fw_obj = NULL;
+	uc_fw->uc_fw_obj = NULL;
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
 	release_firmware(fw);		/* OK even if fw is NULL */
-	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
+	uc_fw->fetch_status = UC_FIRMWARE_FAIL;
 }
 
 /**
@@ -721,7 +721,7 @@  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
  */
 void intel_guc_init(struct drm_i915_private *dev_priv)
 {
-	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
 	if (!HAS_GUC(dev_priv)) {
@@ -739,23 +739,24 @@  void intel_guc_init(struct drm_i915_private *dev_priv)
 		fw_path = NULL;
 	} else if (IS_SKYLAKE(dev_priv)) {
 		fw_path = I915_SKL_GUC_UCODE;
-		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
-		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
+		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
+		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
 	} else if (IS_BROXTON(dev_priv)) {
 		fw_path = I915_BXT_GUC_UCODE;
-		guc_fw->guc_fw_major_wanted = BXT_FW_MAJOR;
-		guc_fw->guc_fw_minor_wanted = BXT_FW_MINOR;
+		guc_fw->major_ver_wanted = BXT_FW_MAJOR;
+		guc_fw->minor_ver_wanted = BXT_FW_MINOR;
 	} else if (IS_KABYLAKE(dev_priv)) {
 		fw_path = I915_KBL_GUC_UCODE;
-		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
-		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
+		guc_fw->major_ver_wanted = KBL_FW_MAJOR;
+		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
 	} else {
 		fw_path = "";	/* unknown device */
 	}
 
-	guc_fw->guc_fw_path = fw_path;
-	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
-	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
+	guc_fw->uc_dev = &dev_priv->drm;
+	guc_fw->uc_fw_path = fw_path;
+	guc_fw->fetch_status = UC_FIRMWARE_NONE;
+	guc_fw->load_status = UC_FIRMWARE_NONE;
 
 	/* Early (and silent) return if GuC loading is disabled */
 	if (!i915.enable_guc_loading)
@@ -765,9 +766,9 @@  void intel_guc_init(struct drm_i915_private *dev_priv)
 	if (*fw_path == '\0')
 		return;
 
-	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
+	guc_fw->fetch_status = UC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
-	guc_fw_fetch(dev_priv, guc_fw);
+	intel_uc_fw_fetch(dev_priv, guc_fw);
 	/* status must now be FAIL or SUCCESS */
 }
 
@@ -777,17 +778,17 @@  void intel_guc_init(struct drm_i915_private *dev_priv)
  */
 void intel_guc_fini(struct drm_i915_private *dev_priv)
 {
-	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	guc_interrupts_release(dev_priv);
 	i915_guc_submission_disable(dev_priv);
 	i915_guc_submission_fini(dev_priv);
 
-	if (guc_fw->guc_fw_obj)
-		i915_gem_object_put(guc_fw->guc_fw_obj);
-	guc_fw->guc_fw_obj = NULL;
+	if (guc_fw->uc_fw_obj)
+		i915_gem_object_put(guc_fw->uc_fw_obj);
+	guc_fw->uc_fw_obj = NULL;
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
-	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
+	guc_fw->fetch_status = UC_FIRMWARE_NONE;
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 11f5608..f9f598d 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -91,29 +91,31 @@  struct i915_guc_client {
 	uint64_t submissions[I915_NUM_ENGINES];
 };
 
-enum intel_guc_fw_status {
-	GUC_FIRMWARE_FAIL = -1,
-	GUC_FIRMWARE_NONE = 0,
-	GUC_FIRMWARE_PENDING,
-	GUC_FIRMWARE_SUCCESS
+enum intel_uc_fw_status {
+	UC_FIRMWARE_FAIL = -1,
+	UC_FIRMWARE_NONE = 0,
+	UC_FIRMWARE_PENDING,
+	UC_FIRMWARE_SUCCESS
 };
 
 /*
  * This structure encapsulates all the data needed during the process
  * of fetching, caching, and loading the firmware image into the GuC.
  */
-struct intel_guc_fw {
-	const char *			guc_fw_path;
-	size_t				guc_fw_size;
-	struct drm_i915_gem_object *	guc_fw_obj;
-	enum intel_guc_fw_status	guc_fw_fetch_status;
-	enum intel_guc_fw_status	guc_fw_load_status;
-
-	uint16_t			guc_fw_major_wanted;
-	uint16_t			guc_fw_minor_wanted;
-	uint16_t			guc_fw_major_found;
-	uint16_t			guc_fw_minor_found;
-
+struct intel_uc_fw {
+	struct drm_device *uc_dev;
+	const char *uc_fw_path;
+	size_t uc_fw_size;
+	struct drm_i915_gem_object *uc_fw_obj;
+	enum intel_uc_fw_status fetch_status;
+	enum intel_uc_fw_status load_status;
+
+	uint16_t major_ver_wanted;
+	uint16_t minor_ver_wanted;
+	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;
@@ -139,7 +141,7 @@  struct intel_guc_log {
 };
 
 struct intel_guc {
-	struct intel_guc_fw guc_fw;
+	struct intel_uc_fw guc_fw;
 	struct intel_guc_log log;
 
 	/* intel_guc_recv interrupt related state */
@@ -181,7 +183,7 @@  int intel_guc_log_control(struct intel_guc *guc, u32 control_val);
 extern void intel_guc_init(struct drm_i915_private *dev_priv);
 extern int intel_guc_setup(struct drm_i915_private *dev_priv);
 extern void intel_guc_fini(struct drm_i915_private *dev_priv);
-extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
+extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
 extern int intel_guc_resume(struct drm_i915_private *dev_priv);