[Intel-gfx] [PATCH v9 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

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