On ti, 2016-07-26 at 09:19 +0100, Chris Wilson wrote: > 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 <chris@xxxxxxxxxxxxxxxxxx> > > > Link: http://patchwork.freedesktop.org/patch/msgid/1469432687-22756-8-git-send-email-chris@xxxxxxxxxxxxxxxxxx > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 6 +- > > > drivers/gpu/drm/i915/i915_gpu_error.c | 255 +++++++++++++++++----------------- > > > 2 files changed, 134 insertions(+), 127 deletions(-) > > > > > > > > > > > > > @@ -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() > Fine. > (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. Ack, Joonas > -Chris > -- Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
