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

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

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.

> 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

> 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)

>>
>>> +    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;

>>
>>> +     */
>>> +    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.

> Another candidate would
> be EBUSY.
>>>   */
>>>  int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>>>  {
>>>      struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
>>> +    bool update_size_reg;
>>> +    bool update_offset_reg;
>>>      int err;
>>>     if (!USES_GUC(dev_priv))
>>> @@ -247,27 +381,46 @@ int intel_wopcm_init_hw(struct intel_wopcm  
>>> *wopcm)
>>>      GEM_BUG_ON(!wopcm->guc.size);
>>>      GEM_BUG_ON(!wopcm->guc.base);
>>> -    err = write_and_verify(dev_priv, GUC_WOPCM_SIZE, wopcm->guc.size,
>>> -                   GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
>>> -                   GUC_WOPCM_SIZE_LOCKED);
>>> -    if (err)
>>> +    err = verify_locked_values_and_update(wopcm, &update_size_reg,
>>> +                          &update_offset_reg);
>>> +    if (err) {
>>> +        DRM_ERROR("WOPCM registers are locked with invalid  
>>> values.\n");
>>>          goto err_out;
>>> +    }
>>> -    err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
>>> -                   wopcm->guc.base | HUC_LOADING_AGENT_GUC,
>>> -                   GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC |
>>> -                   GUC_WOPCM_OFFSET_VALID,
>>> -                   GUC_WOPCM_OFFSET_VALID);
>>> -    if (err)
>>> -        goto err_out;
>>> +    if (update_size_reg) {
>>> +        err = write_and_verify(dev_priv, GUC_WOPCM_SIZE,
>>> +                       wopcm->guc.size,
>>> +                       GUC_WOPCM_SIZE_MASK |
>>> +                       GUC_WOPCM_SIZE_LOCKED,
>>> +                       GUC_WOPCM_SIZE_LOCKED);
>>> +        if (err) {
>>> +            DRM_ERROR("Failed to GuC WOPCM size with %#x.\n",
>>> +                  wopcm->guc.size);
>>> +            goto err_out;
>>> +        }
>>> +    }
>>> +    if (update_offset_reg) {
>>> +        err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
>>> +                       wopcm->guc.base | HUC_LOADING_AGENT_GUC,
>>> +                       GUC_WOPCM_OFFSET_MASK |
>>> +                       HUC_LOADING_AGENT_GUC |
>>> +                       GUC_WOPCM_OFFSET_VALID,
>>> +                       GUC_WOPCM_OFFSET_VALID);
>>> +        if (err) {
>>> +            DRM_ERROR("Failed to GuC WOPCM offset with %#x.\n",
>>> +                  wopcm->guc.base);
>>> +            goto err_out;
>>> +        }
>>> +    }
>>>      return 0;
>>> err_out:
>>> -    DRM_ERROR("Failed to init WOPCM registers:\n");
>>>      DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
>>>            I915_READ(DMA_GUC_WOPCM_OFFSET));
>>>      DRM_ERROR("GUC_WOPCM_SIZE=%#x\n", I915_READ(GUC_WOPCM_SIZE));
>>> +    DRM_ERROR("A system reboot is required.\n");
>>>     return err;
>>>  }
>>
>> as mentioned above, maybe we should start with:
>>
>>     static int wopcm_init_from_registers(wopcm)
>>     {
>>         u32 base = I915_READ();
>>         u32 size = I915_READ();
>>
>>         wopcm->guc.base = base & LOCKED ? base & MASK : 0;
>>         wopcm->guc.size = size & LOCKED ? size & MASK : 0;
>>
>>         return wopcm->guc.base && wopcm->guc.size ? 0 : -ENODATA;
>>     }
>>
>> then we can do everything in one pass:
>>
>>     int intel_wopcm_init(wopcm)
>>     {
>>         ret = wopcm_init_from_registers(wopcm);
>>         if (ret)
>>             wopcm_finish_partitioning(wopcm);
>>
>>         ret = wopcm_verify_partitions(wopcm);
>>         if (ret)
>>             return ret;
>>
>>         ret = wopcm_write_registers(wopcm);
>>         if (ret)
>>             return ret;
>>         return 0;
>>     }
>>
>> where "wopcm_finish_partitioning" must not try to update any non-zero
>> values but then can be implemented in any fashion.
>>
>> (btw, I guess we don't need separate wopcm_init_hw step)
> As I mentioned above, the main reason to this separation is because
> we only expect one time calculation while the locking check and reg  
> update
> may happen every time hw init was called. e.g. resume.
>
> Thanks,
> -Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx