| Message ID | 1518479153-28429-5-git-send-email-yaodong.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 13 Feb 2018 00:45:51 +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 verfication 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) > > 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_guc_wopcm.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c > b/drivers/gpu/drm/i915/intel_guc_wopcm.c > index 9a276fe..0194266 100644 > --- a/drivers/gpu/drm/i915/intel_guc_wopcm.c > +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c > @@ -19,6 +19,20 @@ static inline u32 context_reserved_size(struct > intel_guc *guc) > return 0; > } > +static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm, > + 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 (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size) What if guc_wopcm->size < GUC_WOPCM_RESERVED ? > + return -E2BIG; > + > + return 0; > +} > + > static inline int gen9_check_dword_gap(struct intel_guc_wopcm > *guc_wopcm) > { > u32 guc_wopcm_start; > @@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct > intel_guc_wopcm *guc_wopcm) > return 0; > } > -static inline int guc_wopcm_size_check(struct intel_guc *guc) > +static inline int guc_wopcm_size_check(struct intel_guc *guc, u32 > huc_fw_size) > { > struct drm_i915_private *i915 = guc_to_i915(guc); > struct intel_guc_wopcm *guc_wopcm = &guc->wopcm; > + int err = 0; > if (IS_GEN9(i915)) > - return gen9_check_dword_gap(guc_wopcm); > + err = gen9_check_dword_gap(guc_wopcm); > - return 0; > + if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) > + err = check_huc_fw_fits(guc_wopcm, huc_fw_size); Hmm, what if gen9_check_dword_gap() fails but check_huc_fw_fits() passes ? > + > + return err; > } > /** > @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm > *guc_wopcm, u32 guc_fw_size, > guc->wopcm.size = size; > guc->wopcm.top = top; > - err = guc_wopcm_size_check(guc); > + err = guc_wopcm_size_check(guc, huc_fw_size); > if (err) { > DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n"); > return err; I'm more and more convinced that we should use "intel_wopcm" to make partition and all these checks Michal
On 02/13/2018 08:06 AM, Michal Wajdeczko wrote: > +static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm, >> + 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 (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size) > > What if guc_wopcm->size < GUC_WOPCM_RESERVED ? > we've already had following check before this. which had guaranteed guc_wopcm->size >= guc_fw_size + reserved, thus, guc_wopcm->size > GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED so, guc_wopcm->size > GUC_WOPCM_RESERVED:-) reserved = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED; if ((guc_fw_size + reserved) > guc_wopcm->size) { DRM_DEBUG_DRIVER("No enough GuC WOPCM for GuC firmware.\n"); return -E2BIG; } >> + return -E2BIG; >> + >> + return 0; >> +} >> + >> static inline int gen9_check_dword_gap(struct intel_guc_wopcm >> *guc_wopcm) >> { >> u32 guc_wopcm_start; >> @@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct >> intel_guc_wopcm *guc_wopcm) >> return 0; >> } >> -static inline int guc_wopcm_size_check(struct intel_guc *guc) >> +static inline int guc_wopcm_size_check(struct intel_guc *guc, u32 >> huc_fw_size) >> { >> struct drm_i915_private *i915 = guc_to_i915(guc); >> struct intel_guc_wopcm *guc_wopcm = &guc->wopcm; >> + int err = 0; >> if (IS_GEN9(i915)) >> - return gen9_check_dword_gap(guc_wopcm); >> + err = gen9_check_dword_gap(guc_wopcm); >> - return 0; >> + if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, >> CNL_REVID_A0)) >> + err = check_huc_fw_fits(guc_wopcm, huc_fw_size); > > Hmm, what if gen9_check_dword_gap() fails but check_huc_fw_fits() > passes ? > oops! will fix this.:-) >> + >> + return err; >> } >> /** >> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm >> *guc_wopcm, u32 guc_fw_size, >> guc->wopcm.size = size; >> guc->wopcm.top = top; >> - err = guc_wopcm_size_check(guc); >> + err = guc_wopcm_size_check(guc, huc_fw_size); >> if (err) { >> DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n"); >> return err; > > I'm more and more convinced that we should use "intel_wopcm" to make > partition and all these checks > These are all GuC wopcm related, it only checks guc wopcm size. so I am wondering whether I am still misunderstanding anything here?since the purpose of these calculation and checks are clearly all GuC related. To be more precisely GuC DMA related. The driver's responsibility is to calculate the GuC DMA offset and GuC wopcm size values based on guc/huc fw sizes and all these checks are all for the correctness for the GuC wopcm size. I don't see any reason why these should be a part of global intel_wopcm even if we really want to keep something like wopcm_regions for both guc/huc(though I still don't know what the benefit real is to keep something like HuC wopcm region except for sth like debugfs output?). even in this case, we still at least keep these as a part of GuC since we really need it to setup GuC HW :- Or do you mean we should create an intel_wopcm to carry info such as global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a little bit confused here:-(
On Tue, 13 Feb 2018 22:50:53 +0100, Yaodong Li <yaodong.li@intel.com> wrote: > On 02/13/2018 08:06 AM, Michal Wajdeczko wrote: >> +static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm, >>> + 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 (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size) >> >> What if guc_wopcm->size < GUC_WOPCM_RESERVED ? >> > we've already had following check before this. which had guaranteed > guc_wopcm->size >= guc_fw_size + reserved, thus, > guc_wopcm->size > GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED > so, guc_wopcm->size > GUC_WOPCM_RESERVED:-) > > reserved = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED; > if ((guc_fw_size + reserved) > guc_wopcm->size) { > DRM_DEBUG_DRIVER("No enough GuC WOPCM for GuC firmware.\n"); > return -E2BIG; > } Hmm, that only proves that current partitioning code is too complicated :) If you look at diagram it should be possible to implement it as guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16)); guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4)) reserved = context_reserved_size(i915); if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE) return -E2BIG; (E&O) >>> + return -E2BIG; >>> + >>> + return 0; >>> +} >>> + >>> static inline int gen9_check_dword_gap(struct intel_guc_wopcm >>> *guc_wopcm) >>> { >>> u32 guc_wopcm_start; >>> @@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct >>> intel_guc_wopcm *guc_wopcm) >>> return 0; >>> } >>> -static inline int guc_wopcm_size_check(struct intel_guc *guc) >>> +static inline int guc_wopcm_size_check(struct intel_guc *guc, u32 >>> huc_fw_size) >>> { >>> struct drm_i915_private *i915 = guc_to_i915(guc); >>> struct intel_guc_wopcm *guc_wopcm = &guc->wopcm; >>> + int err = 0; >>> if (IS_GEN9(i915)) >>> - return gen9_check_dword_gap(guc_wopcm); >>> + err = gen9_check_dword_gap(guc_wopcm); >>> - return 0; >>> + if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, >>> CNL_REVID_A0)) >>> + err = check_huc_fw_fits(guc_wopcm, huc_fw_size); >> >> Hmm, what if gen9_check_dword_gap() fails but check_huc_fw_fits() >> passes ? >> > oops! will fix this.:-) >>> + >>> + return err; >>> } >>> /** >>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm >>> *guc_wopcm, u32 guc_fw_size, >>> guc->wopcm.size = size; >>> guc->wopcm.top = top; >>> - err = guc_wopcm_size_check(guc); >>> + err = guc_wopcm_size_check(guc, huc_fw_size); >>> if (err) { >>> DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n"); >>> return err; >> >> I'm more and more convinced that we should use "intel_wopcm" to make >> partition and all these checks >> > These are all GuC wopcm related, it only checks guc wopcm size. so I am > wondering whether > I am still misunderstanding anything here?since the purpose of these > calculation and checks are > clearly all GuC related. To be more precisely GuC DMA related. The > driver's responsibility is to > calculate the GuC DMA offset and GuC wopcm size values based on guc/huc > fw sizes and > all these checks are all for the correctness for the GuC wopcm size. > I don't see any reason why these should be a part of global intel_wopcm > even if we really > want to keep something like wopcm_regions for both guc/huc(though I > still don't know what > the benefit real is to keep something like HuC wopcm region except for > sth like debugfs output?). > even in this case, we still at least keep these as a part of GuC since > we really need it to setup > GuC HW :- Or do you mean we should create an intel_wopcm to carry info > such as > global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a > little bit confused here:-( struct intel_wopcm should carry only results of WOPCM partitioning between all agents including GuC. There is no need to carry fw sizes as those are only needed as input for calculations. You can still program GuC region from uc code.
On 02/13/2018 02:59 PM, Michal Wajdeczko wrote: > Hmm, that only proves that current partitioning code is too > complicated :) > If you look at diagram it should be possible to implement it as current calculation tries to set the maximum available WOPCM to avoid Gen9 limitations. that the reason I didn't use guc_fw_size + GUC_WOPCM_RESERVED for size calculation.:-) > > guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16)); the same as current calculation. > guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4)) it's sample but likely run into gen9 gaps HW restriction.:-) > reserved = context_reserved_size(i915); > > if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE) > return -E2BIG; > > (E&O) > >>>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm >>>> *guc_wopcm, u32 guc_fw_size, >>>> guc->wopcm.size = size; >>>> guc->wopcm.top = top; >>>> - err = guc_wopcm_size_check(guc); >>>> + err = guc_wopcm_size_check(guc, huc_fw_size); >>>> if (err) { >>>> DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n"); >>>> return err; >>> >>> I'm more and more convinced that we should use "intel_wopcm" to make >>> partition and all these checks >>> >> These are all GuC wopcm related, it only checks guc wopcm size. so I >> am wondering whether >> I am still misunderstanding anything here?since the purpose of these >> calculation and checks are >> clearly all GuC related. To be more precisely GuC DMA related. The >> driver's responsibility is to >> calculate the GuC DMA offset and GuC wopcm size values based on >> guc/huc fw sizes and >> all these checks are all for the correctness for the GuC wopcm size. >> I don't see any reason why these should be a part of global >> intel_wopcm even if we really >> want to keep something like wopcm_regions for both guc/huc(though I >> still don't know what >> the benefit real is to keep something like HuC wopcm region except >> for sth like debugfs output?). >> even in this case, we still at least keep these as a part of GuC >> since we really need it to setup >> GuC HW :- Or do you mean we should create an intel_wopcm to carry >> info such as >> global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a >> little bit confused here:-( > > struct intel_wopcm should carry only results of WOPCM partitioning > between > all agents including GuC. There is no need to carry fw sizes as those are > only needed as input for calculations. > I understand the point. but what I am trying to explain is that we can only focus on GuC WOPCM setting since there's no need to keep tracking other regions since they are just results of setting of GuC WOPCM registers (what we are trying to do is to figure out a window on the WOPCM for GuC, and we don't need to keep tracking info such as HuC WOPCM size, CTX reserved WOPCM size because KMD driver won't use these info anymore). If you do think it's clearer to have something like intel_uc_wopcm.h/intel_uc_wopcm.c I can sure make these changes, but what I am saying is we won't need to keep likely unused info data in KMD. And we always can treat the other parts of WOPCM as reserved for other HW use. and only take care of what KMD needs to do. Please let me know if you still think we should have something like uc_wopcm. I will work it out. Regards, -Jackie
On Wed, 14 Feb 2018 01:41:57 +0100, Yaodong Li <yaodong.li@intel.com> wrote: > > > On 02/13/2018 02:59 PM, Michal Wajdeczko wrote: >> Hmm, that only proves that current partitioning code is too complicated >> :) >> If you look at diagram it should be possible to implement it as > current calculation tries to set the maximum available WOPCM to avoid > Gen9 limitations. that the reason I didn't use guc_fw_size + > GUC_WOPCM_RESERVED > for size calculation.:-) >> >> guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16)); > the same as current calculation. but definitely simpler and easier to review ;) >> guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4)) > it's sample but likely run into gen9 gaps HW restriction.:-) feel free to add/fix it here ;) >> reserved = context_reserved_size(i915); >> >> if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE) >> return -E2BIG; >> >> (E&O) >> > >>>>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm >>>>> *guc_wopcm, u32 guc_fw_size, >>>>> guc->wopcm.size = size; >>>>> guc->wopcm.top = top; >>>>> - err = guc_wopcm_size_check(guc); >>>>> + err = guc_wopcm_size_check(guc, huc_fw_size); >>>>> if (err) { >>>>> DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n"); >>>>> return err; >>>> >>>> I'm more and more convinced that we should use "intel_wopcm" to make >>>> partition and all these checks >>>> >>> These are all GuC wopcm related, it only checks guc wopcm size. so I >>> am wondering whether >>> I am still misunderstanding anything here?since the purpose of these >>> calculation and checks are >>> clearly all GuC related. To be more precisely GuC DMA related. The >>> driver's responsibility is to >>> calculate the GuC DMA offset and GuC wopcm size values based on >>> guc/huc fw sizes and >>> all these checks are all for the correctness for the GuC wopcm size. >>> I don't see any reason why these should be a part of global >>> intel_wopcm even if we really >>> want to keep something like wopcm_regions for both guc/huc(though I >>> still don't know what >>> the benefit real is to keep something like HuC wopcm region except for >>> sth like debugfs output?). >>> even in this case, we still at least keep these as a part of GuC since >>> we really need it to setup >>> GuC HW :- Or do you mean we should create an intel_wopcm to carry info >>> such as >>> global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a >>> little bit confused here:-( >> >> struct intel_wopcm should carry only results of WOPCM partitioning >> between >> all agents including GuC. There is no need to carry fw sizes as those >> are >> only needed as input for calculations. >> > I understand the point. but what I am trying to explain is that we can > only focus on GuC WOPCM setting since there's no > need to keep tracking other regions since they are just results of > setting of GuC WOPCM registers Wrong, start thinking that values used to program GuC WOPCM registers are derived from WOPCM partitioning that was done after taking into account HuC WOPCM size, CTX reserved WOPCM, etc. Then you can avoid all these tricky reverse calculation that you have. > (what we are > trying to do is to figure out a window on the WOPCM for GuC, and we > don't need to keep tracking info such as > HuC WOPCM size, CTX reserved WOPCM size because KMD driver won't use > these info anymore). > > If you do think it's clearer to have something like > intel_uc_wopcm.h/intel_uc_wopcm.c I can sure make these changes, > but what I am saying is we won't need to keep likely unused info data in > KMD. And we always can treat the other parts > of WOPCM as reserved for other HW use. and only take care of what KMD > needs to do. Please let me know if you > still think we should have something like uc_wopcm. I will work it out. I'm looking for intel_wopcm that will do partitioning and hold results. Then you can program GuC WOPCM in static function inside intel_uc.c using values from intel_wopcm that we guarantee to be *always* valid at that point. Michal
On 02/14/2018 09:24 AM, Michal Wajdeczko wrote: > On Wed, 14 Feb 2018 01:41:57 +0100, Yaodong Li <yaodong.li@intel.com> > wrote: > >> >> >> On 02/13/2018 02:59 PM, Michal Wajdeczko wrote: >>> Hmm, that only proves that current partitioning code is too >>> complicated :) >>> If you look at diagram it should be possible to implement it as >> current calculation tries to set the maximum available WOPCM to avoid >> Gen9 limitations. that the reason I didn't use guc_fw_size + >> GUC_WOPCM_RESERVED >> for size calculation.:-) >>> >>> guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16)); >> the same as current calculation. > > but definitely simpler and easier to review ;) > >>> guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4)) >> it's sample but likely run into gen9 gaps HW restriction.:-) > > feel free to add/fix it here ;) It will likely fall into the same steps. But sure will be > >>> reserved = context_reserved_size(i915); >>> >>> if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE) >>> return -E2BIG; >>> >>> (E&O) >>> >> >>>>>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct >>>>>> intel_guc_wopcm *guc_wopcm, u32 guc_fw_size, >>>>>> guc->wopcm.size = size; >>>>>> guc->wopcm.top = top; >>>>>> - err = guc_wopcm_size_check(guc); >>>>>> + err = guc_wopcm_size_check(guc, huc_fw_size); >>>>>> if (err) { >>>>>> DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n"); >>>>>> return err; >>>>> >>>>> I'm more and more convinced that we should use "intel_wopcm" to make >>>>> partition and all these checks >>>>> >>>> These are all GuC wopcm related, it only checks guc wopcm size. so >>>> I am wondering whether >>>> I am still misunderstanding anything here?since the purpose of >>>> these calculation and checks are >>>> clearly all GuC related. To be more precisely GuC DMA related. The >>>> driver's responsibility is to >>>> calculate the GuC DMA offset and GuC wopcm size values based on >>>> guc/huc fw sizes and >>>> all these checks are all for the correctness for the GuC wopcm size. >>>> I don't see any reason why these should be a part of global >>>> intel_wopcm even if we really >>>> want to keep something like wopcm_regions for both guc/huc(though I >>>> still don't know what >>>> the benefit real is to keep something like HuC wopcm region except >>>> for sth like debugfs output?). >>>> even in this case, we still at least keep these as a part of GuC >>>> since we really need it to setup >>>> GuC HW :- Or do you mean we should create an intel_wopcm to carry >>>> info such as >>>> global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a >>>> little bit confused here:-( >>> >>> struct intel_wopcm should carry only results of WOPCM partitioning >>> between >>> all agents including GuC. There is no need to carry fw sizes as >>> those are >>> only needed as input for calculations. >>> >> I understand the point. but what I am trying to explain is that we >> can only focus on GuC WOPCM setting since there's no >> need to keep tracking other regions since they are just results of >> setting of GuC WOPCM registers > > Wrong, start thinking that values used to program GuC WOPCM registers > are derived from WOPCM partitioning that was done after taking into > account HuC WOPCM size, CTX reserved WOPCM, etc. > > Then you can avoid all these tricky reverse calculation that you have. Hmm.I don't think an abstraction would be wrong or right.;-) In this case, I don't think the abstraction of all WOPCM regions is a wise thing to do because it's unnecessary to keep tracking these only results to more memory unit to store redundant data, especially after we known for sure that no one would use these data because they are likely to consumed by some hw components that are transparent to kernel mode driver or no hw would access them at all - so why bother? this is the question I had and I believe this is the main reason I was not convinced by the idea of partitioning and had switched to current implementation. (Actually, I had similar thought sin my earlier patches:-), and I even had a patch which already has similar implementations, so It would be very fast for me to switch to these after I am convinced). I really respect every comments and really appreciate each correction. and I would even appreciate more if I was totally convinced by good ideas:-) Regards, -Jackie
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c b/drivers/gpu/drm/i915/intel_guc_wopcm.c index 9a276fe..0194266 100644 --- a/drivers/gpu/drm/i915/intel_guc_wopcm.c +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c @@ -19,6 +19,20 @@ static inline u32 context_reserved_size(struct intel_guc *guc) return 0; } +static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm, + 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 (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size) + return -E2BIG; + + return 0; +} + static inline int gen9_check_dword_gap(struct intel_guc_wopcm *guc_wopcm) { u32 guc_wopcm_start; @@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct intel_guc_wopcm *guc_wopcm) return 0; } -static inline int guc_wopcm_size_check(struct intel_guc *guc) +static inline int guc_wopcm_size_check(struct intel_guc *guc, u32 huc_fw_size) { struct drm_i915_private *i915 = guc_to_i915(guc); struct intel_guc_wopcm *guc_wopcm = &guc->wopcm; + int err = 0; if (IS_GEN9(i915)) - return gen9_check_dword_gap(guc_wopcm); + err = gen9_check_dword_gap(guc_wopcm); - return 0; + if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) + err = check_huc_fw_fits(guc_wopcm, huc_fw_size); + + return err; } /** @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size, guc->wopcm.size = size; guc->wopcm.top = top; - err = guc_wopcm_size_check(guc); + err = guc_wopcm_size_check(guc, huc_fw_size); if (err) { DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n"); return err;