Re: [PATCH 2/5] drm/i915/wopcm: Check WOPCM layout separately from calculations

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 8/15/19 5:21 PM, Michal Wajdeczko wrote:
> On Fri, 16 Aug 2019 02:10:26 +0200, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@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@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Chris Wilson <chris@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)
> 

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.
>>   */
> 
> 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 ;)
> 

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

>>
>> 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);
>>>   }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx