On Tue, Jul 26, 2016 at 07:59:29AM +0300, Joonas Lahtinen wrote:
> 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 <[email protected]>
> > Link:
> > http://patchwork.freedesktop.org/patch/msgid/[email protected]
> > ---
> > 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?
i915_error_print_engine()
(We may eventually get to the point where the capture/error functions
are clearly and distinctly labelled!)
>
> > @@ -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.
This becomes
for (i = 0; i < ARRAY_SIZE(error->engine); i++)
i915_error_print_engine(m, &error->engine[i]);
> And how about the messages? Should we update them more agressively
> where necessary.
Ignoring the tautology in engine->name, in the actual error print the
only times we explicitly refer to ring we do mean the ring.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
