[Intel-gfx] [PATCH 2/5] drm/i915/wopcm: Check WOPCM layout separately from calculations
Michal Wajdeczko
michal.wajdeczko at intel.com
Fri Aug 16 00:21:53 UTC 2019
On Fri, 16 Aug 2019 02:10:26 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio at intel.com> 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 at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>> 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.c
>> index 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)
>
>
>> +
>> + 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.
> */
ok
>
> Also, 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 ;)
>
> Daniele
>
>> 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);
>> + 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);
>> }
More information about the Intel-gfx
mailing list