[Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Apr 19 16:37:45 UTC 2018
On Mon, 16 Apr 2018 20:43:39 +0200, Yaodong Li <yaodong.li at 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 at 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 at intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>> Cc: Michal Winiarski <michal.winiarski at intel.com>
>>> Cc: John Spotswood <john.a.spotswood at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at 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
More information about the Intel-gfx
mailing list