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

[Kelvin Gardiner]

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 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.

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
>>