All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] drm/i915: Add GuC timeout config options to Kconfig.debug
@ 2017-06-13 23:19 Kelvin Gardiner
  2017-06-13 23:41 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Kelvin Gardiner @ 2017-06-13 23:19 UTC (permalink / raw)
  To: intel-gfx

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@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".
+
+config DRM_I915_TIMEOUT_GUC_LOAD
+	int "Set the value of the GuC load timeout"
+        depends on DRM_I915_OVERRIDE_TIMEOUTS
+        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
+	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
+
+
 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);
 
 	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,
+					   &status);
+
 	if (status != INTEL_GUC_STATUS_SUCCESS) {
 		/*
 		 * Either the GuC explicitly returned an error (which
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Add GuC timeout config options to Kconfig.debug
  2017-06-13 23:19 [RFC] drm/i915: Add GuC timeout config options to Kconfig.debug Kelvin Gardiner
@ 2017-06-13 23:41 ` Patchwork
  2017-06-27 21:22 ` [RFC] " Kelvin Gardiner
  2017-06-28  8:54 ` Tvrtko Ursulin
  2 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2017-06-13 23:41 UTC (permalink / raw)
  To: Kelvin Gardiner; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Add GuC timeout config options to Kconfig.debug
URL   : https://patchwork.freedesktop.org/series/25741/
State : success

== Summary ==

Series 25741v1 drm/i915: Add GuC timeout config options to Kconfig.debug
https://patchwork.freedesktop.org/api/1.0/series/25741/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-r) fdo#100125
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-bsw-n3050) fdo#100651

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#100651 https://bugs.freedesktop.org/show_bug.cgi?id=100651

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:443s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:430s
fi-bsw-n3050     total:237  pass:204  dwarn:0   dfail:0   fail:0   skip:32 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:509s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:491s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:485s
fi-glk-2a        total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:592s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:427s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:412s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:414s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:489s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:470s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:569s
fi-kbl-r         total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time:575s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:468s
fi-skl-6700hq    total:278  pass:228  dwarn:1   dfail:0   fail:27  skip:22  time:404s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:468s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:475s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:448s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:544s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:411s

d4bedb8b0f9ba91df2e8cb136a489145a83e96a7 drm-tip: 2017y-06m-13d-14h-22m-46s UTC integration manifest
b37f219 drm/i915: Add GuC timeout config options to Kconfig.debug

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4946/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC] drm/i915: Add GuC timeout config options to Kconfig.debug
  2017-06-13 23:19 [RFC] drm/i915: Add GuC timeout config options to Kconfig.debug Kelvin Gardiner
  2017-06-13 23:41 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-06-27 21:22 ` Kelvin Gardiner
  2017-06-28  8:54 ` Tvrtko Ursulin
  2 siblings, 0 replies; 5+ messages in thread
From: Kelvin Gardiner @ 2017-06-27 21:22 UTC (permalink / raw)
  To: intel-gfx

Hi,

Any feedback on this?

Thanks,
Kelvin

On 13/06/17 16: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@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".
> +
> +config DRM_I915_TIMEOUT_GUC_LOAD
> +	int "Set the value of the GuC load timeout"
> +        depends on DRM_I915_OVERRIDE_TIMEOUTS
> +        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
> +	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
> +
> +
>  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);
>
>  	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,
> +					   &status);
> +
>  	if (status != INTEL_GUC_STATUS_SUCCESS) {
>  		/*
>  		 * Either the GuC explicitly returned an error (which
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC] drm/i915: Add GuC timeout config options to Kconfig.debug
  2017-06-13 23:19 [RFC] drm/i915: Add GuC timeout config options to Kconfig.debug Kelvin Gardiner
  2017-06-13 23:41 ` ✓ Fi.CI.BAT: success for " Patchwork
  2017-06-27 21:22 ` [RFC] " Kelvin Gardiner
@ 2017-06-28  8:54 ` Tvrtko Ursulin
  2017-07-14 20:57   ` Kelvin Gardiner
  2 siblings, 1 reply; 5+ messages in thread
From: Tvrtko Ursulin @ 2017-06-28  8:54 UTC (permalink / raw)
  To: Kelvin Gardiner, intel-gfx


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@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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC] drm/i915: Add GuC timeout config options to Kconfig.debug
  2017-06-28  8:54 ` Tvrtko Ursulin
@ 2017-07-14 20:57   ` Kelvin Gardiner
  0 siblings, 0 replies; 5+ messages in thread
From: Kelvin Gardiner @ 2017-07-14 20:57 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx



On 28/06/17 01:54, Tvrtko Ursulin wrote:
>
> 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@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.

Agreed. I went with this implementation after some discussion with others.
I'll send a version with the changes you suggest, and we can see which 
seems cleaner.

> 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
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-07-14 20:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-13 23:19 [RFC] drm/i915: Add GuC timeout config options to Kconfig.debug Kelvin Gardiner
2017-06-13 23:41 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-06-27 21:22 ` [RFC] " Kelvin Gardiner
2017-06-28  8:54 ` Tvrtko Ursulin
2017-07-14 20:57   ` Kelvin Gardiner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.