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@xxxxxxxxx> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > Cc: Jackie Li <yaodong.li@xxxxxxxxx> > > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> > > --- > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
