On 04/19/2018 09:37 AM, Michal Wajdeczko wrote:
On Mon, 16 Apr 2018 20:43:39 +0200, Yaodong Li <yaodong.li@xxxxxxxxx>
wrote:
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.
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
