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

[Ruhl, Michael J]

>-----Original Message-----
>From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of
>Matt Roper
>Sent: Friday, September 2, 2022 7:33 PM
>To: intel-gfx at lists.freedesktop.org
>Cc: dri-devel at lists.freedesktop.org; Sripada, Radhakrishna
><radhakrishna.sripada at 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 at intel.com>
>Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada at 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?

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