On Tue, Sep 06, 2022 at 06:39:14AM -0700, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> >Matt Roper
> >Sent: Friday, September 2, 2022 7:33 PM
> >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Sripada, Radhakrishna
> ><radhakrishna.sripada@xxxxxxxxx>
> >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@xxxxxxxxx>
> >Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> >---
> > 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
