| Message ID | 20180323123411.3214-4-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/23/2018 05:34 AM, Michał Winiarski wrote: > In the following patches we're going to support constraints checking on > an already locked partitioning. Let's structure the code now to allow > for code reuse and reduce the churn later on. > > 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 | 141 +++++++++++++++++++++++++++---------- > 1 file changed, 102 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c > index 50854a6b9493..52841d340002 100644 > --- a/drivers/gpu/drm/i915/intel_wopcm.c > +++ b/drivers/gpu/drm/i915/intel_wopcm.c > @@ -84,6 +84,17 @@ static inline u32 context_reserved_size(struct drm_i915_private *i915) > return 0; > } > > +static inline u32 guc_fw_size_in_wopcm(u32 guc_fw_size) > +{ > + return ALIGN(guc_fw_size + GUC_WOPCM_RESERVED + > + GUC_WOPCM_STACK_RESERVED, PAGE_SIZE); > +} > + > +static inline u32 huc_fw_size_in_wopcm(u32 huc_fw_size) > +{ > + return huc_fw_size + WOPCM_RESERVED_SIZE; > +} > + > static u32 > gen9_size_for_dword_gap_restriction(u32 guc_wopcm_base, u32 guc_wopcm_size) > { > @@ -121,19 +132,54 @@ gen9_size_for_huc_restriction(u32 guc_wopcm_size, u32 huc_fw_size) > return additional_size; > } > > -static inline int check_hw_restriction(struct drm_i915_private *i915, > - u32 guc_wopcm_base, u32 guc_wopcm_size, > - u32 huc_fw_size) > +static int check_huc_fw_fits(struct intel_wopcm *wopcm, u32 huc_fw_size) inline? > +{ > + if (huc_fw_size_in_wopcm(huc_fw_size) > wopcm->guc.base) { > + DRM_ERROR("Need %uKiB WOPCM for HuC, %uKiB available.\n", > + huc_fw_size_in_wopcm(huc_fw_size) / 1024, > + wopcm->guc.base / 1024); > + return -E2BIG; > + } > + > + return 0; > +} > + > +static int check_guc_fw_fits(struct intel_wopcm *wopcm, u32 guc_fw_size) inline? > +{ > + if (guc_fw_size_in_wopcm(guc_fw_size) > wopcm->guc.size) { > + DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n", > + huc_fw_size_in_wopcm(guc_fw_size) / 1024, > + wopcm->guc.size / 1024); > + return -E2BIG; > + } > + > + return 0; > +} > + > +static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd) inline? > +{ > + if ((wopcm->guc.base + wopcm->guc.size + ctx_rsvd) > wopcm->size) { > + DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n", > + wopcm->guc.base / 1024); > + return -E2BIG; > + } > + > + return 0; > +} > + > +static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm) > { We are repeating the the old discussion over the parameters here. we wanted to keep wopcm contains either valid (non-zero) values or to be zero all the time, so that we can make a check such as if (!wopcm->guc.size) then it's valid. Originally, we needed check this before accessing it in guc_ggtt_offset. Since we are not doing so maybe we can change it back in this way, or just keep it as it is now to continue gaining the benefit that it will continue contains valid data? > + struct drm_i915_private *i915 = wopcm_to_i915(wopcm); > + u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw); > u32 size; > > if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) { > - size = gen9_size_for_dword_gap_restriction(guc_wopcm_base, > - guc_wopcm_size); > + size = gen9_size_for_dword_gap_restriction(wopcm->guc.base, > + wopcm->guc.size); > if (size) > goto err; > > - size = gen9_size_for_huc_restriction(guc_wopcm_size, > + size = gen9_size_for_huc_restriction(wopcm->guc.size, > huc_fw_size); > if (size) > goto err; > @@ -143,12 +189,54 @@ static inline int check_hw_restriction(struct drm_i915_private *i915, > > err: > DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n", > - guc_wopcm_size / 1024, > + wopcm->guc.size / 1024, > size / 1024); > > return -E2BIG; > } > > +static bool wopcm_check_components_fit(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); > + u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw); > + u32 ctx_rsvd = context_reserved_size(i915); > + int err; > + > + err = check_huc_fw_fits(wopcm, huc_fw_size); > + if (err) > + return err; > + > + err = check_guc_fw_fits(wopcm, guc_fw_size); > + if (err) > + return err; > + > + err = check_ctx_rsvd_fits(wopcm, ctx_rsvd); > + if (err) > + return err; > + > + return 0; > +} > + > +static int wopcm_guc_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 ctx_rsvd = context_reserved_size(dev_priv); > + > + wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size), > + GUC_WOPCM_OFFSET_ALIGNMENT); I think we should grow it to the alignment boundary instead of trim it down. > + > + wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd, > + PAGE_SIZE); Hmm, what if wopcm->guc.base + ctx_rsvd > wopcm->size? we have no guarantee on what the value would be after doing an add:) Plus, we should trim it down instead of growing it up:) since we've already got the maximum available size with wopcm->size - wopcm->guc.base - ctx_rsvd. Plus, No PAGE_SIZE since it's not fixed to 4K. > + > + DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n", > + wopcm->guc.base / 1024, > + (wopcm->guc.base + wopcm->guc.size) / 1024); > + > + return 0; > +} > + > /** > * intel_wopcm_init() - Initialize the WOPCM structure. > * @wopcm: pointer to intel_wopcm. > @@ -163,46 +251,21 @@ static inline int check_hw_restriction(struct drm_i915_private *i915, > */ > int intel_wopcm_init(struct intel_wopcm *wopcm) > { > - struct drm_i915_private *i915 = wopcm_to_i915(wopcm); > - u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw); > - u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw); > - u32 ctx_rsvd = context_reserved_size(i915); > - u32 guc_wopcm_base; > - u32 guc_wopcm_size; > - u32 guc_wopcm_rsvd; > int err; > > GEM_BUG_ON(!wopcm->size); > > - guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE, > - GUC_WOPCM_OFFSET_ALIGNMENT); > - if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) { > - DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n", > - guc_wopcm_base / 1024); > - return -E2BIG; > - } > - > - guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd; > - guc_wopcm_size &= GUC_WOPCM_SIZE_MASK; > - > - DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n", > - guc_wopcm_base / 1024, guc_wopcm_size / 1024); > - > - guc_wopcm_rsvd = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED; > - if ((guc_fw_size + guc_wopcm_rsvd) > guc_wopcm_size) { > - DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n", > - (guc_fw_size + guc_wopcm_rsvd) / 1024, > - guc_wopcm_size / 1024); > - return -E2BIG; > - } > + err = wopcm_guc_init(wopcm); > + if (err) > + return err; > > - err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size, > - huc_fw_size); > + err = wopcm_check_hw_restrictions(wopcm); > if (err) > return err; > > - wopcm->guc.base = guc_wopcm_base; > - wopcm->guc.size = guc_wopcm_size; > + err = wopcm_check_components_fit(wopcm); Check fits here seems a little bit overkill, especially for the checks such as check_huc_fw_fit() and check_ctx_rsvd_fit() against the calculated values at this points since these values are actually calculated by the facts that guc_wopcm_base should definitely larger than huc_fw_size since we calculated it with huc_fw_size + WOPCM_RESERVED_SIZE which means check_huc_fw_fits should always returns true and guc_wopcm_size is based on wopcm->size - wopcm->guc.base - ctx_rsvd which means check_ctx_rsvd_fit() will always return true for this check as well. Regards, -Jackie
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c index 50854a6b9493..52841d340002 100644 --- a/drivers/gpu/drm/i915/intel_wopcm.c +++ b/drivers/gpu/drm/i915/intel_wopcm.c @@ -84,6 +84,17 @@ static inline u32 context_reserved_size(struct drm_i915_private *i915) return 0; } +static inline u32 guc_fw_size_in_wopcm(u32 guc_fw_size) +{ + return ALIGN(guc_fw_size + GUC_WOPCM_RESERVED + + GUC_WOPCM_STACK_RESERVED, PAGE_SIZE); +} + +static inline u32 huc_fw_size_in_wopcm(u32 huc_fw_size) +{ + return huc_fw_size + WOPCM_RESERVED_SIZE; +} + static u32 gen9_size_for_dword_gap_restriction(u32 guc_wopcm_base, u32 guc_wopcm_size) { @@ -121,19 +132,54 @@ gen9_size_for_huc_restriction(u32 guc_wopcm_size, u32 huc_fw_size) return additional_size; } -static inline int check_hw_restriction(struct drm_i915_private *i915, - u32 guc_wopcm_base, u32 guc_wopcm_size, - u32 huc_fw_size) +static int check_huc_fw_fits(struct intel_wopcm *wopcm, u32 huc_fw_size) +{ + if (huc_fw_size_in_wopcm(huc_fw_size) > wopcm->guc.base) { + DRM_ERROR("Need %uKiB WOPCM for HuC, %uKiB available.\n", + huc_fw_size_in_wopcm(huc_fw_size) / 1024, + wopcm->guc.base / 1024); + return -E2BIG; + } + + return 0; +} + +static int check_guc_fw_fits(struct intel_wopcm *wopcm, u32 guc_fw_size) +{ + if (guc_fw_size_in_wopcm(guc_fw_size) > wopcm->guc.size) { + DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n", + huc_fw_size_in_wopcm(guc_fw_size) / 1024, + wopcm->guc.size / 1024); + return -E2BIG; + } + + return 0; +} + +static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd) +{ + if ((wopcm->guc.base + wopcm->guc.size + ctx_rsvd) > wopcm->size) { + DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n", + wopcm->guc.base / 1024); + return -E2BIG; + } + + return 0; +} + +static int wopcm_check_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); u32 size; if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) { - size = gen9_size_for_dword_gap_restriction(guc_wopcm_base, - guc_wopcm_size); + size = gen9_size_for_dword_gap_restriction(wopcm->guc.base, + wopcm->guc.size); if (size) goto err; - size = gen9_size_for_huc_restriction(guc_wopcm_size, + size = gen9_size_for_huc_restriction(wopcm->guc.size, huc_fw_size); if (size) goto err; @@ -143,12 +189,54 @@ static inline int check_hw_restriction(struct drm_i915_private *i915, err: DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n", - guc_wopcm_size / 1024, + wopcm->guc.size / 1024, size / 1024); return -E2BIG; } +static bool wopcm_check_components_fit(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); + u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw); + u32 ctx_rsvd = context_reserved_size(i915); + int err; + + err = check_huc_fw_fits(wopcm, huc_fw_size); + if (err) + return err; + + err = check_guc_fw_fits(wopcm, guc_fw_size); + if (err) + return err; + + err = check_ctx_rsvd_fits(wopcm, ctx_rsvd); + if (err) + return err; + + return 0; +} + +static int wopcm_guc_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 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 = 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); + + return 0; +} + /** * intel_wopcm_init() - Initialize the WOPCM structure. * @wopcm: pointer to intel_wopcm. @@ -163,46 +251,21 @@ static inline int check_hw_restriction(struct drm_i915_private *i915, */ int intel_wopcm_init(struct intel_wopcm *wopcm) { - struct drm_i915_private *i915 = wopcm_to_i915(wopcm); - u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw); - u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw); - u32 ctx_rsvd = context_reserved_size(i915); - u32 guc_wopcm_base; - u32 guc_wopcm_size; - u32 guc_wopcm_rsvd; int err; GEM_BUG_ON(!wopcm->size); - guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE, - GUC_WOPCM_OFFSET_ALIGNMENT); - if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) { - DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n", - guc_wopcm_base / 1024); - return -E2BIG; - } - - guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd; - guc_wopcm_size &= GUC_WOPCM_SIZE_MASK; - - DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n", - guc_wopcm_base / 1024, guc_wopcm_size / 1024); - - guc_wopcm_rsvd = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED; - if ((guc_fw_size + guc_wopcm_rsvd) > guc_wopcm_size) { - DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n", - (guc_fw_size + guc_wopcm_rsvd) / 1024, - guc_wopcm_size / 1024); - return -E2BIG; - } + err = wopcm_guc_init(wopcm); + if (err) + return err; - err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size, - huc_fw_size); + err = wopcm_check_hw_restrictions(wopcm); if (err) return err; - wopcm->guc.base = guc_wopcm_base; - wopcm->guc.size = guc_wopcm_size; + err = wopcm_check_components_fit(wopcm); + if (err) + return err; return 0; }
In the following patches we're going to support constraints checking on an already locked partitioning. Let's structure the code now to allow for code reuse and reduce the churn later on. 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 | 141 +++++++++++++++++++++++++++---------- 1 file changed, 102 insertions(+), 39 deletions(-)