On 02/09/2018 11:42 AM, Michal Wajdeczko wrote:
+static inline bool __reg_locked(struct drm_i915_private *dev_priv,
+ i915_reg_t reg)
+{
+ /* Set of bit-0 means this write-once register is locked. */
+ return I915_READ(reg) & BIT(0);
+}
Hmm, I'm still not happy as not all registers supports write-once bit.
What about something more generic/robust
static inline bool
__reg_test(struct drm_i915_private *dev_priv, i915_reg_t reg, u32
mask, u32 value)
{
return (I915_READ(reg) & mask) == value;
}
static inline bool
__reg_is_set(struct drm_i915_private *dev_priv, i915_reg_t reg, u32
value)
{
return __reg_test(dev_priv, reg, value, value);
}
...
#define WOPCM_OFFSET_VALID (1<<0)
...
#define WOPCM_LOCKED (1<<0)
...
locked = __reg_is_set(i915, GUC_WOPCM_SIZE, WOPCM_LOCKED);
locked = __reg_is_set(i915, DMA_GUC_WOPCM_OFFSET, WOPCM_OFFSET_VALID);
Feels like a bit over engineering in this way. two reasons:
0) we use this only in this file and with full control over it.
1) All GuC WOPCM related registers are all write-once registers (and
using the same bit
for locking status)
+ /* GuC WOPCM registers should be unlocked at this point. */
+ GEM_BUG_ON(__reg_locked(dev_priv, GUC_WOPCM_SIZE));
+ GEM_BUG_ON(__reg_locked(dev_priv, DMA_GUC_WOPCM_OFFSET));
I'm not sure that GEM_BUG_ON can be used on bits that we don't
fully control
Agreed. at least it's not a GEM Bug.
+}
+
+static inline bool guc_wopcm_regs_valid(struct intel_guc *guc)
can't we always have intel_guc_wopcm as first param ?
I don't think it's necessary for it's a static func plus we would need
another
wopcm_to_guc() with a guc_wopcm first param. we can save some time
with no confusion to external callers in this way. so why not?
+ /* Register locked without valid values. Abort HW init. */
+ return -EINVAL;
I'm not sure that EINVAL is correct error code here ... maybe EACCES ?
hmm, EACCES (-13) always feels like no permission to do this. It's more
like a IO error, so
maybe -EIO and go ahead to trigger GEM warning for this such an error?
since I found following
code in uc_init_hw()
if (GEM_WARN_ON(ret == -EIO))
ret = -EINVAL;
Regards,
-Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
