[Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values
Yaodong Li
yaodong.li at intel.com
Mon Apr 16 18:43:39 UTC 2018
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. 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)?
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.
>
>> + 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.
>
>> + */
>> + 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? 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