> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Monday, August 29, 2022 10:03 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Sripada, Radhakrishna
> <radhakrishna.sripada@xxxxxxxxx>; Roper, Matthew D
> <matthew.d.roper@xxxxxxxxx>
> Subject: [PATCH 1/8] 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.
>
LGTM,
Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@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)
> {
> - 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
