[Intel-gfx] [PATCH v5 1/5] drm/i915: Add engine reset count in get-reset-stats ioctl

[Chris Wilson]

Quoting Carlos Santa (2019-03-22 23:41:14)
> From: Michel Thierry <michel.thierry at intel.com>
> 
> Users/tests relying on the total reset count will start seeing a smaller
> number since most of the hangs can be handled by engine reset.
> Note that if reset engine x, context a running on engine y will be unaware
> and unaffected.
> 
> To start the discussion, include just a total engine reset count. If it
> is deemed useful, it can be extended to report each engine separately.
> 
> Our igt's gem_reset_stats test will need changes to ignore the pad field,
> since it can now return reset_engine_count.
> 
> v2: s/engine_reset/reset_engine/, use union in uapi to not break compatibility.
> v3: Keep rejecting attempts to use pad as input (Antonio)
> v4: Rebased.
> v5: Rebased.
>     Get rid of the union to store pad/engine count (Chris)
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> Cc: Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> Signed-off-by: Carlos Santa <carlos.santa at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++++++--
>  include/uapi/drm/i915_drm.h             |  4 ++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 21208a865380..9625b5f7faf7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -1350,6 +1350,8 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
>         struct drm_i915_private *dev_priv = to_i915(dev);
>         struct drm_i915_reset_stats *args = data;
>         struct i915_gem_context *ctx;
> +       struct intel_engine_cs *engine;
> +       enum intel_engine_id id;
>         int ret;
>  
>         if (args->flags || args->pad)
> @@ -1368,10 +1370,16 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
>          * we should wrap the hangstats with a seqlock.
>          */
>  
> -       if (capable(CAP_SYS_ADMIN))
> +       if (capable(CAP_SYS_ADMIN)) {
>                 args->reset_count = i915_reset_count(&dev_priv->gpu_error);
> -       else
> +               for_each_engine(engine, dev_priv, id)
> +                       args->reset_engine_count +=
> +                               i915_reset_engine_count(&dev_priv->gpu_error,
> +                                                       engine);

Do we really care about device-vs-engine here? Amalgamating all engine
resets into one variable is barely any more information than
amalgamating them with the device count.

> +       } else {
>                 args->reset_count = 0;
> +               args->reset_engine_count = 0;
> +       }
>  
>         args->batch_active = atomic_read(&ctx->guilty_count);
>         args->batch_pending = atomic_read(&ctx->active_count);
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index aa2d4c73a97d..5e7bc6412880 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1459,6 +1459,9 @@ struct drm_i915_reset_stats {
>         /* All resets since boot/module reload, for all contexts */
>         __u32 reset_count;
>  
> +       /* Engine resets since boot/module reload, for all contexts */
> +       __u32 reset_engine_count;

You cannot insert elements into the middle of an uABI struct. You can
only extend. To avoid problems with sw layout, always introduce a new
struct.

> +
>         /* Number of batches lost when active in GPU, for this context */
>         __u32 batch_active;
>  
> @@ -1466,6 +1469,7 @@ struct drm_i915_reset_stats {
>         __u32 batch_pending;
>  
>         __u32 pad;
> +
>  };
>  
>  struct drm_i915_gem_userptr {
> -- 
> 2.17.1
>