Re: [PATCH 07/55] drm/i915: Avoid using intel_engine_cs *ring for GPU error capture

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ma, 2016-07-25 at 18:31 +0100, Chris Wilson wrote:

> Inside the error capture itself, we refer to not only the hardware
> engine, its ringbuffer but also the capture state. Finding clear names
> for each whilst avoiding mixing ring/intel_engine_cs is tricky. As a
> compromise we keep using ering for the error capture.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Link: http://patchwork.freedesktop.org/patch/msgid/1469432687-22756-8-git-send-email-chris@chris-wilson.co.uk
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |   6 +-
>  drivers/gpu/drm/i915/i915_gpu_error.c | 255 +++++++++++++++++-----------------
>  2 files changed, 134 insertions(+), 127 deletions(-)
> 

<SNIP>

> @@ -240,69 +240,71 @@ static const char *hangcheck_action_to_str(enum intel_ring_hangcheck_action a)
>  }
>  
>  static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
> -				  struct drm_device *dev,
> -				  struct drm_i915_error_state *error,
> -				  int ring_idx)
> +				    struct drm_device *dev,
> +				    struct drm_i915_error_state *error,
> +				    int engine_idx)
>  {
> -	struct drm_i915_error_ring *ring = &error->ring[ring_idx];
> +	struct drm_i915_error_engine *ering = &error->engine[engine_idx];
>  

I'd be inclined keeping the struct and variable names close, so rather
eengine. Even though the error state is a mashup. We fill the ring
state to the engine error state. Function could be
i915_engine_error_ring_state() or so, to "reduce" confusion?

> @@ -414,7 +416,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>  	if (IS_GEN7(dev))
>  		err_printf(m, "ERR_INT: 0x%08x\n", error->err_int);
>  
> -	for (i = 0; i < ARRAY_SIZE(error->ring); i++)
> +	for (i = 0; i < ARRAY_SIZE(error->engine); i++)
>  		i915_ring_error_state(m, dev, error, i);
>  

This captures the engine related ring state, I think it's even worth a
comment when there is engine vs. error disparity.

And how about the messages? Should we update them more agressively
where necessary.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx