On 02/13/2018 05:18 PM, Yaodong Li wrote:
On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:
+
+static inline u32 lockable_reg_read(struct lockable_reg *lreg)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+ lreg->reg_val = I915_READ(lreg->reg);
+
+ return lreg->reg_val;
+}
+
+static inline bool lockable_reg_validate(struct lockable_reg *lreg,
u32 new_val)
+{
+ GEM_BUG_ON(!lreg->validate);
+
+ return lreg->validate(lreg, lreg->reg_val, new_val);
+}
+
+static inline bool lockable_reg_locked(struct lockable_reg *lreg)
+{
+ u32 reg_val = lockable_reg_read(lreg);
+
+ return reg_val & lreg->lock_bit;
+}
+
+static inline bool lockable_reg_locked_and_valid(struct
lockable_reg *lreg,
+ u32 new_val)
+{
+ if (lockable_reg_locked(lreg)) {
+ if (lockable_reg_validate(lreg, new_val))
+ return true;
+
+ return false;
+ }
+
+ return false;
+}
+
+static inline bool lockable_reg_write(struct lockable_reg *lreg,
u32 val)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+ /*
+ * Write-once register was locked which may happen with either
a faulty
+ * BIOS code or driver module reloading. We should still return
success
+ * for the write if the register was locked with a valid value.
+ */
+ if (lockable_reg_locked(lreg)) {
+ if (lockable_reg_validate(lreg, val))
+ goto out;
+
+ DRM_DEBUG_DRIVER("Register %s was locked with invalid
value\n",
+ lreg->name);
+
+ return false;
+ }
+
+ I915_WRITE(lreg->reg, val);
+
+ if (!lockable_reg_locked_and_valid(lreg, val)) {
+ DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
+ return false;
+ }
As we acknowledge that there are scenarios where registers can be
already
locked, do we really need to make our code so complex ? Maybe
int write_and_verify(struct drm_i915_private *dev_priv,
i915_reg_t reg, u32 value, u32 locked_bit)
{
I915_WRITE(reg, value);
return I915_READ(reg) != (value | locked_bit) ? -EIO : 0;
}
Yes, I agree it's too complex at least for the validation part. Thanks!
My intention was trying to avoid extra write once we found the reg
was locked and to distinguish between faulty SW behavior and
hardware locking error? but now I feel it's not worth it.:-(
Sorry. I regret:-). I think it makes since to use my complex way
to validate the values because of the rsvd bits these regs.
a comparison I915_READ(reg) != (value | locked_bit) cannot
rule out the possibility that these regs were updated by faulty
software code (e.g. BIOS) with these rsvd bit set to random
values. and it's aways better to not make any assumption to
these rsvd bits. so I will still keep using the complex
way (explicitly clear this rsvd bit before comparison) to do this.
As for the lock_bit_check->write->verify sequence v.s. write->verify
sequence I think I can prefer to first one since it would provide
comprehensive info when any error occurs. so let's keep it?:-)
+
+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func) \
+ struct lockable_reg var = { \
+ .name = #rg, \
+ .guc = guc_wopcm_to_guc(guc_wopcm), \
btw, implicit macro params are evil...
Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)
I will avoid this implicit macro.
+ .reg = rg, \
+ .reg_val = 0, \
+ .lock_bit = lb, \
+ .validate = func, \
...and macro names should be always wrapped into ()
Thanks!
Will Add them.
Regards,
-Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
