[Intel-gfx] [PATCH 2/5] drm/i915/wopcm: Check WOPCM layout separately from calculations
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Fri Aug 16 00:28:34 UTC 2019
On 8/15/19 5:21 PM, Michal Wajdeczko wrote:
> 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)
>
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);
>>> }
More information about the Intel-gfx
mailing list