>-----Original Message-----
>From: Roper, Matthew D <matthew.d.ro...@intel.com>
>Sent: Tuesday, September 6, 2022 1:09 PM
>To: Ruhl, Michael J <michael.j.r...@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; Sripada,
>Radhakrishna <radhakrishna.srip...@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-boun...@lists.freedesktop.org> On Behalf Of
>> >Matt Roper
>> >Sent: Friday, September 2, 2022 7:33 PM
>> >To: intel-gfx@lists.freedesktop.org
>> >Cc: dri-de...@lists.freedesktop.org; Sripada, Radhakrishna
>> ><radhakrishna.srip...@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.ro...@intel.com>
>> >Reviewed-by: Radhakrishna Sripada <radhakrishna.srip...@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
