Re: [PATCH 5/6] drm/i915: move load failure injection to selftests

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/12/2018 14:33, Jani Nikula wrote:
> Seems like selftests is a better home for everything related to load
> failure injection, including the module parameter.

Hm not sure I would immediately want to couple the two, however..

> The failure injection code gets moved from under DRM_I915_DEBUG to
> DRM_I915_SELFTEST config option. This should be of no consequence, as
> the former selects the latter.

... I also don't see why debug would auto-select self-tests.

So don't know. Personally I'd have selftests separate from debug, and 
load failure injection separate from selftests. But I don't feel that 
strongly about it. At least I agree that losing failure injection from 
the error state is not an issue. (Since all of them are strictly during 
driver load phase.)

Regards,

Tvrtko

> 
> With the parameter no longer part of i915_params, its value will not be
> recorded in error capture or debugfs param dump. Note that the value
> would have been zeroed anyway if a selftest had been hit, so there
> should not be meaningful information loss here, especially with all the
> logging around failure injection.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c                | 25 -------------------------
>   drivers/gpu/drm/i915/i915_drv.h                | 15 ---------------
>   drivers/gpu/drm/i915/i915_params.c             |  5 -----
>   drivers/gpu/drm/i915/i915_params.h             |  1 -
>   drivers/gpu/drm/i915/i915_selftest.h           | 10 ++++++++++
>   drivers/gpu/drm/i915/selftests/i915_selftest.c | 26 ++++++++++++++++++++++++++
>   6 files changed, 36 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index caa055ac9472..559a1b11f3e4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -57,31 +57,6 @@
>   
>   static struct drm_driver driver;
>   
> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
> -static unsigned int i915_load_fail_count;
> -
> -bool __i915_inject_load_failure(const char *func, int line)
> -{
> -	if (i915_load_fail_count >= i915_modparams.inject_load_failure)
> -		return false;
> -
> -	if (++i915_load_fail_count == i915_modparams.inject_load_failure) {
> -		DRM_INFO("Injecting failure at checkpoint %u [%s:%d]\n",
> -			 i915_modparams.inject_load_failure, func, line);
> -		i915_modparams.inject_load_failure = 0;
> -		return true;
> -	}
> -
> -	return false;
> -}
> -
> -bool i915_error_injected(void)
> -{
> -	return i915_load_fail_count && !i915_modparams.inject_load_failure;
> -}
> -
> -#endif
> -
>   #define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI"
>   #define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \
>   		    "providing the dmesg log by booting with drm.debug=0xf"
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c60cf17e50a0..1f29f3933625 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -111,21 +111,6 @@
>   #define I915_STATE_WARN_ON(x)						\
>   	I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")")
>   
> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
> -
> -bool __i915_inject_load_failure(const char *func, int line);
> -#define i915_inject_load_failure() \
> -	__i915_inject_load_failure(__func__, __LINE__)
> -
> -bool i915_error_injected(void);
> -
> -#else
> -
> -#define i915_inject_load_failure() false
> -#define i915_error_injected() false
> -
> -#endif
> -
>   #define i915_load_error(i915, fmt, ...)					 \
>   	__i915_printk(i915, i915_error_injected() ? KERN_DEBUG : KERN_ERR, \
>   		      fmt, ##__VA_ARGS__)
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 9f0539bdaa39..2853b54570eb 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -159,11 +159,6 @@ i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
>   i915_param_named_unsafe(enable_dp_mst, bool, 0600,
>   	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
>   
> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
> -i915_param_named_unsafe(inject_load_failure, uint, 0400,
> -	"Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
> -#endif
> -
>   i915_param_named(enable_dpcd_backlight, bool, 0600,
>   	"Enable support for DPCD backlight control (default:false)");
>   
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 93f665eced16..85a9007c0ed6 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -53,7 +53,6 @@ struct drm_printer;
>   	param(int, mmio_debug, 0) \
>   	param(int, edp_vswing, 0) \
>   	param(int, reset, 2) \
> -	param(unsigned int, inject_load_failure, 0) \
>   	/* leave bools at the end to not create holes */ \
>   	param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \
>   	param(bool, enable_hangcheck, true) \
> diff --git a/drivers/gpu/drm/i915/i915_selftest.h b/drivers/gpu/drm/i915/i915_selftest.h
> index a73472dd12fd..1266cef8bf17 100644
> --- a/drivers/gpu/drm/i915/i915_selftest.h
> +++ b/drivers/gpu/drm/i915/i915_selftest.h
> @@ -33,6 +33,7 @@ struct i915_selftest {
>   	unsigned int random_seed;
>   	int mock;
>   	int live;
> +	unsigned int inject_load_failure;
>   };
>   
>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> @@ -77,6 +78,12 @@ int __i915_subtests(const char *caller,
>   #define I915_SELFTEST_DECLARE(x) x
>   #define I915_SELFTEST_ONLY(x) unlikely(x)
>   
> +bool __i915_inject_load_failure(const char *func, int line);
> +#define i915_inject_load_failure() \
> +	__i915_inject_load_failure(__func__, __LINE__)
> +
> +bool i915_error_injected(void);
> +
>   #else /* !IS_ENABLED(CONFIG_DRM_I915_SELFTEST) */
>   
>   static inline int i915_mock_selftests(void) { return 0; }
> @@ -85,6 +92,9 @@ static inline int i915_live_selftests(struct pci_dev *pdev) { return 0; }
>   #define I915_SELFTEST_DECLARE(x)
>   #define I915_SELFTEST_ONLY(x) 0
>   
> +static inline int i915_inject_load_failure(void) { return false; }
> +static inline int i915_error_injected(void) { return false; }
> +
>   #endif
>   
>   /* Using the i915_selftest_ prefix becomes a little unwieldy with the helpers.
> diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c b/drivers/gpu/drm/i915/selftests/i915_selftest.c
> index 86c54ea37f48..9e556fc4707d 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_selftest.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c
> @@ -250,3 +250,29 @@ MODULE_PARM_DESC(mock_selftests, "Run selftests before loading, using mock hardw
>   
>   module_param_named_unsafe(live_selftests, i915_selftest.live, int, 0400);
>   MODULE_PARM_DESC(live_selftests, "Run selftests after driver initialisation on the live system (0:disabled [default], 1:run tests then continue, -1:run tests then exit module)");
> +
> +module_param_named_unsafe(inject_load_failure, i915_selftest.inject_load_failure, uint, 0400);
> +MODULE_PARM_DESC(inject_load_failure,
> +		 "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
> +
> +static unsigned int i915_load_fail_count;
> +
> +bool __i915_inject_load_failure(const char *func, int line)
> +{
> +	if (i915_load_fail_count >= i915_selftest.inject_load_failure)
> +		return false;
> +
> +	if (++i915_load_fail_count == i915_selftest.inject_load_failure) {
> +		DRM_INFO("Injecting failure at checkpoint %u [%s:%d]\n",
> +			 i915_selftest.inject_load_failure, func, line);
> +		i915_selftest.inject_load_failure = 0;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +bool i915_error_injected(void)
> +{
> +	return i915_load_fail_count && !i915_selftest.inject_load_failure;
> +}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx