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