| Message ID | 20180323123411.3214-5-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/23/2018 05:34 AM, Michał Winiarski wrote: > We need GuC to load HuC, but it's also possible for GuC to operate on > its own. We don't know what the size of HuC FW may be, so, not wanting > to make any platform-specific hardcoded guesses, we assume that its size > is 0... Which is a very bad approximation. > This has a very unfortunate consequence - once we've booted with GuC and > no HuC, we'll never be able to load HuC (unless we reboot, since the > registers are locked). > Rather than using unknown values in our computations - let's partition > based on GuC size. we can do this based on known GuC and HuC sizes without any assumption on FW sizes :) please also refer to: https://patchwork.freedesktop.org/series/40541/ > We have one HW restriction where we're using HuC size (GuC size needs to > be roughly similar to HuC size - which may be unknown at this point), > luckily, another HW restriction makes it very unlikely to run in any > sort of issues in this case. > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jackie Li <yaodong.li@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++----------------- > 1 file changed, 34 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c > index 52841d340002..295d302e97b9 100644 > --- a/drivers/gpu/drm/i915/intel_wopcm.c > +++ b/drivers/gpu/drm/i915/intel_wopcm.c > @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd) > return 0; > } > > -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm) > +static inline void > +__guc_region_grow(struct intel_wopcm *wopcm, u32 size) > +{ > + /* > + * We're growing guc region in the direction of lower addresses. > + * We need to use multiples of base alignment, because it has more > + * strict alignment rules. > + */ > + size = DIV_ROUND_UP(size, 2); A bit confused here. Don't we just want to push the base downward for *size* bytes? > + size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT); Hmm, I think we only need align it to 4K boundary. > + > + wopcm->guc.base -= size; > + wopcm->guc.size += size; > +} > + > +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm) > { > struct drm_i915_private *i915 = wopcm_to_i915(wopcm); > u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw); > @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm) > size = gen9_size_for_dword_gap_restriction(wopcm->guc.base, > wopcm->guc.size); > if (size) > - goto err; > + __guc_region_grow(wopcm, size); > > size = gen9_size_for_huc_restriction(wopcm->guc.size, > huc_fw_size); Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case. Once the registers were locked, enable_guc=3 may still fail since huc_fw_size may still large enough to break the restriction we want to enforce that: hw_fw_size + 16KB > guc_fw_size. > if (size) > - goto err; > - } > - > - return 0; > + __guc_region_grow(wopcm, size); > > -err: > - DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n", > - wopcm->guc.size / 1024, > - size / 1024); > - > - return -E2BIG; > + GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base, > + wopcm->guc.size)); > + GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size, > + huc_fw_size)); > + } > } > > static bool wopcm_check_components_fit(struct intel_wopcm *wopcm) > @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm) > return 0; > } > > -static int wopcm_guc_init(struct intel_wopcm *wopcm) > +static int wopcm_guc_region_init(struct intel_wopcm *wopcm) > { > struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm); > - u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw); > + u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw); > u32 ctx_rsvd = context_reserved_size(dev_priv); > > - wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size), > - GUC_WOPCM_OFFSET_ALIGNMENT); > + wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size); > > - wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd, > - PAGE_SIZE); > - > - DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n", > - wopcm->guc.base / 1024, > - (wopcm->guc.base + wopcm->guc.size) / 1024); > + wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size, > + GUC_WOPCM_OFFSET_ALIGNMENT); guc.size + ctx_rsvd > wopcm->size check? > > return 0; > } > @@ -255,18 +261,20 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) > > GEM_BUG_ON(!wopcm->size); > > - err = wopcm_guc_init(wopcm); > + err = wopcm_guc_region_init(wopcm); Again, wopcm will contain invalid data on failure. But maybe we can go with it now :) Regards, -Jackie
On Fri, Mar 23, 2018 at 01:00:35PM -0700, Yaodong Li wrote: > On 03/23/2018 05:34 AM, Michał Winiarski wrote: > > We need GuC to load HuC, but it's also possible for GuC to operate on > > its own. We don't know what the size of HuC FW may be, so, not wanting > > to make any platform-specific hardcoded guesses, we assume that its size > > is 0... Which is a very bad approximation. > > This has a very unfortunate consequence - once we've booted with GuC and > > no HuC, we'll never be able to load HuC (unless we reboot, since the > > registers are locked). > > Rather than using unknown values in our computations - let's partition > > based on GuC size. > we can do this based on known GuC and HuC sizes without > any assumption on FW sizes :) > please also refer to: https://patchwork.freedesktop.org/series/40541/ You need to have HuC FW present in the filesystem do do that though. (IIUC we still get 0 if it's not there) > > We have one HW restriction where we're using HuC size (GuC size needs to > > be roughly similar to HuC size - which may be unknown at this point), > > luckily, another HW restriction makes it very unlikely to run in any > > sort of issues in this case. > > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Jackie Li <yaodong.li@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > --- > > drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++----------------- > > 1 file changed, 34 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c > > index 52841d340002..295d302e97b9 100644 > > --- a/drivers/gpu/drm/i915/intel_wopcm.c > > +++ b/drivers/gpu/drm/i915/intel_wopcm.c > > @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd) > > return 0; > > } > > -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm) > > +static inline void > > +__guc_region_grow(struct intel_wopcm *wopcm, u32 size) > > +{ > > + /* > > + * We're growing guc region in the direction of lower addresses. > > + * We need to use multiples of base alignment, because it has more > > + * strict alignment rules. > > + */ > > + size = DIV_ROUND_UP(size, 2); > A bit confused here. Don't we just want to push the base downward for > *size* bytes? Starting from the following: +--------------+ |--------------| || || || || || GuC || || region || || || || || || || || || +--------------+ | | | | | | | | | | | | | | | | | | +--------------+ We want to grow the whole region by size bytes. Pushing the base downward gives us this: +--------------+ | Empty | | space | +--------------+ || || || || || GuC || || region || || || || || || || || || +--------------+ | | | | | | | | | | | | | | +--------------+ Which leaves less space for HuC (and we're also leaving a bunch of unused space in our partitioning). If we modify both base and size to end up with this: +--------------+ |--------------| || || || || || GuC || || region || || || || || || || || || || || +--------------+ | | | | | | | | | | | | | | | | +--------------+ We're still satisfying the HW restriciton, but we're not wasting any space from the top (and also we're leaving more space for HuC). > > + size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT); > Hmm, I think we only need align it to 4K boundary. No - because we're modifying both base (16K alignment) and size (4K alignment). > > + > > + wopcm->guc.base -= size; > > + wopcm->guc.size += size; > > +} > > + > > +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm) > > { > > struct drm_i915_private *i915 = wopcm_to_i915(wopcm); > > u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw); > > @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm) > > size = gen9_size_for_dword_gap_restriction(wopcm->guc.base, > > wopcm->guc.size); > > if (size) > > - goto err; > > + __guc_region_grow(wopcm, size); > > size = gen9_size_for_huc_restriction(wopcm->guc.size, > > huc_fw_size); > Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case. > Once the registers were locked, enable_guc=3 may still fail since > huc_fw_size > may still large enough to break the restriction we want to enforce that: > hw_fw_size + 16KB > guc_fw_size. Well - as mentioned in the commit message, since we also have a "dword gap" restriction, we're left with pretty large GuC size, which makes this problem dissaperar for real-world HuC FW sizes. But yeah - I agree, we're not able to guarantee this. > > if (size) > > - goto err; > > - } > > - > > - return 0; > > + __guc_region_grow(wopcm, size); > > -err: > > - DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n", > > - wopcm->guc.size / 1024, > > - size / 1024); > > - > > - return -E2BIG; > > + GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base, > > + wopcm->guc.size)); > > + GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size, > > + huc_fw_size)); > > + } > > } > > static bool wopcm_check_components_fit(struct intel_wopcm *wopcm) > > @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm) > > return 0; > > } > > -static int wopcm_guc_init(struct intel_wopcm *wopcm) > > +static int wopcm_guc_region_init(struct intel_wopcm *wopcm) > > { > > struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm); > > - u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw); > > + u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw); > > u32 ctx_rsvd = context_reserved_size(dev_priv); > > - wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size), > > - GUC_WOPCM_OFFSET_ALIGNMENT); > > + wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size); > > - wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd, > > - PAGE_SIZE); > > - > > - DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n", > > - wopcm->guc.base / 1024, > > - (wopcm->guc.base + wopcm->guc.size) / 1024); > > + wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size, > > + GUC_WOPCM_OFFSET_ALIGNMENT); > guc.size + ctx_rsvd > wopcm->size check? It's being done in check_ctx_rsvd_fits later on (in wopcm_check_components_fit). > > return 0; > > } > > @@ -255,18 +261,20 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) > > GEM_BUG_ON(!wopcm->size); > > - err = wopcm_guc_init(wopcm); > > + err = wopcm_guc_region_init(wopcm); > Again, wopcm will contain invalid data on failure. But maybe we can > go with it now :) I could go back to passing around a bunch of locals, but since we're only accessing wopcm here I think passing around struct intel_wopcm makes the code a bit cleaner. And we're failing the driver load on error anyways. -Michał > > Regards, > -Jackie
On 03/26/2018 03:04 AM, Michał Winiarski wrote: > On Fri, Mar 23, 2018 at 01:00:35PM -0700, Yaodong Li wrote: >> On 03/23/2018 05:34 AM, Michał Winiarski wrote: >>> We need GuC to load HuC, but it's also possible for GuC to operate on >>> its own. We don't know what the size of HuC FW may be, so, not wanting >>> to make any platform-specific hardcoded guesses, we assume that its size >>> is 0... Which is a very bad approximation. >>> This has a very unfortunate consequence - once we've booted with GuC and >>> no HuC, we'll never be able to load HuC (unless we reboot, since the >>> registers are locked). >>> Rather than using unknown values in our computations - let's partition >>> based on GuC size. >> we can do this based on known GuC and HuC sizes without >> any assumption on FW sizes :) >> please also refer to: https://patchwork.freedesktop.org/series/40541/ > You need to have HuC FW present in the filesystem do do that though. > (IIUC we still get 0 if it's not there) Yes. we cannot make any assumption to the status of the FW files as well. so I think we should expect a system reboot for any FW updates. >>> We have one HW restriction where we're using HuC size (GuC size needs to >>> be roughly similar to HuC size - which may be unknown at this point), >>> luckily, another HW restriction makes it very unlikely to run in any >>> sort of issues in this case. >>> >>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Jackie Li <yaodong.li@intel.com> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++----------------- >>> 1 file changed, 34 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c >>> index 52841d340002..295d302e97b9 100644 >>> --- a/drivers/gpu/drm/i915/intel_wopcm.c >>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c >>> @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd) >>> return 0; >>> } >>> -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm) >>> +static inline void >>> +__guc_region_grow(struct intel_wopcm *wopcm, u32 size) >>> +{ >>> + /* >>> + * We're growing guc region in the direction of lower addresses. >>> + * We need to use multiples of base alignment, because it has more >>> + * strict alignment rules. >>> + */ >>> + size = DIV_ROUND_UP(size, 2); >> A bit confused here. Don't we just want to push the base downward for >> *size* bytes? > Starting from the following: > > +--------------+ > |--------------| > || || > || || > || GuC || > || region || > || || > || || > || || > || || > +--------------+ > | | > | | > | | > | | > | | > | | > | | > | | > | | > +--------------+ > > We want to grow the whole region by size bytes. Pushing the base downward gives > us this: > > +--------------+ > | Empty | > | space | > +--------------+ > || || > || || > || GuC || > || region || > || || > || || > || || > || || > +--------------+ > | | > | | > | | > | | > | | > | | > | | > +--------------+ > > Which leaves less space for HuC (and we're also leaving a bunch of unused space > in our partitioning). > > If we modify both base and size to end up with this: > > +--------------+ > |--------------| > || || > || || > || GuC || > || region || > || || > || || > || || > || || > || || > +--------------+ > | | > | | > | | > | | > | | > | | > | | > | | > +--------------+ > > We're still satisfying the HW restriciton, but we're not wasting any space from > the top (and also we're leaving more space for HuC). > >>> + size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT); >> Hmm, I think we only need align it to 4K boundary. > No - because we're modifying both base (16K alignment) and size (4K > alignment). Got it:-) Thanks! > >>> + >>> + wopcm->guc.base -= size; >>> + wopcm->guc.size += size; >>> +} >>> + >>> +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm) >>> { >>> struct drm_i915_private *i915 = wopcm_to_i915(wopcm); >>> u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw); >>> @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm) >>> size = gen9_size_for_dword_gap_restriction(wopcm->guc.base, >>> wopcm->guc.size); >>> if (size) >>> - goto err; >>> + __guc_region_grow(wopcm, size); >>> size = gen9_size_for_huc_restriction(wopcm->guc.size, >>> huc_fw_size); >> Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case. >> Once the registers were locked, enable_guc=3 may still fail since >> huc_fw_size >> may still large enough to break the restriction we want to enforce that: >> hw_fw_size + 16KB > guc_fw_size. > Well - as mentioned in the commit message, since we also have a "dword gap" > restriction, we're left with pretty large GuC size, which makes this problem > dissaperar for real-world HuC FW sizes. > > But yeah - I agree, we're not able to guarantee this. Hmm, Still I think we shall not make any assumption on this which driver has no control. It seems that partitioning based on real FW sizes + expecting system reboot for FW file changes should be a driver side solution to support enable_guc=1 then enable_guc=3 case. > >>> if (size) >>> - goto err; >>> - } >>> - >>> - return 0; >>> + __guc_region_grow(wopcm, size); >>> -err: >>> - DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n", >>> - wopcm->guc.size / 1024, >>> - size / 1024); >>> - >>> - return -E2BIG; >>> + GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base, >>> + wopcm->guc.size)); >>> + GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size, >>> + huc_fw_size)); >>> + } >>> } >>> static bool wopcm_check_components_fit(struct intel_wopcm *wopcm) >>> @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm) >>> return 0; >>> } >>> -static int wopcm_guc_init(struct intel_wopcm *wopcm) >>> +static int wopcm_guc_region_init(struct intel_wopcm *wopcm) >>> { >>> struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm); >>> - u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw); >>> + u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw); >>> u32 ctx_rsvd = context_reserved_size(dev_priv); >>> - wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size), >>> - GUC_WOPCM_OFFSET_ALIGNMENT); >>> + wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size); >>> - wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd, >>> - PAGE_SIZE); >>> - >>> - DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n", >>> - wopcm->guc.base / 1024, >>> - (wopcm->guc.base + wopcm->guc.size) / 1024); >>> + wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size, >>> + GUC_WOPCM_OFFSET_ALIGNMENT); >> guc.size + ctx_rsvd > wopcm->size check? > It's being done in check_ctx_rsvd_fits later on (in wopcm_check_components_fit). > Hmm, Just worried about wopcm->size - ctx_rsvd - wopcm->guc.size may be incorrect (wrap around) since we don't have any checks on the value of ctx_rsvd + wopcm->guc.size (it might be larger than wopcm size since the checks we've done before could only guarantee wopcm->guc.size < wopcm.size). Regards, -Jackie
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c index 52841d340002..295d302e97b9 100644 --- a/drivers/gpu/drm/i915/intel_wopcm.c +++ b/drivers/gpu/drm/i915/intel_wopcm.c @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd) return 0; } -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm) +static inline void +__guc_region_grow(struct intel_wopcm *wopcm, u32 size) +{ + /* + * We're growing guc region in the direction of lower addresses. + * We need to use multiples of base alignment, because it has more + * strict alignment rules. + */ + size = DIV_ROUND_UP(size, 2); + size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT); + + wopcm->guc.base -= size; + wopcm->guc.size += size; +} + +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm) { struct drm_i915_private *i915 = wopcm_to_i915(wopcm); u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw); @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm) size = gen9_size_for_dword_gap_restriction(wopcm->guc.base, wopcm->guc.size); if (size) - goto err; + __guc_region_grow(wopcm, size); size = gen9_size_for_huc_restriction(wopcm->guc.size, huc_fw_size); if (size) - goto err; - } - - return 0; + __guc_region_grow(wopcm, size); -err: - DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n", - wopcm->guc.size / 1024, - size / 1024); - - return -E2BIG; + GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base, + wopcm->guc.size)); + GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size, + huc_fw_size)); + } } static bool wopcm_check_components_fit(struct intel_wopcm *wopcm) @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm) return 0; } -static int wopcm_guc_init(struct intel_wopcm *wopcm) +static int wopcm_guc_region_init(struct intel_wopcm *wopcm) { struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm); - u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw); + u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw); u32 ctx_rsvd = context_reserved_size(dev_priv); - wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size), - GUC_WOPCM_OFFSET_ALIGNMENT); + wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size); - wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd, - PAGE_SIZE); - - DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n", - wopcm->guc.base / 1024, - (wopcm->guc.base + wopcm->guc.size) / 1024); + wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size, + GUC_WOPCM_OFFSET_ALIGNMENT); return 0; } @@ -255,18 +261,20 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) GEM_BUG_ON(!wopcm->size); - err = wopcm_guc_init(wopcm); + err = wopcm_guc_region_init(wopcm); if (err) return err; - err = wopcm_check_hw_restrictions(wopcm); - if (err) - return err; + wopcm_adjust_for_hw_restrictions(wopcm); err = wopcm_check_components_fit(wopcm); if (err) return err; + DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n", + wopcm->guc.base / 1024, + (wopcm->guc.base + wopcm->guc.size) / 1024); + return 0; }
We need GuC to load HuC, but it's also possible for GuC to operate on its own. We don't know what the size of HuC FW may be, so, not wanting to make any platform-specific hardcoded guesses, we assume that its size is 0... Which is a very bad approximation. This has a very unfortunate consequence - once we've booted with GuC and no HuC, we'll never be able to load HuC (unless we reboot, since the registers are locked). Rather than using unknown values in our computations - let's partition based on GuC size. We have one HW restriction where we're using HuC size (GuC size needs to be roughly similar to HuC size - which may be unknown at this point), luckily, another HW restriction makes it very unlikely to run in any sort of issues in this case. Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jackie Li <yaodong.li@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> --- drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 26 deletions(-)