Re: [PATCH] drm/i915/gt: debugfs: Evaluate forcewake usage within locks

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
>