| Message ID | 1524070894-8120-1-git-send-email-yaodong.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 18 Apr 2018 19:01:31 +0200, Jackie Li <yaodong.li@intel.com> wrote: > After enabled the WOPCM write-once registers locking status checking, > reloading of the i915 module will fail with modparam enable_guc set to 3 > (enable GuC and HuC firmware loading) if the module was originally loaded > with enable_guc set to 1 (only enable GuC firmware loading). This is > because WOPCM registers were updated and locked without considering the > HuC > FW size. Since we need both GuC and HuC FW sizes to determine the final > layout of WOPCM, we should always calculate the WOPCM layout based on the > actual sizes of the GuC and HuC firmware available for a specific > platform > if we need continue to support enable/disable HuC FW loading dynamically > with enable_guc modparam. > > This patch splits uC firmware fetching into two stages. First stage is to > fetch the firmware image and verify the firmware header. uC firmware will > be marked as verified and this will make FW info available for following > WOPCM layout calculation. The second stage is to create a GEM object and > copy the FW data into the created GEM object which will only be available > when GuC/HuC loading is enabled by enable_guc modparam. This will > guarantee > that the WOPCM layout will be always be calculated correctly without > making > any assumptions to the GuC and HuC firmware sizes. > > v3: > - Rebase > > v4: > - Renamed the new parameter add to intel_uc_fw_fetch (Michal) > > Signed-off-by: Jackie Li <yaodong.li@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: John Spotswood <john.a.spotswood@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Reviewed-by: John Spotswood <john.a.spotswood@intel.com> > --- > drivers/gpu/drm/i915/intel_uc.c | 14 ++++---------- > drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++----------- > drivers/gpu/drm/i915/intel_uc_fw.h | 7 +++++-- > 3 files changed, 29 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index 1cffaf7..73b8f6c 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private > *i915) > sanitize_options_early(i915); > - if (USES_GUC(i915)) > - intel_uc_fw_fetch(i915, &guc->fw); > - > - if (USES_HUC(i915)) > - intel_uc_fw_fetch(i915, &huc->fw); > + intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915)); > + intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915)); Again: I'm don't think that unconditional fetch of HuC fw is a right choice. We should look for other options how to support enable_guc 1-->3 scenario. > } > void intel_uc_cleanup_early(struct drm_i915_private *i915) > @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct drm_i915_private > *i915) > struct intel_guc *guc = &i915->guc; > struct intel_huc *huc = &i915->huc; > - if (USES_HUC(i915)) > - intel_uc_fw_fini(&huc->fw); > - > - if (USES_GUC(i915)) > - intel_uc_fw_fini(&guc->fw); > + intel_uc_fw_fini(&huc->fw); > + intel_uc_fw_fini(&guc->fw); > guc_free_load_err_log(guc); > } > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c > b/drivers/gpu/drm/i915/intel_uc_fw.c > index 6e8e0b5..c1fed06 100644 > --- a/drivers/gpu/drm/i915/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/intel_uc_fw.c > @@ -33,11 +33,13 @@ > * > * @dev_priv: device private > * @uc_fw: uC firmware > + * @fetch: whether fetch uC firmware into GEM object or not > * > - * Fetch uC firmware into GEM obj. > + * Fetch and verify uC firmware and copy firmware data into GEM object > if > + * @fetch is true. > */ > void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, > - struct intel_uc_fw *uc_fw) > + struct intel_uc_fw *uc_fw, bool fetch) > { > struct pci_dev *pdev = dev_priv->drm.pdev; > struct drm_i915_gem_object *obj; > @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private > *dev_priv, > goto fail; > } > - obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size); > - if (IS_ERR(obj)) { > - err = PTR_ERR(obj); > - DRM_DEBUG_DRIVER("%s fw object_create err=%d\n", > - intel_uc_fw_type_repr(uc_fw->type), err); > - goto fail; > + uc_fw->size = fw->size; > + uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED; btw, I'm not sure that this new state is needed at all, as you don't plan to use that fw obj if you only loaded fw blob to read header... > + > + if (fetch) { > + obj = i915_gem_object_create_from_data(dev_priv, fw->data, > + fw->size); > + if (IS_ERR(obj)) { > + err = PTR_ERR(obj); > + DRM_DEBUG_DRIVER("%s fw object_create err=%d\n", > + intel_uc_fw_type_repr(uc_fw->type), > + err); > + goto fail; > + } > + > + uc_fw->obj = obj; > + uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS; > } > - uc_fw->obj = obj; > - uc_fw->size = fw->size; > - uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS; > DRM_DEBUG_DRIVER("%s fw fetch %s\n", > intel_uc_fw_type_repr(uc_fw->type), > intel_uc_fw_status_repr(uc_fw->fetch_status)); > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h > b/drivers/gpu/drm/i915/intel_uc_fw.h > index dc33b12..a1308c0 100644 > --- a/drivers/gpu/drm/i915/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/intel_uc_fw.h > @@ -36,6 +36,7 @@ enum intel_uc_fw_status { > INTEL_UC_FIRMWARE_FAIL = -1, > INTEL_UC_FIRMWARE_NONE = 0, > INTEL_UC_FIRMWARE_PENDING, > + INTEL_UC_FIRMWARE_VERIFIED, > INTEL_UC_FIRMWARE_SUCCESS > }; > @@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum > intel_uc_fw_status status) > return "NONE"; > case INTEL_UC_FIRMWARE_PENDING: > return "PENDING"; > + case INTEL_UC_FIRMWARE_VERIFIED: > + return "VERIFIED"; > case INTEL_UC_FIRMWARE_SUCCESS: > return "SUCCESS"; > } > @@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct > intel_uc_fw *uc_fw) > */ > static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw) > { > - if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) > + if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED) > return 0; > return uc_fw->header_size + uc_fw->ucode_size; > } > void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, > - struct intel_uc_fw *uc_fw); > + struct intel_uc_fw *uc_fw, bool fetch); > int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, > int (*xfer)(struct intel_uc_fw *uc_fw, > struct i915_vma *vma));
On 04/19/2018 08:45 AM, Michal Wajdeczko wrote: > On Wed, 18 Apr 2018 19:01:31 +0200, Jackie Li <yaodong.li@intel.com> > wrote: > >> After enabled the WOPCM write-once registers locking status checking, >> reloading of the i915 module will fail with modparam enable_guc set to 3 >> (enable GuC and HuC firmware loading) if the module was originally >> loaded >> with enable_guc set to 1 (only enable GuC firmware loading). This is >> because WOPCM registers were updated and locked without considering >> the HuC >> FW size. Since we need both GuC and HuC FW sizes to determine the final >> layout of WOPCM, we should always calculate the WOPCM layout based on >> the >> actual sizes of the GuC and HuC firmware available for a specific >> platform >> if we need continue to support enable/disable HuC FW loading dynamically >> with enable_guc modparam. >> >> This patch splits uC firmware fetching into two stages. First stage >> is to >> fetch the firmware image and verify the firmware header. uC firmware >> will >> be marked as verified and this will make FW info available for following >> WOPCM layout calculation. The second stage is to create a GEM object and >> copy the FW data into the created GEM object which will only be >> available >> when GuC/HuC loading is enabled by enable_guc modparam. This will >> guarantee >> that the WOPCM layout will be always be calculated correctly without >> making >> any assumptions to the GuC and HuC firmware sizes. >> >> v3: >> - Rebase >> >> v4: >> - Renamed the new parameter add to intel_uc_fw_fetch (Michal) >> >> Signed-off-by: Jackie Li <yaodong.li@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Michal Winiarski <michal.winiarski@intel.com> >> Cc: John Spotswood <john.a.spotswood@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Reviewed-by: John Spotswood <john.a.spotswood@intel.com> >> --- >> drivers/gpu/drm/i915/intel_uc.c | 14 ++++---------- >> drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++----------- >> drivers/gpu/drm/i915/intel_uc_fw.h | 7 +++++-- >> 3 files changed, 29 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index 1cffaf7..73b8f6c 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private >> *i915) >> sanitize_options_early(i915); >> - if (USES_GUC(i915)) >> - intel_uc_fw_fetch(i915, &guc->fw); >> - >> - if (USES_HUC(i915)) >> - intel_uc_fw_fetch(i915, &huc->fw); >> + intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915)); >> + intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915)); > > Again: I'm don't think that unconditional fetch of HuC fw is a right > choice. > We should look for other options how to support enable_guc 1-->3 > scenario. > This is the real fix I can think of to support such a scenario. I think the performance impact here is minimal since it's only one time operation. I will think about the use case of HAS_HUC = 1 and only enable guc submission. But first I suggest we need to define the expected use case (like I mentioned in my last reply): how to define "support enable_guc 1->3" (whether we should expect some system reboot, or we need guarantee 100% work with no system reboot required)? whether a system reboot for FW changes should be treated as code defects, etc. >> } >> void intel_uc_cleanup_early(struct drm_i915_private *i915) >> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct >> drm_i915_private *i915) >> struct intel_guc *guc = &i915->guc; >> struct intel_huc *huc = &i915->huc; >> - if (USES_HUC(i915)) >> - intel_uc_fw_fini(&huc->fw); >> - >> - if (USES_GUC(i915)) >> - intel_uc_fw_fini(&guc->fw); >> + intel_uc_fw_fini(&huc->fw); >> + intel_uc_fw_fini(&guc->fw); >> guc_free_load_err_log(guc); >> } >> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c >> b/drivers/gpu/drm/i915/intel_uc_fw.c >> index 6e8e0b5..c1fed06 100644 >> --- a/drivers/gpu/drm/i915/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c >> @@ -33,11 +33,13 @@ >> * >> * @dev_priv: device private >> * @uc_fw: uC firmware >> + * @fetch: whether fetch uC firmware into GEM object or not >> * >> - * Fetch uC firmware into GEM obj. >> + * Fetch and verify uC firmware and copy firmware data into GEM >> object if >> + * @fetch is true. >> */ >> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, >> - struct intel_uc_fw *uc_fw) >> + struct intel_uc_fw *uc_fw, bool fetch) >> { >> struct pci_dev *pdev = dev_priv->drm.pdev; >> struct drm_i915_gem_object *obj; >> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private >> *dev_priv, >> goto fail; >> } >> - obj = i915_gem_object_create_from_data(dev_priv, fw->data, >> fw->size); >> - if (IS_ERR(obj)) { >> - err = PTR_ERR(obj); >> - DRM_DEBUG_DRIVER("%s fw object_create err=%d\n", >> - intel_uc_fw_type_repr(uc_fw->type), err); >> - goto fail; >> + uc_fw->size = fw->size; >> + uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED; > > btw, I'm not sure that this new state is needed at all, as you don't > plan to use that fw obj if you only loaded fw blob to read header... > we will have the header info stored along with the intel_guc/intel_fw anyway. this state only suggests that valid FW images are available but wasn't loaded to the memory. Regards, -Jackie
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 1cffaf7..73b8f6c 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private *i915) sanitize_options_early(i915); - if (USES_GUC(i915)) - intel_uc_fw_fetch(i915, &guc->fw); - - if (USES_HUC(i915)) - intel_uc_fw_fetch(i915, &huc->fw); + intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915)); + intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915)); } void intel_uc_cleanup_early(struct drm_i915_private *i915) @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct drm_i915_private *i915) struct intel_guc *guc = &i915->guc; struct intel_huc *huc = &i915->huc; - if (USES_HUC(i915)) - intel_uc_fw_fini(&huc->fw); - - if (USES_GUC(i915)) - intel_uc_fw_fini(&guc->fw); + intel_uc_fw_fini(&huc->fw); + intel_uc_fw_fini(&guc->fw); guc_free_load_err_log(guc); } diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c index 6e8e0b5..c1fed06 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/intel_uc_fw.c @@ -33,11 +33,13 @@ * * @dev_priv: device private * @uc_fw: uC firmware + * @fetch: whether fetch uC firmware into GEM object or not * - * Fetch uC firmware into GEM obj. + * Fetch and verify uC firmware and copy firmware data into GEM object if + * @fetch is true. */ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, - struct intel_uc_fw *uc_fw) + struct intel_uc_fw *uc_fw, bool fetch) { struct pci_dev *pdev = dev_priv->drm.pdev; struct drm_i915_gem_object *obj; @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, goto fail; } - obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size); - if (IS_ERR(obj)) { - err = PTR_ERR(obj); - DRM_DEBUG_DRIVER("%s fw object_create err=%d\n", - intel_uc_fw_type_repr(uc_fw->type), err); - goto fail; + uc_fw->size = fw->size; + uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED; + + if (fetch) { + obj = i915_gem_object_create_from_data(dev_priv, fw->data, + fw->size); + if (IS_ERR(obj)) { + err = PTR_ERR(obj); + DRM_DEBUG_DRIVER("%s fw object_create err=%d\n", + intel_uc_fw_type_repr(uc_fw->type), + err); + goto fail; + } + + uc_fw->obj = obj; + uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS; } - uc_fw->obj = obj; - uc_fw->size = fw->size; - uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS; DRM_DEBUG_DRIVER("%s fw fetch %s\n", intel_uc_fw_type_repr(uc_fw->type), intel_uc_fw_status_repr(uc_fw->fetch_status)); diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h index dc33b12..a1308c0 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/intel_uc_fw.h @@ -36,6 +36,7 @@ enum intel_uc_fw_status { INTEL_UC_FIRMWARE_FAIL = -1, INTEL_UC_FIRMWARE_NONE = 0, INTEL_UC_FIRMWARE_PENDING, + INTEL_UC_FIRMWARE_VERIFIED, INTEL_UC_FIRMWARE_SUCCESS }; @@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) return "NONE"; case INTEL_UC_FIRMWARE_PENDING: return "PENDING"; + case INTEL_UC_FIRMWARE_VERIFIED: + return "VERIFIED"; case INTEL_UC_FIRMWARE_SUCCESS: return "SUCCESS"; } @@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) */ static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw) { - if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) + if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED) return 0; return uc_fw->header_size + uc_fw->ucode_size; } void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, - struct intel_uc_fw *uc_fw); + struct intel_uc_fw *uc_fw, bool fetch); int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, int (*xfer)(struct intel_uc_fw *uc_fw, struct i915_vma *vma));