On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:19 +0200, Jackie Li <yaodong.li@xxxxxxxxx>
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@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx>
Cc: John Spotswood <john.a.spotswood@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
---
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
