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