[Intel-gfx] [PATCH v6 1/4] drm/i915: Introduce intel_gt_mcr_lock_reset()

[Nirmoy Das]

On 9/28/2023 12:23 AM, Matt Roper wrote:
> On Wed, Sep 27, 2023 at 11:03:54PM +0200, Nirmoy Das wrote:
>> Implement intel_gt_mcr_lock_reset() to provide a mechanism
>> for resetting the steer semaphore when absolutely necessary.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 29 ++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/gt/intel_gt_mcr.h |  1 +
>>   2 files changed, 30 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..d98e0d2fc2ee 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> @@ -419,6 +419,35 @@ 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_reset - Reset MCR steering lock
>> + * @gt: GT structure
>> + *
>> + * Performs a steer semaphore reset operation. On MTL and beyond, a hardware
>> + * lock will also be taken to serialize access not only for the driver,
>> + * but also for external hardware and firmware agents.
> The text here makes it sound like this reset function is going to take
> the lock.  Since we have the same language in the lock() function's
> kerneldoc, I think you can just delete this whole sentence.
>
>> + * However, there may be situations where the driver must reset the semaphore
>> + * but only when it is absolutely certain that no other agent should own the
>> + * lock at that given time.
> This part leads to questions about what such situations would be and how
> we'd know it's safe to use.  Maybe it's best to just say something like
> "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."
sounds better than mine. I will just keep that as description and remove 
rest.
>
>> + *
>> + * Context: Takes gt->mcr_lock.  uncore->lock should *not* be held when this
>> + *          function is called, although it may be acquired after this
>> + *          function call.
>> + */
>> +void intel_gt_mcr_lock_reset(struct intel_gt *gt)
>> +{
>> +	unsigned long __flags;
>> +
>> +	lockdep_assert_not_held(&gt->uncore->lock);
>> +
>> +	spin_lock_irqsave(&gt->mcr_lock, __flags);
> If we're doing this to sanitize at load/resume, then presumably we
> shouldn't ever be racing with other driver threads either, right?


Driver load and suspend/resume is single threaded afaik but I think I 
just needed double conformation.

Will remove the lock.


Thanks,

Nirmoy

>   If it
> was possible for some other thread to already be grabbing the MCR lock,
> then that would mean it also isn't safe for us to reset it here either.
>
>
> Matt
>
>> +
>> +	if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
>> +		intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
>> +
>> +	spin_unlock_irqrestore(&gt->mcr_lock, __flags);
>> +}
>> +
>>   /**
>>    * 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..485c7711f2e8 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_reset(struct intel_gt *gt);
>>   
>>   u32 intel_gt_mcr_read(struct intel_gt *gt,
>>   		      i915_mcr_reg_t reg,
>> -- 
>> 2.41.0
>>