On 8/15/19 5:21 PM, Michal Wajdeczko wrote:
On Fri, 16 Aug 2019 02:10:26 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> wrote:On 8/15/19 2:48 PM, Michal Wajdeczko wrote:We can do WOPCM partitioning using rough estimates and limits and perform detailed check as separate step. v2: oops! s/max/min Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> --- drivers/gpu/drm/i915/intel_wopcm.c | 105 ++++++++++++++++++++--------- 1 file changed, 74 insertions(+), 31 deletions(-)diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.cindex 2975e00f57f5..39f2764ca3a8 100644 --- a/drivers/gpu/drm/i915/intel_wopcm.c +++ b/drivers/gpu/drm/i915/intel_wopcm.c @@ -87,7 +87,8 @@ void intel_wopcm_init_early(struct intel_wopcm *wopcm) else wopcm->size = GEN9_WOPCM_SIZE; - DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024); + DRM_DEV_DEBUG_DRIVER(i915->drm.dev, "WOPCM: size %uKiB\n", + wopcm->size / SZ_1K); }static inline u32 context_reserved_size(struct drm_i915_private *i915) @@ -138,9 +139,9 @@ static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 huc_fw_size)return 0; } -static inline int check_hw_restriction(struct drm_i915_private *i915, - u32 guc_wopcm_base, u32 guc_wopcm_size, - u32 huc_fw_size) +static inline bool check_hw_restrictions(struct drm_i915_private *i915, + u32 guc_wopcm_base, u32 guc_wopcm_size, + u32 huc_fw_size) { int err = 0;@@ -151,7 +152,64 @@ static inline int check_hw_restriction(struct drm_i915_private *i915, (IS_GEN(i915, 9) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)))err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size); - return err; + return !err; +} ++static inline bool __check_layout(struct drm_i915_private *i915, u32 wopcm_size,+ u32 guc_wopcm_base, u32 guc_wopcm_size, + u32 guc_fw_size, u32 huc_fw_size) +{ + const u32 ctx_rsvd = context_reserved_size(i915); + u32 size; + + if (unlikely(guc_wopcm_base > wopcm_size)) { + dev_err(i915->drm.dev, + "WOPCM: invalid GuC region base: %uK > %uK\n", + guc_wopcm_base / SZ_1K, wopcm_size / SZ_1K); + return false; + } + + size = wopcm_size - ctx_rsvd; + if (unlikely(guc_wopcm_base > size)) { + dev_err(i915->drm.dev, + "WOPCM: invalid GuC region base: %uK > %uK\n", + guc_wopcm_base / SZ_1K, size / SZ_1K); + return false; + } + + if (unlikely(guc_wopcm_size > wopcm_size)) { + dev_err(i915->drm.dev, + "WOPCM: invalid GuC region size: %uK > %uK\n", + guc_wopcm_size / SZ_1K, wopcm_size / SZ_1K); + return false; + } + + size = wopcm_size - guc_wopcm_base - ctx_rsvd; + if (unlikely(guc_wopcm_size > size)) { + dev_err(i915->drm.dev, + "WOPCM: invalid GuC region size: %uK > %uK\n", + guc_wopcm_size / SZ_1K, size / SZ_1K); + return false; + }I think we can consolidate all the checks above in just: wopcm_guc_max = wopcm_size - ctx_rsvd; if (range_overflows(guc_wopcm_base, guc_wopcm_size, wopcm_guc_max) return false;if we consolidate, then it will be hard to tell what went wrong. with separate logs we can find which check failed (they all are unlikely, but still possible)
As long as we print guc_wopcm_base, guc_wopcm_size and wopcm_guc_max on error we should be able to easily see what's going wrong, it's easy to see if guc_wopcm_base or guc_wopcm_size are greater than wopcm_guc_max, which covers 3 of the 4 checks above.
+ + size = guc_fw_size + GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED; + if (unlikely(guc_wopcm_size < size)) { + dev_err(i915->drm.dev, "WOPCM: no space for %s: %uK < %uK\n", + intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_GUC), + guc_wopcm_size / SZ_1K, size / SZ_1K); + return false; + } + + size = huc_fw_size + WOPCM_RESERVED_SIZE; + if (unlikely(guc_wopcm_base < size)) { + dev_err(i915->drm.dev, "WOPCM: no space for %s: %uK < %uK\n", + intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_HUC), + guc_wopcm_base / SZ_1K, size / SZ_1K); + return false; + } + + return check_hw_restrictions(i915, guc_wopcm_base, guc_wopcm_size, + huc_fw_size); } /** @@ -172,8 +230,6 @@ void intel_wopcm_init(struct intel_wopcm *wopcm) u32 ctx_rsvd = context_reserved_size(i915); u32 guc_wopcm_base; u32 guc_wopcm_size; - u32 guc_wopcm_rsvd; - int err; if (!guc_fw_size) return; @@ -183,39 +239,26 @@ void intel_wopcm_init(struct intel_wopcm *wopcm) GEM_BUG_ON(wopcm->guc.size); GEM_BUG_ON(guc_fw_size >= wopcm->size); GEM_BUG_ON(huc_fw_size >= wopcm->size); + GEM_BUG_ON(ctx_rsvd + WOPCM_RESERVED_SIZE >= wopcm->size); if (i915_inject_probe_failure(i915)) return; 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; - } - + guc_wopcm_base = min(wopcm->size - ctx_rsvd, guc_wopcm_base);This line confused me quite a bit until we chatted on IM about it. maybe add a comment, e.g.:/* * we want to keep all the checks in the same place to be able to re-use * them when we find locked values in WOPCM so we don't validate * guc_wopcm_base here, but we still need to clamp it to make sure the * following math is sane. */okAlso, with my suggestion for consolidation above, for the checks we always care about wopcm->size - ctx_rsvd, so maybe store that in a local var to use it here and below and pass that into __check_layout().all math tries to use sizes from the diagram above, introducing one sub-size helper might be over engineering ;)
It just made the code slightly easier to follow IMO by avoiding doing the subtraction in multiple places, but it was just a preference and I'm ok if you prefer to keep it as-is.
Daniele
Danieleguc_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); + DRM_DEV_DEBUG_DRIVER(i915->drm.dev, + "Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n", + guc_wopcm_base / SZ_1K, guc_wopcm_size / SZ_1K); - 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;+ if (__check_layout(i915, wopcm->size, guc_wopcm_base, guc_wopcm_size,+ guc_fw_size, huc_fw_size)) { + wopcm->guc.base = guc_wopcm_base; + wopcm->guc.size = guc_wopcm_size; + GEM_BUG_ON(!wopcm->guc.base); + GEM_BUG_ON(!wopcm->guc.size); } - - err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size, - huc_fw_size); - if (err) - return; - - wopcm->guc.base = guc_wopcm_base; - wopcm->guc.size = guc_wopcm_size; - GEM_BUG_ON(!wopcm->guc.base); - GEM_BUG_ON(!wopcm->guc.size); }
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
