Re: [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li <yaodong.li@intel.com>]

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.:-(
>> +
>> +
>> +#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:-)
>> +        .reg = rg,    \
>> +        .reg_val = 0,    \
>> +        .lock_bit = lb,    \
>> +        .validate = func,    \
>
> ...and macro names should be always wrapped into ()
>
Thanks!
>> diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h 
>> b/drivers/gpu/drm/i915/intel_guc_wopcm.h
>> index 13fcab6..89dd44c 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
>> @@ -66,7 +66,8 @@ struct intel_guc;
>>   * @offset: GuC WOPCM offset from the WOPCM base.
>>   * @size: size of GuC WOPCM for GuC firmware.
>>   * @top: start of the non-GuC WOPCM memory.
>> - * @valid: whether this structure contains valid (1-valid, 
>> 0-invalid) info.
>> + * @valid: whether the values in this struct are valid.
>> + * @load_huc_fw: whether need to configure GuC to load HuC firmware.
>
> I'm not sure that we need to track this flag inside structure.
> It is just a parameter for doing partitioning and final check.
>
I think it's related to actual reg configuration. Any suggestions since
we do need it in hw_init to setup offset reg?
> As mentioned before, we can avoid this flag and "valid" flag if do
> partitioning and validation *before* writing final results to the
> struct.
In current code, we do verify the ggtt offset against wopcm top in our 
current code which means
current code won't trust the fact that ggtt offset would never be used 
after uc/guc init failed.
This is the reason for this valid bit (which clearly suggests the struct 
is ready to use) - I won't
assume the ggtt_offset would never be called even if the uc/guc_init 
returned failure.


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx