Re: [PATCH v3] drm/i915: Disable the mmio.debug WARN after it fires

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 18 Dec 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If we have a single unclaimed register, we will have lots. A WARN for
> each one makes the machine unusable and does not aid debugging. Convert
> the i915.mmio_debug option to a counter for how many WARNs to fire
> before shutting up. Even when i915.mmio_debug was disabled it would
> continue to shout an *ERROR* for every interrupt, without any
> information at all for debugging.
>
> The massive verbiage was added in
> commit 5978118c39c2f72fd8b39ef9c086723542384809
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Wed Jul 16 17:49:29 2014 -0300
>
>     drm/i915: reorganize the unclaimed register detection code
>
> v2: Automatically enable invalid mmio reporting for the *next* invalid
> access if mmio_debug is disabled by default. This should give us clearer
> debug information without polluting the logs too much.
> v3: Compile fixes, rebase.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
>  drivers/gpu/drm/i915/i915_params.c  |  6 +++---
>  drivers/gpu/drm/i915/intel_uncore.c | 10 ++++++++--
>  3 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3047291ff2b9..ca9e21545063 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2430,7 +2430,7 @@ struct i915_params {
>  	bool disable_display;
>  	bool disable_vtd_wa;
>  	int use_mmio_flip;
> -	bool mmio_debug;
> +	int mmio_debug;
>  	bool verbose_state_checks;
>  };
>  extern struct i915_params i915 __read_mostly;
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 07252d8dc726..43c1df830531 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -170,10 +170,10 @@ module_param_named(use_mmio_flip, i915.use_mmio_flip, int, 0600);
>  MODULE_PARM_DESC(use_mmio_flip,
>  		 "use MMIO flips (-1=never, 0=driver discretion [default], 1=always)");
>  
> -module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
> +module_param_named(mmio_debug, i915.mmio_debug, int, 0600);
>  MODULE_PARM_DESC(mmio_debug,
> -	"Enable the MMIO debug code (default: false). This may negatively "
> -	"affect performance.");
> +	"Enable the MMIO debug code (default: off). "
> +	"This may negatively affect performance.");

Why not describe the new behaviour here instead of a comment in
intel_uncore.c?

>  
>  module_param_named(verbose_state_checks, i915.verbose_state_checks, bool, 0600);
>  MODULE_PARM_DESC(verbose_state_checks,
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index e9561de382aa..a3b662de1bdb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -722,18 +722,24 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read,
>  		WARN(1, "Unclaimed register detected %s %s register 0x%x\n",
>  		     when, op, reg);
>  		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> +		i915.mmio_debug--; /* Only report the first N failures */
>  	}
>  }
>  
>  static void
>  hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>  {
> -	if (i915.mmio_debug)
> +	static bool mmio_debug_once = true;
> +
> +	if (i915.mmio_debug || !mmio_debug_once)
>  		return;
>  
>  	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> -		DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
> +		DRM_DEBUG("Unclaimed register detected, "
> +			  "enabling oneshot unclaimed register reporting. "
> +			  "Please use i915.mmio_debug=N for more information.\n");
>  		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> +		i915.mmio_debug = mmio_debug_once--;

/me frowns upon the bool assignment to int and bool post-decrement. It's
not quite IOCCC but a demonstration of things that suck about C.

Is it on purpose that, if you've set i915.mmio_debug=N, you first
decrement it to zero, then enter here and set i915.mmio_debug=1 to do
hsw_unclaimed_reg_debug once more?


BR,
Jani.

>  	}
>  }
>  
> -- 
> 2.1.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx