diff mbox series

[v6,1/4] drm/i915: Introduce intel_gt_mcr_lock_reset()

Message ID 20230927210357.17461-1-nirmoy.das@intel.com (mailing list archive)
State New, archived
Headers show
Series [v6,1/4] drm/i915: Introduce intel_gt_mcr_lock_reset() | expand

Commit Message

Nirmoy Das Sept. 27, 2023, 9:03 p.m. UTC 
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@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(+)

Comments

Matt Roper Sept. 27, 2023, 10:23 p.m. UTC | #1 
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@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."

> + *
> + * 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?  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
>
Andi Shyti Sept. 28, 2023, 7:18 a.m. UTC | #2 
Hi Nirmoy,

your client is still missing my e-mails? :)

> +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 (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);

I commented here that probably we don't need the locks. And I
asked you whether you had any reason for locking here.

Andi

> +}
Nirmoy Das Sept. 28, 2023, 8:15 a.m. UTC | #3 
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@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
>>
Nirmoy Das Sept. 28, 2023, 8:19 a.m. UTC | #4 
On 9/28/2023 9:18 AM, Andi Shyti wrote:
> Hi Nirmoy,
>
> your client is still missing my e-mails? :)

I did reply with a question!


>
>> +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 (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);
> I commented here that probably we don't need the locks. And I
> asked you whether you had any reason for locking here.


I asked the question back to you and I send this before you could reply.

Will remove the lock.


Thanks,

Nirmoy

>
> Andi
>
>> +}
diff mbox series

Patch

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.
+ * 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.
+ *
+ * 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 (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,