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

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

 




>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of
>Arkadiusz Hiler
>Sent: Friday, December 9, 2016 5:09 AM
>To: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Alex Dai <yu.dai@xxxxxxxxx>; Peter Antoine
><peter.antoine@xxxxxxxxx>
>Subject: Re:  [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 <peter.antoine@xxxxxxxxx>
>> > > >
>> > > > 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 <anusha.srivatsa@xxxxxxxxx>
>> > > > Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx>
>> > > > Signed-off-by: Peter Antoine <peter.antoine@xxxxxxxxx>
>> > > > ---
>> > > >  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
>> > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> > > _______________________________________________
>> > > Intel-gfx mailing list
>> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Cheers,
>> > Arek
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Cheers,
>Arek
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




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