Re: [PATCH v2 01/12] drm/i915: Move locking and unclaimed check into mmio_debug_{suspend, resume}

From: Matt Roper <matthew.d.roper@intel.com>
To: "Ruhl, Michael J" <michael.j.ruhl@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Sripada, Radhakrishna" <radhakrishna.sripada@intel.com>
Subject: Re: [PATCH v2 01/12] drm/i915: Move locking and unclaimed check into mmio_debug_{suspend, resume}
Date: Tue, 6 Sep 2022 10:08:37 -0700	[thread overview]
Message-ID: <Yxd+lQRGriwsyJjX@mdroper-desk1.amr.corp.intel.com> (raw)
In-Reply-To: <DM5PR11MB13245A45A7277B9548CADFB5C17E9@DM5PR11MB1324.namprd11.prod.outlook.com>

On Tue, Sep 06, 2022 at 06:39:14AM -0700, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> >Matt Roper
> >Sent: Friday, September 2, 2022 7:33 PM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: dri-devel@lists.freedesktop.org; Sripada, Radhakrishna
> ><radhakrishna.sripada@intel.com>
> >Subject: [PATCH v2 01/12] drm/i915: Move locking and unclaimed check into
> >mmio_debug_{suspend, resume}
> >
> >Moving the locking for MMIO debug (and the final check for unclaimed
> >accesses when resuming debug after a userspace-initiated forcewake) will
> >make it simpler to completely skip MMIO debug handling on uncores that
> >don't support it in future patches.
> >
> >Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> >---
> > drivers/gpu/drm/i915/intel_uncore.c | 41 +++++++++++++++--------------
> > 1 file changed, 21 insertions(+), 20 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> >b/drivers/gpu/drm/i915/intel_uncore.c
> >index 9b81b2543ce2..e717ea55484a 100644
> >--- a/drivers/gpu/drm/i915/intel_uncore.c
> >+++ b/drivers/gpu/drm/i915/intel_uncore.c
> >@@ -50,23 +50,33 @@ intel_uncore_mmio_debug_init_early(struct
> >intel_uncore_mmio_debug *mmio_debug)
> > 	mmio_debug->unclaimed_mmio_check = 1;
> > }
> >
> >-static void mmio_debug_suspend(struct intel_uncore_mmio_debug
> >*mmio_debug)
> >+static void mmio_debug_suspend(struct intel_uncore *uncore)
> 
> /bike-shedding...
> 
> It seems like there has been a tend to name functions with the
> 
> _unlocked
> 
> postfix when the lock is being taken within the function.
> 
> Would this be a reasonable name update for these changes?

I think foo_unlocked() naming is usually used when there's also a
separate foo() that can be called if/when locks are already held (or
sometimes it's foo() and foo_locked() if the situation is the other way
around).  In this case we still only have one version of the function,
and it's only called from a single place in the code
(intel_uncore_forcewake_user_get) so I don't think the special naming is
necessary.  It might actually add confusion here since there's a
different lock (the general uncore lock) that is still held by the
caller.  It's just the mmio_debug-specific lock that's been moved into
the mmio-debug specific function here.

Matt

> 
> M
> 
> > {
> >-	lockdep_assert_held(&mmio_debug->lock);
> >+	spin_lock(&uncore->debug->lock);
> >
> > 	/* Save and disable mmio debugging for the user bypass */
> >-	if (!mmio_debug->suspend_count++) {
> >-		mmio_debug->saved_mmio_check = mmio_debug-
> >>unclaimed_mmio_check;
> >-		mmio_debug->unclaimed_mmio_check = 0;
> >+	if (!uncore->debug->suspend_count++) {
> >+		uncore->debug->saved_mmio_check = uncore->debug-
> >>unclaimed_mmio_check;
> >+		uncore->debug->unclaimed_mmio_check = 0;
> > 	}
> >+
> >+	spin_unlock(&uncore->debug->lock);
> > }
> >
> >-static void mmio_debug_resume(struct intel_uncore_mmio_debug
> >*mmio_debug)
> >+static bool check_for_unclaimed_mmio(struct intel_uncore *uncore);
> >+
> >+static void mmio_debug_resume(struct intel_uncore *uncore)
> > {
> >-	lockdep_assert_held(&mmio_debug->lock);
> >+	spin_lock(&uncore->debug->lock);
> >+
> >+	if (!--uncore->debug->suspend_count)
> >+		uncore->debug->unclaimed_mmio_check = uncore->debug-
> >>saved_mmio_check;
> >
> >-	if (!--mmio_debug->suspend_count)
> >-		mmio_debug->unclaimed_mmio_check = mmio_debug-
> >>saved_mmio_check;
> >+	if (check_for_unclaimed_mmio(uncore))
> >+		drm_info(&uncore->i915->drm,
> >+			 "Invalid mmio detected during user access\n");
> >+
> >+	spin_unlock(&uncore->debug->lock);
> > }
> >
> > static const char * const forcewake_domain_names[] = {
> >@@ -677,9 +687,7 @@ void intel_uncore_forcewake_user_get(struct
> >intel_uncore *uncore)
> > 	spin_lock_irq(&uncore->lock);
> > 	if (!uncore->user_forcewake_count++) {
> > 		intel_uncore_forcewake_get__locked(uncore,
> >FORCEWAKE_ALL);
> >-		spin_lock(&uncore->debug->lock);
> >-		mmio_debug_suspend(uncore->debug);
> >-		spin_unlock(&uncore->debug->lock);
> >+		mmio_debug_suspend(uncore);
> > 	}
> > 	spin_unlock_irq(&uncore->lock);
> > }
> >@@ -695,14 +703,7 @@ void intel_uncore_forcewake_user_put(struct
> >intel_uncore *uncore)
> > {
> > 	spin_lock_irq(&uncore->lock);
> > 	if (!--uncore->user_forcewake_count) {
> >-		spin_lock(&uncore->debug->lock);
> >-		mmio_debug_resume(uncore->debug);
> >-
> >-		if (check_for_unclaimed_mmio(uncore))
> >-			drm_info(&uncore->i915->drm,
> >-				 "Invalid mmio detected during user
> >access\n");
> >-		spin_unlock(&uncore->debug->lock);
> >-
> >+		mmio_debug_resume(uncore);
> > 		intel_uncore_forcewake_put__locked(uncore,
> >FORCEWAKE_ALL);
> > 	}
> > 	spin_unlock_irq(&uncore->lock);
> >--
> >2.37.2
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation