[Intel-gfx] [PATCH] drm/i915/uncore: Warn on previous unclaimed accesses

[Matt Roper]

On Mon, Apr 04, 2022 at 05:11:49PM -0700, Lucas De Marchi wrote:
> Since gen6 we use FPGA_DBG register to detect unclaimed MMIO registers.
> This register is in the display engine IP and can only ever detect
> unclaimed accesses to registers in this area. However sometimes there
> are reports of this triggering for registers in other areas, which
> should not be possible.
> 
> Right now we always warn after the read/write of registers going through
> unclaimed_reg_debug(). However places using __raw_uncore_* may be
> triggering the unclaimed access and those being later accounted to a
> different register. Let's warn both before and after the read/write
> with a slightly different message, so it's clear if the register
> reported in the warning is actually the culprit.

You should probably probably give an explicit mention of commit
dda960335e020 ("drm/i915: Just clear the mmiodebug before a register
access") in the commit message since we're reversing direction here.

> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 8b9caaaacc21..df59ec88459e 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1502,11 +1502,10 @@ ilk_dummy_write(struct intel_uncore *uncore)
>  static void
>  __unclaimed_reg_debug(struct intel_uncore *uncore,
>  		      const i915_reg_t reg,
> -		      const bool read,
> -		      const bool before)
> +		      const bool read)
>  {
>  	if (drm_WARN(&uncore->i915->drm,
> -		     check_for_unclaimed_mmio(uncore) && !before,
> +		     check_for_unclaimed_mmio(uncore),
>  		     "Unclaimed %s register 0x%x\n",

Might be simpler to just keep it all in one function and do something
like a

        before ? "MMIO operation *before* a " : ""

in the message?  Up to you.  Either way,

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

>  		     read ? "read from" : "write to",
>  		     i915_mmio_reg_offset(reg)))
> @@ -1514,6 +1513,20 @@ __unclaimed_reg_debug(struct intel_uncore *uncore,
>  		uncore->i915->params.mmio_debug--;
>  }
>  
> +static void
> +__unclaimed_previous_reg_debug(struct intel_uncore *uncore,
> +			       const i915_reg_t reg,
> +			       const bool read)
> +{
> +	if (drm_WARN(&uncore->i915->drm,
> +		     check_for_unclaimed_mmio(uncore),
> +		     "Unclaimed access detected before %s register 0x%x\n",
> +		     read ? "read from" : "write to",
> +		     i915_mmio_reg_offset(reg)))
> +		/* Only report the first N failures */
> +		uncore->i915->params.mmio_debug--;
> +}
> +
>  static inline void
>  unclaimed_reg_debug(struct intel_uncore *uncore,
>  		    const i915_reg_t reg,
> @@ -1526,13 +1539,13 @@ unclaimed_reg_debug(struct intel_uncore *uncore,
>  	/* interrupts are disabled and re-enabled around uncore->lock usage */
>  	lockdep_assert_held(&uncore->lock);
>  
> -	if (before)
> +	if (before) {
>  		spin_lock(&uncore->debug->lock);
> -
> -	__unclaimed_reg_debug(uncore, reg, read, before);
> -
> -	if (!before)
> +		__unclaimed_previous_reg_debug(uncore, reg, read);
> +	} else {
> +		__unclaimed_reg_debug(uncore, reg, read);
>  		spin_unlock(&uncore->debug->lock);
> +	}
>  }
>  
>  #define __vgpu_read(x) \
> -- 
> 2.35.1
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795