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

[Ruhl, Michael J]

>-----Original Message-----
>From: Roper, Matthew D <matthew.d.roper at intel.com>
>Sent: Tuesday, September 6, 2022 1:09 PM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Sripada,
>Radhakrishna <radhakrishna.sripada at intel.com>
>Subject: Re: [PATCH v2 01/12] drm/i915: Move locking and unclaimed check
>into mmio_debug_{suspend, resume}
>
>On Tue, Sep 06, 2022 at 06:39:14AM -0700, Ruhl, Michael J wrote:
>> >-----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?
>
>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.

Got it.  That makes sense.

Thanks,

Mike

>
>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