Re: [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

[Yaodong Li <yaodong.li@intel.com>]

On 04/19/2018 09:37 AM, Michal Wajdeczko wrote:
> On Mon, 16 Apr 2018 20:43:39 +0200, Yaodong Li <yaodong.li@intel.com> 
> wrote:
>
>> On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
>>> On Tue, 10 Apr 2018 02:42:19 +0200, Jackie Li <yaodong.li@intel.com> 
>>> wrote:
>>>
>>>> In current code, we only compare the locked WOPCM register values 
>>>> with the
>>>> calculated values. However, we can continue loading GuC/HuC 
>>>> firmware if the
>>>> locked (or partially locked) values were valid for current GuC/HuC 
>>>> firmware
>>>> sizes.
>>>>
>>>> This patch added a new code path to verify whether the locked register
>>>> values can be used for GuC/HuC firmware loading, it will 
>>>> recalculate the
>>>> verify the new values if these registers were partially locked, so 
>>>> that we
>>>> won't fail the GuC/HuC firmware loading even if the locked register 
>>>> values
>>>> are different from the calculated ones.
>>>>
>>>> v2:
>>>>  - Update WOPCM register only if it's not locked
>>>>
>>>> Signed-off-by: Jackie Li <yaodong.li@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>>>> Cc: John Spotswood <john.a.spotswood@intel.com>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_wopcm.c | 217 
>>>> +++++++++++++++++++++++++++++++------
>>>>  1 file changed, 185 insertions(+), 32 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
>>>> b/drivers/gpu/drm/i915/intel_wopcm.c
>>>> index b1c08ca..fa8d2be 100644
>>>> --- a/drivers/gpu/drm/i915/intel_wopcm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
>>>> @@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct 
>>>> drm_i915_private *i915,
>>>>      return err;
>>>>  }
>>>> +static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
>>>> +{
>>>> +    return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
>>>> +             GUC_WOPCM_OFFSET_ALIGNMENT);
>>>> +}
>>>> +
>>>> +static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
>>>> +{
>>>> +    return guc_fw_size + GUC_WOPCM_RESERVED + 
>>>> GUC_WOPCM_STACK_RESERVED;
>>>> +}
>>>> +
>>>> +static inline int calculate_max_guc_wopcm_size(struct intel_wopcm 
>>>> *wopcm,
>>>> +                           u32 guc_wopcm_base,
>>>> +                           u32 *guc_wopcm_size)
>>>
>>> Can't we just return this size as positive value?
>>> I guess wopcm size will never be larger than MAX_INT.
>>> We can add GEM_BUG_ON to be sure.
>>>
>>>> +{
>>>> +    struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>>>> +    u32 ctx_rsvd = context_reserved_size(i915);
>>>> +
>>>> +    if (guc_wopcm_base >= wopcm->size ||
>>>> +        (guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
>>>> +        DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
>>>> +              guc_wopcm_base / 1024);
>>>> +        return -E2BIG;
>>>
>>> You are mixing calculations with verifications here.
>>> Focus on calculations as you have separate function that verifies 
>>> values.
>>>
>>>> +    }
>>>> +
>>>> +    *guc_wopcm_size =
>>>> +        (wopcm->size - guc_wopcm_base - ctx_rsvd) & 
>>>> GUC_WOPCM_SIZE_MASK;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static inline int verify_calculated_values(struct drm_i915_private
>>>
>>> hmm, maybe we can unify somehow this verification to be able to work 
>>> with
>>> both calculated and locked values?
>> I actually thought about that. However, since the verification based 
>> on the assumption
>> that the unlocked reg value could be any numbers so it puts more 
>> checks on these values
>> which are unnecessary during the calculation.
>
> maybe some checks would be "unnecessary" but they still should be valid.
> and code reuse is nice benefit that sacrifice that few extra checks.
Hmm, actually there's no duplicated code here. But I agree that it will 
be clearer to
have single place for the verification - it makes sense too:). So will 
choose to
sacrifice a little bit time here for unnecessary checks.
>
>> I've made the responding check common
>> enough to be reused by this verification code and the calculation code.
>>>
>>>> *i915,
>>>> +                       u32 guc_fw_size, u32 huc_fw_size,
>>>> +                       u32 guc_wopcm_base,
>>>> +                       u32 guc_wopcm_size)
>>>> +{
>>>> +    if (guc_wopcm_size < calculate_min_guc_wopcm_size(guc_fw_size)) {
>>>> +        DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
>>>> +              calculate_min_guc_wopcm_size(guc_fw_size),
>>>
>>> you are calling calculate_min_guc_wopcm_size() twice
>> Will update it.
>>>
>>>> +              guc_wopcm_size / 1024);
>>>> +        return -E2BIG;
>>>> +    }
>>>> +
>>>> +    return check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
>>>> +                    huc_fw_size);
>>>> +}
>>>> +
>>>>  /**
>>>>   * intel_wopcm_init() - Initialize the WOPCM structure.
>>>>   * @wopcm: pointer to intel_wopcm.
>>>> @@ -157,10 +204,8 @@ 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);
>>>> @@ -177,35 +222,121 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>>>>          return -E2BIG;
>>>>      }
>>>> -    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);
>>>> +    guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
>>>> +    err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
>>>> +                       &guc_wopcm_size);
>>>> +    if (err)
>>>> +        return err;
>>>> +
>>>> +    DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
>>>> +             guc_wopcm_base / 1024,
>>>> +             (guc_wopcm_base + guc_wopcm_size) / 1024);
>>>> +
>>>> +    err = verify_calculated_values(i915, guc_fw_size, huc_fw_size,
>>>> +                       guc_wopcm_base, guc_wopcm_size);
>>>
>>> If we want to support scenario where wopcm values are already 
>>> locked, maybe
>>> we should check first for them, as if they are really locked, we should
>>> avoid calculating and printing our values...
>> I think we shall not access registers in early init (by 
>> i915_driver_load design purpose)?
>
> but we don't have to do it in early, intel_wopmc_init[_hw]() should be ok
if we want to do so then intel_wopcm_init_hw should be the right place. 
Just that these changes
is only for handling the worst cases that rarely happens and it is 
unlikely to be called. The benefit
to distinguish early init and hw init is that we can do the calculation 
without powering on the HW
which should be expected as a common use case.
>
>> and the reason we need to put the calculation and locking check + reg 
>> update into
>> separate functions is we only want to calculate the values for once 
>> and locking check
>> and reg update should be done every time hw init is called.
>>
>> The purpose of this patch is only to take care of worst cases that 
>> BIOS had locked these
>> regs and we still want give it a try if we could go with these 
>> values. and it should be very
>> unlikely.
>
> hmm, I was assuming that you want to cover the driver reload scenario
> loaded with different guc/huc fw sizes, which is likely scenario during
> debugging (see your patch 1 motivation)
Yes, this is one of the motivations too. e.g. without this patch, we 
will have to
reboot the system even if for use case such as we are trying to reload a 
smaller
guc fw. This patch will enable such scenarios when it's possible.
>
>>>
>>>> +    if (err)
>>>> +        return err;
>>>> +
>>>> +    wopcm->guc.base = guc_wopcm_base;
>>>> +    wopcm->guc.size = guc_wopcm_size;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static inline int verify_locked_values(struct intel_wopcm *wopcm,
>>>> +                       u32 guc_wopcm_base, u32 guc_wopcm_size)
>>>
>>> hmm, either pass locked values in 'the' wopcm structure, or use i915
>>> parameter to avoid confusion.
>> we are trying to keep the wopcm structure clean (only contains valid 
>> values).
>>>
>>>> +{
>>>> +    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);
>>>> +
>>>> +    /*
>>>> +     * Locked GuC WOPCM base and size could be any values which 
>>>> are large
>>>> +     * enough to lead to a wrap around after performing add 
>>>> operations.
>>>
>>> maybe simpler and more robust way would be to just use u64 for 
>>> calculations?
>> Hmm. These values are for 32-bit regs, so even with u64 variables we 
>> still need
>> similar checks to keep them to be 32-bit values. e.g. we need 
>> guarantee that
>> wopcm->size is within 32-bit boundary.
>
> I was referring to use u64 vars for calculations, not inside wopcm.
> and we already have many checks so we should catch values over 32b
>
> Just checking guc_base/size against wopcm size may be insufficient:
>
>     wopcm->size    = 0xF0000000;
>     guc_wopcm_base = 0x80000000;
>     guc_wopcm_size = 0x80000000;
>
my v4 patch did some simplification to these checks.
>>>
>>>> +     */
>>>> +    if (guc_wopcm_base >= wopcm->size) {
>>>> +        DRM_ERROR("Locked base value %uKiB >= WOPCM size %uKiB.\n",
>>>> +              guc_wopcm_base / 1024,
>>>> +              wopcm->size / 1024);
>>>>          return -E2BIG;
>>>>      }
>>>> -    guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
>>>> -    guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
>>>> +    if (guc_wopcm_size >= wopcm->size) {
>>>> +        DRM_ERROR("Locked size value %uKiB >= WOPCM size %uKiB.\n",
>>>> +              guc_wopcm_base / 1024,
>>>> +              wopcm->size / 1024);
>>>> +        return -E2BIG;
>>>> +    }
>>>> -    DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
>>>> -             guc_wopcm_base / 1024, guc_wopcm_size / 1024);
>>>> +    if (guc_wopcm_base + guc_wopcm_size + ctx_rsvd > wopcm->size) {
>>>> +        DRM_ERROR("Need %uKiB WOPCM in total, %uKiB available.\n",
>>>> +              (guc_wopcm_base + guc_wopcm_size + ctx_rsvd) / 1024,
>>>> +              (wopcm->size) / 1024);
>>>> +        return -E2BIG;
>>>> +    }
>>>> -    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);
>>>> +    if (guc_wopcm_base < calculate_min_guc_wopcm_base(huc_fw_size)) {
>>>> +        DRM_ERROR("Need %uKiB WOPCM for HuC FW, %uKiB available.\n",
>>>> +              calculate_min_guc_wopcm_base(huc_fw_size) / 1024,
>>>
>>> calculate_min_guc_wopcm_base() called twice
>>>
>>>> +              guc_wopcm_base / 1024);
>>>>          return -E2BIG;
>>>>      }
>>>> -    err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
>>>> -                   huc_fw_size);
>>>> +    return verify_calculated_values(i915, guc_fw_size, huc_fw_size,
>>>> +                    guc_wopcm_base, guc_wopcm_size);
>>>> +}
>>>> +
>>>> +static inline int verify_locked_values_and_update(struct intel_wopcm
>>>
>>> "update" what ?
>>>
>>>> *wopcm,
>>>> +                          bool *update_size_reg,
>>>> +                          bool *update_offset_reg)
>>>> +{
>>>> +    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 size_val = I915_READ(GUC_WOPCM_SIZE);
>>>> +    u32 offset_val = I915_READ(DMA_GUC_WOPCM_OFFSET);
>>>> +    bool offset_reg_locked = offset_val & GUC_WOPCM_OFFSET_VALID;
>>>> +    bool size_reg_locked = size_val & GUC_WOPCM_SIZE_LOCKED;
>>>> +    u32 guc_wopcm_base = offset_val & GUC_WOPCM_OFFSET_MASK;
>>>> +    u32 guc_wopcm_size = size_val & GUC_WOPCM_SIZE_MASK;
>>>> +    int err;
>>>> +
>>>> +    *update_size_reg = !size_reg_locked;
>>>> +    *update_offset_reg = !offset_reg_locked;
>>>> +
>>>> +    if (!offset_reg_locked && !size_reg_locked)
>>>> +        return 0;
>>>> +
>>>> +    if (guc_wopcm_base == wopcm->guc.base &&
>>>> +        guc_wopcm_size == wopcm->guc.size)
>>>> +        return 0;
>>>> +
>>>> +    if (USES_HUC(dev_priv) && offset_reg_locked &&
>>>> +        !(offset_val & HUC_LOADING_AGENT_GUC)) {
>>>> +        DRM_ERROR("HUC_LOADING_AGENT_GUC isn't locked for 
>>>> USES_HUC.\n");
>>>> +        return -EIO;
>>>> +    }
>>>> +
>>>> +    if (!offset_reg_locked)
>>>> +        guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
>>>> +
>>>> +    if (!size_reg_locked) {
>>>> +        err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
>>>> +                           &guc_wopcm_size);
>>>> +        if (err)
>>>> +            return err;
>>>> +    }
>>>> +
>>>> +    DRM_DEBUG_DRIVER("Recalculated GuC WOPCM Region: [%uKiB, 
>>>> %uKiB)\n",
>>>> +             guc_wopcm_base / 1024,
>>>> +             (guc_wopcm_base + guc_wopcm_size) / 1024);
>>>> +
>>>> +    err = verify_locked_values(wopcm, guc_wopcm_base, 
>>>> guc_wopcm_size);
>>>>      if (err)
>>>>          return err;
>>>> -    wopcm->guc.base = guc_wopcm_base;
>>>>      wopcm->guc.size = guc_wopcm_size;
>>>> +    wopcm->guc.base = guc_wopcm_base;
>>>
>>> btw, setting base then size is the correct order ;)
>>>
>>>>     return 0;
>>>>  }
>>>> @@ -233,11 +364,14 @@ static inline int write_and_verify(struct 
>>>> drm_i915_private *dev_priv,
>>>>   * will verify the register values to make sure the registers are 
>>>> locked with
>>>>   * correct values.
>>>>   *
>>>> - * Return: 0 on success. -EIO if registers were locked with 
>>>> incorrect values.
>>>> + * Return: 0 on success. Minus error code if registered were 
>>>> locked with
>>>> + * incorrect values.-EIO if registers failed to lock with correct 
>>>> values.
>>>
>>> please fix typos and spaces.
>>> btw, do we really care about different error codes?
>>> note that -EIO has special meaning during driver load.
>>>
>> I think is better to return different err codes to distinguish 
>> different errors.
>>
>> Will EIO will be handled different by i915_driver_load?
>
> yes, it is handled differently.
Can you tell any negative impact if EIO was used? I can switch to EBUSY 
instead
but I think it's more accurate to use EIO for errors like reg is locked 
without HUC_AGENT
bit set when using HUC.

Thanks,
-Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx