| Message ID | 20220902233257.3088492-2-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i915: Add "standalone media" support for MTL | expand |
>-----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? 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
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 >
>-----Original Message----- >From: Roper, Matthew D <matthew.d.roper@intel.com> >Sent: Tuesday, September 6, 2022 1:09 PM >To: Ruhl, Michael J <michael.j.ruhl@intel.com> >Cc: intel-gfx@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} > >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. 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
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) { - 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);