Re: [Intel-gfx] [PATCH v7 1/4] drm/i915: Introduce intel_gt_mcr_lock_sanitize()

[Nirmoy Das <nirmoy.das@linux.intel.com>]

On 9/28/2023 11:42 PM, Andrzej Hajda wrote:
> On 28.09.2023 15:00, Nirmoy Das wrote:
>> Implement intel_gt_mcr_lock_sanitize() to provide a mechanism
>> for cleaning the steer semaphore when absolutely necessary.
>>
>> v2: remove unnecessary lock(Andi, Matt)
>>      improve the kernel doc(Matt)
>>      s/intel_gt_mcr_lock_clear/intel_gt_mcr_lock_sanitize
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 22 ++++++++++++++++++++++
>>   drivers/gpu/drm/i915/gt/intel_gt_mcr.h |  1 +
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c 
>> b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> index bf4a933de03a..326c2ed1d99b 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> @@ -419,6 +419,28 @@ void intel_gt_mcr_unlock(struct intel_gt *gt, 
>> unsigned long flags)
>>           intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
>>   }
>>   +/**
>> + * intel_gt_mcr_lock_sanitize - Sanitize MCR steering lock
>> + * @gt: GT structure
>> + *
>> + * This will be used to sanitize the initial status of the hardware 
>> lock
>> + * during driver load and resume since there won't be any concurrent 
>> access
>> + * from other agents at those times, but it's possible that boot 
>> firmware
>> + * may have left the lock in a bad state.
>> + *
>> + */
>> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt)
>> +{
>> +    /*
>> +     * This gets called at load/resume time, so we shouldn't be
>> +     * racing with other driver threads grabbing the mcr lock.
>> +     */
>> +    lockdep_assert_not_held(&gt->mcr_lock);
>> +
>> +    if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
>> +        intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
>
> I wonder if it wouldn't be useful to check and report if it is locked 
> before unconditional release, no strong feelings.
Not so useful for user but may be as debug log if we need.
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>


Thanks,

Nirmoy

>
> Regards
> Andrzej
>
>
>> +}
>> +
>>   /**
>>    * intel_gt_mcr_read - read a specific instance of an MCR register
>>    * @gt: GT structure
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h 
>> b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
>> index 41684495b7da..01ac565a56a4 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
>> @@ -11,6 +11,7 @@
>>   void intel_gt_mcr_init(struct intel_gt *gt);
>>   void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags);
>>   void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags);
>> +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt);
>>     u32 intel_gt_mcr_read(struct intel_gt *gt,
>>                 i915_mcr_reg_t reg,
>