| Message ID | 1523320940-32742-1-git-send-email-yaodong.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2018-04-09 at 17:42 -0700, Jackie Li 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 > > 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)); > } > > 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..a9cb900 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 > + * @copy_to_obj: 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 > + * @copy_to_obj 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 copy_to_obj) > { > 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 (copy_to_obj) { > + 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..4e7ecc8 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 copy_to_obj); > int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, > int (*xfer)(struct intel_uc_fw *uc_fw, > struct i915_vma *vma));
On Tue, 10 Apr 2018 02:42:17 +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). Is this frequent and required scenario ? or just for debug/development ? > 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. You are also assuming that on reload exactly the same GuC/HuC firmwares will bee used as in initial run. This will make this useless for debug/ development scenarios, where custom fw are likely to be specified. If we want to support enable_guc=1->3->1 scenarios for debug/dev then maybe more flexible will be other approach that makes allocations from the other end as proposed in [1] [1] https://patchwork.freedesktop.org/patch/212471/ > > v3: > - Rebase > > 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> > --- > 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)); Hmm, side effect of those unconditional fetches might be unwanted warnings about missing firmwares (on configs with disabled guc) as well as extended driver load time. Do we really need to support this corner case enable_guc=1->3 at all costs? /michal > } > 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..a9cb900 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 > + * @copy_to_obj: whether fetch uC firmware into GEM object or not s/copy_to_obj/fetch > * > - * Fetch uC firmware into GEM obj. > + * Fetch and verify uC firmware and copy firmware data into GEM object > if > + * @copy_to_obj 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 copy_to_obj) > { > 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 (copy_to_obj) { > + 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..4e7ecc8 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 copy_to_obj); > 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/13/2018 07:15 PM, Michal Wajdeczko wrote: > On Tue, 10 Apr 2018 02:42:17 +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). > > Is this frequent and required scenario ? or just for debug/development ? > My understanding is this should be a nice to have feature and mainly for debugging. >> 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. > > You are also assuming that on reload exactly the same GuC/HuC firmwares > will bee used as in initial run. This will make this useless for debug/ > development scenarios, where custom fw are likely to be specified. > This patch is mainly for providing a real fix to support enable_guc=1->3->1 use case. It based on the fact that it is inevitable that sometimes we need to reboot the system if the status of the fw was changed on the file system. I am not sure how often we switch between different HuC FW with different sizes? > If we want to support enable_guc=1->3->1 scenarios for debug/dev then > maybe more flexible will be other approach that makes allocations from > the other end as proposed in [1] > > [1] https://patchwork.freedesktop.org/patch/212471/ Actually, I do think this might be one of the options, and I've also put some comments on this series. The main concern I have is it still make assumption on the GuC FW size and may run into the same issue if the GuC FW failed to meet the requirement. and for debugging purpose it would have the same possible for different GuC FW debugging. > >> >> v3: >> - Rebase >> >> 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> >> --- >> 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)); > > Hmm, side effect of those unconditional fetches might be unwanted > warnings > about missing firmwares (on configs with disabled guc) as well as > extended > driver load time. Hmm, if HAS_GUC is false then fw path would be NULL. The fetch will return directly. > > Do we really need to support this corner case enable_guc=1->3 at all > costs? I think this is the real solution for this issue (with no assumption). However, we do need to decide whether we should support such a corner case which is mainly for debugging. > > /michal > >> } >> 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..a9cb900 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 >> + * @copy_to_obj: whether fetch uC firmware into GEM object or not > > s/copy_to_obj/fetch sure. > >> * >> - * Fetch uC firmware into GEM obj. >> + * Fetch and verify uC firmware and copy firmware data into GEM >> object if >> + * @copy_to_obj 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 copy_to_obj) >> { >> 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 (copy_to_obj) { >> + 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..4e7ecc8 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 copy_to_obj); >> int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, >> int (*xfer)(struct intel_uc_fw *uc_fw, >> struct i915_vma *vma));
On Mon, 16 Apr 2018 19:28:04 +0200, Yaodong Li <yaodong.li@intel.com> wrote: > On 04/13/2018 07:15 PM, Michal Wajdeczko wrote: >> On Tue, 10 Apr 2018 02:42:17 +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). >> >> Is this frequent and required scenario ? or just for debug/development ? >> > My understanding is this should be a nice to have feature and mainly for > debugging. >>> 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. >> >> You are also assuming that on reload exactly the same GuC/HuC firmwares >> will bee used as in initial run. This will make this useless for debug/ >> development scenarios, where custom fw are likely to be specified. >> > This patch is mainly for providing a real fix to support > enable_guc=1->3->1 use case. > It based on the fact that it is inevitable that sometimes we need to > reboot the system > if the status of the fw was changed on the file system. What do you mean by "status of the fw was changed on the file system" ? * change of the fw binary/version/size, or * change from not-present to present ? > I am not sure how often we switch between different HuC FW with > different sizes? Just above you said that you need this "mainly for debugging" so I would expect that then different fw sizes are expected. >> If we want to support enable_guc=1->3->1 scenarios for debug/dev then >> maybe more flexible will be other approach that makes allocations from >> the other end as proposed in [1] >> >> [1] https://patchwork.freedesktop.org/patch/212471/ > Actually, I do think this might be one of the options, and I've also put > some comments on this > series. The main concern I have is it still make assumption on the GuC > FW size and may But in enable_guc=1-->3 scenario, I would assume that the only difference will be HuC fw (as with enable=1 we already loaded GuC) If you want just to test different GuC fws, then it is different scenario as then enable_guc will always be = 1. > run into the same issue if the GuC FW failed to meet the requirement. > and for debugging purpose it would have the same possible for different > GuC FW debugging. >> >>> >>> v3: >>> - Rebase >>> >>> 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> >>> --- >>> 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)); >> >> Hmm, side effect of those unconditional fetches might be unwanted >> warnings >> about missing firmwares (on configs with disabled guc) as well as >> extended >> driver load time. > Hmm, if HAS_GUC is false then fw path would be NULL. The fetch will > return directly. I was referring to scenario when on platform with HAS_HUC and with enable_guc=1 (just submission, no HuC) we will try to fetch HuC fw (that may not be present at all) and then drop it as don't need it. >> >> Do we really need to support this corner case enable_guc=1->3 at all >> costs? > I think this is the real solution for this issue (with no assumption). > However, we do > need to decide whether we should support such a corner case which is > mainly for > debugging. I'm repeating here Joonas' earlier statement: "Then just require a reboot if after that partitioning, changing the parameter causes the FW not to fit" >> >> /michal >> >>> } >>> 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..a9cb900 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 >>> + * @copy_to_obj: whether fetch uC firmware into GEM object or not >> >> s/copy_to_obj/fetch > sure. >> >>> * >>> - * Fetch uC firmware into GEM obj. >>> + * Fetch and verify uC firmware and copy firmware data into GEM >>> object if >>> + * @copy_to_obj 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 copy_to_obj) >>> { >>> 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 (copy_to_obj) { >>> + 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..4e7ecc8 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 copy_to_obj); >>> 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:31 AM, Michal Wajdeczko wrote: > On Mon, 16 Apr 2018 19:28:04 +0200, Yaodong Li <yaodong.li@intel.com> > wrote: > >> On 04/13/2018 07:15 PM, Michal Wajdeczko wrote: >>> On Tue, 10 Apr 2018 02:42:17 +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). >>> >>> Is this frequent and required scenario ? or just for >>> debug/development ? >>> >> My understanding is this should be a nice to have feature and mainly >> for debugging. >>>> 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. >>> >>> You are also assuming that on reload exactly the same GuC/HuC firmwares >>> will bee used as in initial run. This will make this useless for debug/ >>> development scenarios, where custom fw are likely to be specified. >>> >> This patch is mainly for providing a real fix to support >> enable_guc=1->3->1 use case. >> It based on the fact that it is inevitable that sometimes we need to >> reboot the system >> if the status of the fw was changed on the file system. > > What do you mean by "status of the fw was changed on the file system" ? > * change of the fw binary/version/size, or > * change from not-present to present ? I think it should include all of the presence changes, file updates. > >> I am not sure how often we switch between different HuC FW with >> different sizes? > > Just above you said that you need this "mainly for debugging" so > I would expect that then different fw sizes are expected. > >>> If we want to support enable_guc=1->3->1 scenarios for debug/dev then >>> maybe more flexible will be other approach that makes allocations from >>> the other end as proposed in [1] >>> >>> [1] https://patchwork.freedesktop.org/patch/212471/ >> Actually, I do think this might be one of the options, and I've also >> put some comments on this >> series. The main concern I have is it still make assumption on the >> GuC FW size and may > > But in enable_guc=1-->3 scenario, I would assume that the only difference > will be HuC fw (as with enable=1 we already loaded GuC) Hmm, my main concern to the current "from the end" solution is it makes assumption on the GuC FW size in order to meet the HW restriction. > > If you want just to test different GuC fws, then it is different scenario > as then enable_guc will always be = 1. > what I mean is the "from the end" approach will lead to the same issue for different GuC FW sizes - we may have to reboot the system for GuC FW debugging (different GuC FW sizes) even if enable_guc is always set to 1. However, with the current "from the beginning" way we won't run into such problems for GuC FW debugging (since it's already used the max available space). Thus I think we should define the enable_guc = 1->3->1 as following: we would support use of enable_guc=1->3->1 correctly without system reboot for the present FWs. A system reboot will be expected (but not necessarily happen if we found current partition works for the new FWs) for any FW changes (including add/remove/update). if we decide to drop the support for enable_guc=1->3->1 since it's only for debugging purpose then we should expect a system reboot for either "from the end" or "from the beginning" solutions since we cannot 100% have this issue (changing FW without a system boot) solved. Therefore, the require of system reboot should not be a bug when it comes to FW updating. >> run into the same issue if the GuC FW failed to meet the requirement. >> and for debugging purpose it would have the same possible for >> different GuC FW debugging. >>> >>>> >>>> v3: >>>> - Rebase >>>> >>>> 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> >>>> --- >>>> 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)); >>> >>> Hmm, side effect of those unconditional fetches might be unwanted >>> warnings >>> about missing firmwares (on configs with disabled guc) as well as >>> extended >>> driver load time. >> Hmm, if HAS_GUC is false then fw path would be NULL. The fetch will >> return directly. > > I was referring to scenario when on platform with HAS_HUC and with > enable_guc=1 (just submission, no HuC) we will try to fetch HuC fw > (that may not be present at all) and then drop it as don't need it. > I think there are two scenarios here for this specific case - a platform with HAS_HUC = 1 and only GuC submission is needed: 0) No HuC FW available - We should expect a system reboot for adding new FW. 1) If HuC FW is present - always get the FW header info in order to support possible enable_guc=1->3->1. IMHO, the problem we have here is that we need to define the use case precisely. e.g. whether we shall support enable_guc=1->3->1 flawlessly? and whether we shall support dynamic HuC FW sizes for debugging rather than supporting dynamic GuC FW sizes for debugging purpose? >>> >>> Do we really need to support this corner case enable_guc=1->3 at all >>> costs? >> I think this is the real solution for this issue (with no >> assumption). However, we do >> need to decide whether we should support such a corner case which is >> mainly for >> debugging. > > I'm repeating here Joonas' earlier statement: > > "Then just require a reboot if after that partitioning, > changing the parameter causes the FW not to fit" > That's my thought too:) 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..a9cb900 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 + * @copy_to_obj: 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 + * @copy_to_obj 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 copy_to_obj) { 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 (copy_to_obj) { + 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..4e7ecc8 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 copy_to_obj); int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, int (*xfer)(struct intel_uc_fw *uc_fw, struct i915_vma *vma));
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 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> --- 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(-)