[Intel-gfx] [RFC] drm/i915: Add GuC timeout config options to Kconfig.debug

[Tvrtko Ursulin]

On 14/06/2017 00:19, Kelvin Gardiner wrote:
> It is sometimes useful for debug purposes to be able to set GuC timeout
> lengths.
> 
> This patch adds GuC load and request timeouts values to Kconfig.debug,
> which can then be optionally set as required for debug cases. A default
> value equal to the current hard-coded values are provided.
> 
> In the case when a Kconfig option has not been set, a default value is
> provide using a define.
> 
> Signed-off-by: Kelvin Gardiner <kelvin.gardiner at intel.com>
> ---
>   drivers/gpu/drm/i915/Kconfig.debug      | 40 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_drv.h         | 13 +++++++++++
>   drivers/gpu/drm/i915/intel_guc_loader.c |  3 ++-
>   drivers/gpu/drm/i915/intel_uc.c         |  5 ++++-
>   4 files changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index 78c5c04..6a0767d 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -112,3 +112,43 @@ config DRM_I915_DEBUG_VBLANK_EVADE
>   	  the vblank.
>   
>   	  If in doubt, say "N".
> +
> +config DRM_I915_OVERRIDE_TIMEOUTS
> +	bool "Enable timeout overrides"
> +        depends on DRM_I915
> +        default n
> +        help
> +          Enable this option to allow overriding of selected timeouts in the
> +	  driver i915.
> +
> +	  Timeouts should only be overridden for debug and not in normal use.
> +
> +	  If in doubt, say "N".

If you drop this config...

> +
> +config DRM_I915_TIMEOUT_GUC_LOAD
> +	int "Set the value of the GuC load timeout"
> +        depends on DRM_I915_OVERRIDE_TIMEOUTS

... and this depends line...

> +        default 100
> +        range 0 10000
> +        help
> +	  Set this option to select the value of the timeout in ms for how long
> +	  the GuC FW load should take.
> +
> +	  The valid range is 0 to 10000
> +
> +	  The default timeout will work in normal use. This option is provided
> +	  for debug.
> +
> +config DRM_I915_TIMEOUT_GUC_REQUEST
> +	int "Set the value of the GuC request timeout"
> +        depends on DRM_I915_OVERRIDE_TIMEOUTS

... and this one...

> +	default 10
> +	range 0 1000
> +	help
> +          Set this option to select the value of the timeout in ms for how long
> +	  a request to the GuC should take.
> +
> +	  The valid range is 0 to 1000
> +
> +	  The default timeout will work in normal use. This option is provided
> +	  for debug.
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 38ef734..efc56d2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1488,6 +1488,19 @@ struct drm_i915_error_state_buf {
>   #define I915_ENGINE_DEAD_TIMEOUT  (4 * HZ)  /* Seqno, head and subunits dead */
>   #define I915_SEQNO_DEAD_TIMEOUT   (12 * HZ) /* Seqno dead with active head */
>   
> +#ifndef CONFIG_DRM_I915_TIMEOUT_GUC_LOAD
> +#define DRM_I915_TIMEOUT_GUC_LOAD 100
> +#else
> +#define DRM_I915_TIMEOUT_GUC_LOAD CONFIG_DRM_I915_TIMEOUT_GUC_LOAD
> +#endif
> +
> +#ifndef CONFIG_DRM_I915_TIMEOUT_GUC_REQUEST
> +#define DRM_I915_TIMEOUT_GUC_REQUEST 10
> +#else
> +#define DRM_I915_TIMEOUT_GUC_REQUEST CONFIG_DRM_I915_TIMEOUT_GUC_REQUEST
> +#endif
> +
> +

... and this whole block ...

>   struct i915_gpu_error {
>   	/* For hangcheck timer */
>   #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 8b0ae7f..0d7abad 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -235,7 +235,8 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>   	 * (Higher levels of the driver will attempt to fall back to
>   	 * execlist mode if this happens.)
>   	 */
> -	ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
> +	ret = wait_for(guc_ucode_response(dev_priv, &status),
> +			DRM_I915_TIMEOUT_GUC_LOAD);

... and use CONFIG_DRM_I915_TIMEOUT_GUC_LOAD directly here ...

>   
>   	DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
>   			I915_READ(DMA_CTRL), status);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 27e072c..de34119 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -501,7 +501,10 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>   					   guc_send_reg(guc, 0),
>   					   INTEL_GUC_RECV_MASK,
>   					   INTEL_GUC_RECV_MASK,
> -					   10, 10, &status);
> +					   10,
> +					   DRM_I915_TIMEOUT_GUC_REQUEST,

... and the same here respectively, then the patch becomes somewhat shorter.

Question is whether that would be worth it, or having an explicit master 
toggle is more desirable?

Since both timeout options provide defaults, and they are under the 
debug submenu already, I thought we could get away with less. Especially 
the #ifdef block in i915_drv.h I think is desirable to avoid if we can.

Regards,

Tvrtko

> +					   &status);
> +
>   	if (status != INTEL_GUC_STATUS_SUCCESS) {
>   		/*
>   		 * Either the GuC explicitly returned an error (which
>