| Message ID | 1519866100-19989-4-git-send-email-yaodong.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 01 Mar 2018 02:01:38 +0100, Jackie Li <yaodong.li@intel.com> wrote: > On CNL A0 and Gen9, there's a hardware restriction that requires the > available GuC WOPCM size to be larger than or equal to HuC firmware size. > > This patch adds new verification code to ensure the available GuC WOPCM > size to be larger than or equal to HuC firmware size on both Gen9 and CNL > A0. > > v6: > - Extended HuC FW size check against GuC WOPCM size to all > Gen9 and CNL A0 platforms > > v7: > - Fixed patch format issues > > v8: > - Renamed variables and functions to avoid ambiguity (Joonas) > - Updated commit message and comments to be more comprehensive (Sagar) > > v9: > - Moved code that is not related to restriction check into a separate > patch and updated the commit message accordingly (Sagar/Michal) > - Avoided to call uc_get_fw_size for better layer isolation (Michal) > > v10: > - Shorten function names and reorganized size_check code to have clear > isolation (Joonas) > - Removed unnecessary comments (Joonas) > > v11: > - Fixed logic error in size check (Michal) > > BSpec: 10875 > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: John Spotswood <john.a.spotswood@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v8) > Signed-off-by: Jackie Li <yaodong.li@intel.com> > --- > drivers/gpu/drm/i915/intel_wopcm.c | 28 +++++++++++++++++++++++++--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c > b/drivers/gpu/drm/i915/intel_wopcm.c > index bb78043..b30d7ff 100644 > --- a/drivers/gpu/drm/i915/intel_wopcm.c > +++ b/drivers/gpu/drm/i915/intel_wopcm.c > @@ -107,8 +107,26 @@ static inline int gen9_check_dword_gap(u32 > guc_wopcm_base, u32 guc_wopcm_size) > return 0; > } > +static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 > huc_fw_size) > +{ > + /* > + * On Gen9 & CNL A0, hardware requires the total available GuC WOPCM > + * size to be larger than or equal to HuC firmware size. Otherwise, > + * firmware uploading would fail. > + */ > + if (huc_fw_size > guc_wopcm_size - GUC_WOPCM_RESERVED) { > + DRM_ERROR("HuC fw(%uKiB) won't fit in GuC WOPCM(%uKiB).\n", > + huc_fw_size / 1024, > + (guc_wopcm_size - GUC_WOPCM_RESERVED) / 1024); bikeshed: in earlier patches in similar error messages, you used "HuC FW (%KiB)" and didn't provide available space. Maybe simplest way to unify and minimize the code is to add one "failed" tag in wopcm_init function where you can print all values used for partitioning: failed: DRM_ERROR("Failed to partition %uKiB WOPCM (%d)\n", wopcm->size/1024, err); DRM_ERROR("HuC FW size=%uKiB\n", ...); DRM_ERROR("GuC FW size=%uKiB\n", ...); return err; > + return -E2BIG; > + } > + > + return 0; > +} > + > static inline int check_hw_restriction(struct drm_i915_private *i915, > - u32 guc_wopcm_base, u32 guc_wopcm_size) > + u32 guc_wopcm_base, u32 guc_wopcm_size, > + u32 huc_fw_size) > { > int err = 0; > @@ -117,7 +135,10 @@ static inline int check_hw_restriction(struct > drm_i915_private *i915, > if (err) > return err; > - return 0; > + if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) > + err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size); > + > + return err; > } > /** > @@ -186,7 +207,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) > return -E2BIG; > } > - err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size); > + err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size, > + huc_fw_size); > if (err) { > DRM_ERROR("Failed to meet HW restriction.\n"); > return err; but bikeshed is not critical, so Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
On 03/01/2018 05:14 AM, Michal Wajdeczko wrote: > On Thu, 01 Mar 2018 02:01:38 +0100, Jackie Li <yaodong.li@intel.com> > wrote: > >> On CNL A0 and Gen9, there's a hardware restriction that requires the >> available GuC WOPCM size to be larger than or equal to HuC firmware >> size. >> >> This patch adds new verification code to ensure the available GuC WOPCM >> size to be larger than or equal to HuC firmware size on both Gen9 and >> CNL >> A0. >> >> v6: >> - Extended HuC FW size check against GuC WOPCM size to all >> Gen9 and CNL A0 platforms >> >> v7: >> - Fixed patch format issues >> >> v8: >> - Renamed variables and functions to avoid ambiguity (Joonas) >> - Updated commit message and comments to be more comprehensive (Sagar) >> >> v9: >> - Moved code that is not related to restriction check into a separate >> patch and updated the commit message accordingly (Sagar/Michal) >> - Avoided to call uc_get_fw_size for better layer isolation (Michal) >> >> v10: >> - Shorten function names and reorganized size_check code to have clear >> isolation (Joonas) >> - Removed unnecessary comments (Joonas) >> >> v11: >> - Fixed logic error in size check (Michal) >> >> BSpec: 10875 >> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: John Spotswood <john.a.spotswood@intel.com> >> Cc: Jeff McGee <jeff.mcgee@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v8) >> Signed-off-by: Jackie Li <yaodong.li@intel.com> >> --- >> drivers/gpu/drm/i915/intel_wopcm.c | 28 +++++++++++++++++++++++++--- >> 1 file changed, 25 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c >> b/drivers/gpu/drm/i915/intel_wopcm.c >> index bb78043..b30d7ff 100644 >> --- a/drivers/gpu/drm/i915/intel_wopcm.c >> +++ b/drivers/gpu/drm/i915/intel_wopcm.c >> @@ -107,8 +107,26 @@ static inline int gen9_check_dword_gap(u32 >> guc_wopcm_base, u32 guc_wopcm_size) >> return 0; >> } >> +static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 >> huc_fw_size) >> +{ >> + /* >> + * On Gen9 & CNL A0, hardware requires the total available GuC >> WOPCM >> + * size to be larger than or equal to HuC firmware size. Otherwise, >> + * firmware uploading would fail. >> + */ >> + if (huc_fw_size > guc_wopcm_size - GUC_WOPCM_RESERVED) { >> + DRM_ERROR("HuC fw(%uKiB) won't fit in GuC WOPCM(%uKiB).\n", >> + huc_fw_size / 1024, >> + (guc_wopcm_size - GUC_WOPCM_RESERVED) / 1024); > > bikeshed: in earlier patches in similar error messages, you used > "HuC FW (%KiB)" and didn't provide available space. Maybe simplest > way to unify and minimize the code is to add one "failed" tag in > wopcm_init function where you can print all values used for partitioning: > > failed: > DRM_ERROR("Failed to partition %uKiB WOPCM (%d)\n", > wopcm->size/1024, err); > DRM_ERROR("HuC FW size=%uKiB\n", ...); > DRM_ERROR("GuC FW size=%uKiB\n", ...); > return err; > I will keep it as it is now to save some time since I was told to keep these message at the point where the error happened.:-) >> + return -E2BIG; >> + } >> + >> + return 0; >> +} >> + >> static inline int check_hw_restriction(struct drm_i915_private *i915, >> - u32 guc_wopcm_base, u32 guc_wopcm_size) >> + u32 guc_wopcm_base, u32 guc_wopcm_size, >> + u32 huc_fw_size) >> { >> int err = 0; >> @@ -117,7 +135,10 @@ static inline int check_hw_restriction(struct >> drm_i915_private *i915, >> if (err) >> return err; >> - return 0; >> + if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, >> CNL_REVID_A0)) >> + err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size); >> + >> + return err; >> } >> /** >> @@ -186,7 +207,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) >> return -E2BIG; >> } >> - err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size); >> + err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size, >> + huc_fw_size); >> if (err) { >> DRM_ERROR("Failed to meet HW restriction.\n"); >> return err; > > but bikeshed is not critical, so > > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Thanks, -Jackie
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c index bb78043..b30d7ff 100644 --- a/drivers/gpu/drm/i915/intel_wopcm.c +++ b/drivers/gpu/drm/i915/intel_wopcm.c @@ -107,8 +107,26 @@ static inline int gen9_check_dword_gap(u32 guc_wopcm_base, u32 guc_wopcm_size) return 0; } +static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 huc_fw_size) +{ + /* + * On Gen9 & CNL A0, hardware requires the total available GuC WOPCM + * size to be larger than or equal to HuC firmware size. Otherwise, + * firmware uploading would fail. + */ + if (huc_fw_size > guc_wopcm_size - GUC_WOPCM_RESERVED) { + DRM_ERROR("HuC fw(%uKiB) won't fit in GuC WOPCM(%uKiB).\n", + huc_fw_size / 1024, + (guc_wopcm_size - GUC_WOPCM_RESERVED) / 1024); + return -E2BIG; + } + + return 0; +} + static inline int check_hw_restriction(struct drm_i915_private *i915, - u32 guc_wopcm_base, u32 guc_wopcm_size) + u32 guc_wopcm_base, u32 guc_wopcm_size, + u32 huc_fw_size) { int err = 0; @@ -117,7 +135,10 @@ static inline int check_hw_restriction(struct drm_i915_private *i915, if (err) return err; - return 0; + if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) + err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size); + + return err; } /** @@ -186,7 +207,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) return -E2BIG; } - err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size); + err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size, + huc_fw_size); if (err) { DRM_ERROR("Failed to meet HW restriction.\n"); return err;