diff mbox series

drm/i915/gt: debugfs: Evaluate forcewake usage within locks

Message ID 20240607145131.217251-1-andi.shyti@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/gt: debugfs: Evaluate forcewake usage within locks | expand

Commit Message

Andi Shyti June 7, 2024, 2:51 p.m. UTC
The forcewake count and domains listing is multi process critical
and the uncore provides a spinlock for such cases.

Lock the forcewake evaluation section in the fw_domains_show()
debugfs interface.

Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Rodrigo Vivi June 7, 2024, 3:16 p.m. UTC | #1
On Fri, Jun 07, 2024 at 04:51:31PM +0200, Andi Shyti wrote:
> The forcewake count and domains listing is multi process critical
> and the uncore provides a spinlock for such cases.
> 
> Lock the forcewake evaluation section in the fw_domains_show()
> debugfs interface.
> 
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> index 4fcba42cfe34..0437fd8217e0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> @@ -71,6 +71,8 @@ static int fw_domains_show(struct seq_file *m, void *data)
>  	struct intel_uncore_forcewake_domain *fw_domain;
>  	unsigned int tmp;
>  
> +	spin_lock_irq(&uncore->lock);
> +
>  	seq_printf(m, "user.bypass_count = %u\n",
>  		   uncore->user_forcewake_count);
>  
> @@ -79,6 +81,8 @@ static int fw_domains_show(struct seq_file *m, void *data)
>  			   intel_uncore_forcewake_domain_to_str(fw_domain->id),
>  			   READ_ONCE(fw_domain->wake_count));
>  
> +	spin_unlock_irq(&uncore->lock);

I was going to ask to move all of this to a function inside intel_uncore.c
so we keep the lock access in there.... But then I noticed it is already
spread all over :(

Well, perhaps we should start from here to set the precedence and move
things to its own component... but well, I won't block or make it hard,
we do need this change and the overall uncore cleanup could be orthogonal.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +
>  	return 0;
>  }
>  DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(fw_domains);
> -- 
> 2.45.1
>
Nirmoy Das June 10, 2024, 9:24 a.m. UTC | #2
Hi Andi,

On 6/7/2024 4:51 PM, Andi Shyti wrote:
> The forcewake count and domains listing is multi process critical
> and the uncore provides a spinlock for such cases.
>
> Lock the forcewake evaluation section in the fw_domains_show()
> debugfs interface.
>
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>

Needs a Fixes tag, below seems to be correct one.


Fixes: 9dd4b065446a ("drm/i915/gt: Move pm debug files into a gt aware 
debugfs")

Cc: <stable@vger.kernel.org> # v5.6+

Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>


Regards,

Nirmoy


> ---
>   drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> index 4fcba42cfe34..0437fd8217e0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> @@ -71,6 +71,8 @@ static int fw_domains_show(struct seq_file *m, void *data)
>   	struct intel_uncore_forcewake_domain *fw_domain;
>   	unsigned int tmp;
>   
> +	spin_lock_irq(&uncore->lock);
> +
>   	seq_printf(m, "user.bypass_count = %u\n",
>   		   uncore->user_forcewake_count);
>   
> @@ -79,6 +81,8 @@ static int fw_domains_show(struct seq_file *m, void *data)
>   			   intel_uncore_forcewake_domain_to_str(fw_domain->id),
>   			   READ_ONCE(fw_domain->wake_count));
>   
> +	spin_unlock_irq(&uncore->lock);
> +
>   	return 0;
>   }
>   DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(fw_domains);
Andi Shyti June 11, 2024, 1:35 p.m. UTC | #3
Hi Rodrigo,

...

> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > index 4fcba42cfe34..0437fd8217e0 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > @@ -71,6 +71,8 @@ static int fw_domains_show(struct seq_file *m, void *data)
> >  	struct intel_uncore_forcewake_domain *fw_domain;
> >  	unsigned int tmp;
> >  
> > +	spin_lock_irq(&uncore->lock);
> > +
> >  	seq_printf(m, "user.bypass_count = %u\n",
> >  		   uncore->user_forcewake_count);
> >  
> > @@ -79,6 +81,8 @@ static int fw_domains_show(struct seq_file *m, void *data)
> >  			   intel_uncore_forcewake_domain_to_str(fw_domain->id),
> >  			   READ_ONCE(fw_domain->wake_count));
> >  
> > +	spin_unlock_irq(&uncore->lock);
> 
> I was going to ask to move all of this to a function inside intel_uncore.c
> so we keep the lock access in there.... But then I noticed it is already
> spread all over :(

Yeah... maybe some refactoring might be needed there.

> Well, perhaps we should start from here to set the precedence and move
> things to its own component... but well, I won't block or make it hard,
> we do need this change and the overall uncore cleanup could be orthogonal.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks,
Andi
Tvrtko Ursulin June 11, 2024, 1:58 p.m. UTC | #4
On 10/06/2024 10:24, Nirmoy Das wrote:
> Hi Andi,
> 
> On 6/7/2024 4:51 PM, Andi Shyti wrote:
>> The forcewake count and domains listing is multi process critical
>> and the uncore provides a spinlock for such cases.
>>
>> Lock the forcewake evaluation section in the fw_domains_show()
>> debugfs interface.
>>
>> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Needs a Fixes tag, below seems to be correct one.
> 
> 
> Fixes: 9dd4b065446a ("drm/i915/gt: Move pm debug files into a gt aware 
> debugfs")
> 
> Cc: <stable@vger.kernel.org> # v5.6+
> 
> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

What is the back story here and why would it need backporting? IGT cares 
about the atomic view of user_forcewake_count and individual domains or 
what?

Regards,

Tvrtko


> 
> Regards,
> 
> Nirmoy
> 
> 
>> ---
>>   drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c 
>> b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
>> index 4fcba42cfe34..0437fd8217e0 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
>> @@ -71,6 +71,8 @@ static int fw_domains_show(struct seq_file *m, void 
>> *data)
>>       struct intel_uncore_forcewake_domain *fw_domain;
>>       unsigned int tmp;
>> +    spin_lock_irq(&uncore->lock);
>> +
>>       seq_printf(m, "user.bypass_count = %u\n",
>>              uncore->user_forcewake_count);
>> @@ -79,6 +81,8 @@ static int fw_domains_show(struct seq_file *m, void 
>> *data)
>>                  intel_uncore_forcewake_domain_to_str(fw_domain->id),
>>                  READ_ONCE(fw_domain->wake_count));
>> +    spin_unlock_irq(&uncore->lock);
>> +
>>       return 0;
>>   }
>>   DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(fw_domains);
Nirmoy Das June 11, 2024, 2:44 p.m. UTC | #5
On 6/11/2024 3:58 PM, Tvrtko Ursulin wrote:
>
> On 10/06/2024 10:24, Nirmoy Das wrote:
>> Hi Andi,
>>
>> On 6/7/2024 4:51 PM, Andi Shyti wrote:
>>> The forcewake count and domains listing is multi process critical
>>> and the uncore provides a spinlock for such cases.
>>>
>>> Lock the forcewake evaluation section in the fw_domains_show()
>>> debugfs interface.
>>>
>>> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
>>
>> Needs a Fixes tag, below seems to be correct one.
>>
>>
>> Fixes: 9dd4b065446a ("drm/i915/gt: Move pm debug files into a gt 
>> aware debugfs")
>>
>> Cc: <stable@vger.kernel.org> # v5.6+
>>
>> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
>
> What is the back story here and why would it need backporting? IGT 
> cares about the atomic view of user_forcewake_count and individual 
> domains or what?

There is no serious back story. This came from a static code analyzer 
report. I keep forgetting debugfs isn't mounted on production systems so 
we don't have to backport this patch.


Thanks,

Nirmoy

>
> Regards,
>
> Tvrtko
>
>
>>
>> Regards,
>>
>> Nirmoy
>>
>>
>>> ---
>>>   drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c 
>>> b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
>>> index 4fcba42cfe34..0437fd8217e0 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
>>> @@ -71,6 +71,8 @@ static int fw_domains_show(struct seq_file *m, 
>>> void *data)
>>>       struct intel_uncore_forcewake_domain *fw_domain;
>>>       unsigned int tmp;
>>> +    spin_lock_irq(&uncore->lock);
>>> +
>>>       seq_printf(m, "user.bypass_count = %u\n",
>>>              uncore->user_forcewake_count);
>>> @@ -79,6 +81,8 @@ static int fw_domains_show(struct seq_file *m, 
>>> void *data)
>>> intel_uncore_forcewake_domain_to_str(fw_domain->id),
>>>                  READ_ONCE(fw_domain->wake_count));
>>> +    spin_unlock_irq(&uncore->lock);
>>> +
>>>       return 0;
>>>   }
>>>   DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(fw_domains);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
index 4fcba42cfe34..0437fd8217e0 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
@@ -71,6 +71,8 @@  static int fw_domains_show(struct seq_file *m, void *data)
 	struct intel_uncore_forcewake_domain *fw_domain;
 	unsigned int tmp;
 
+	spin_lock_irq(&uncore->lock);
+
 	seq_printf(m, "user.bypass_count = %u\n",
 		   uncore->user_forcewake_count);
 
@@ -79,6 +81,8 @@  static int fw_domains_show(struct seq_file *m, void *data)
 			   intel_uncore_forcewake_domain_to_str(fw_domain->id),
 			   READ_ONCE(fw_domain->wake_count));
 
+	spin_unlock_irq(&uncore->lock);
+
 	return 0;
 }
 DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(fw_domains);