Re: [Intel-gfx] [PATCH] drm/i915: Remove the modparam verbose_state_checks

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

On Thu, 12 Oct 2023, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> By default warn_on are enabled, hence removing this module parameter.

I'm uneasy about this change, though.

Do we really want to force people to see WARNs (and oops if
panic_on_warn=1) on state checker? On the other hand, that's the way to
get bug reports and fix the stuff...

I wonder if distros actually use the module parameter.

An alternative might be to add a debug config option.

> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.h | 2 +-
>  drivers/gpu/drm/i915/i915_params.c           | 4 ----
>  drivers/gpu/drm/i915/i915_params.h           | 1 -
>  3 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 0e5dffe8f018..8e2453e010a7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -552,7 +552,7 @@ bool assert_port_valid(struct drm_i915_private *i915, enum port port);
>  	struct drm_device *drm = &(__i915)->drm;			\
>  	int __ret_warn_on = !!(condition);				\
>  	if (unlikely(__ret_warn_on))					\
> -		if (!drm_WARN(drm, i915_modparams.verbose_state_checks, format)) \
> +		if (!drm_WARN(drm, true, format))			\
>  			drm_err(drm, format);				\

Everything in the whole macro is about that one condition on the verbose
state checks, and with that taken away, it all simplifies to something
along the lines of:

#define I915_STATE_WARN(__i915, condition, ...) \
	drm_WARN(&(__i915)->drm, condition, ## __VA_ARGS__)

I didn't build check that I got the va arg stuff right, but it's
something like that *if* we want to remove the condition altogether.

The comment also becomes stale and needs to be fixed.

BR,
Jani.


>  	unlikely(__ret_warn_on);					\
>  })
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 036c4c3ed6ed..23453d9be175 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -162,10 +162,6 @@ i915_param_named(mmio_debug, int, 0400,
>  	"Enable the MMIO debug code for the first N failures (default: off). "
>  	"This may negatively affect performance.");
>  
> -/* Special case writable file */
> -i915_param_named(verbose_state_checks, bool, 0600,
> -	"Enable verbose logs (ie. WARN_ON()) in case of unexpected hw state conditions.");
> -
>  i915_param_named_unsafe(nuclear_pageflip, bool, 0400,
>  	"Force enable atomic functionality on platforms that don't have full support yet.");
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index d5194b039aab..af675618ab07 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -83,7 +83,6 @@ struct drm_printer;
>  	param(bool, force_reset_modeset_test, false, 0600) \
>  	param(bool, error_capture, true, IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR) ? 0600 : 0) \
>  	param(bool, disable_display, false, 0400) \
> -	param(bool, verbose_state_checks, true, 0) \
>  	param(bool, nuclear_pageflip, false, 0400) \
>  	param(bool, enable_dp_mst, true, 0600) \
>  	param(bool, enable_gvt, false, IS_ENABLED(CONFIG_DRM_I915_GVT) ? 0400 : 0)

-- 
Jani Nikula, Intel