| Message ID | 20180817085039.884-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Correct CSB probing for engine state dumper | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Since we no longer maintain our read position in the CSB pointers > register, it always returns 0 and not where we last read up to. As a > result the CSB probing in the state dumper starts from 0, either missing > entries or showing stale one. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 8628567d8f6e..49b580c188eb 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -1345,20 +1345,20 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine, > > if (HAS_EXECLISTS(dev_priv)) { > const u32 *hws = &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX]; > - u32 ptr, read, write; > unsigned int idx; > + u8 read, write; > > drm_printf(m, "\tExeclist status: 0x%08x %08x\n", > I915_READ(RING_EXECLIST_STATUS_LO(engine)), > I915_READ(RING_EXECLIST_STATUS_HI(engine))); > > - ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine)); > - read = GEN8_CSB_READ_PTR(ptr); > - write = GEN8_CSB_WRITE_PTR(ptr); > - drm_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws], tasklet queued? %s (%s)\n", > - read, execlists->csb_head, > - write, > - intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)), > + read = execlists->csb_head; > + write = intel_read_status_page(engine, > + intel_hws_csb_write_index(dev_priv)); > + > + drm_printf(m, "\tExeclist CSB read %d, write %d [mmio:%d], tasklet queued? %s (%s)\n", > + read, write, In here I was thinking that we want to keep the write as 32bit so it would show weirdness in here with %d. But the odds of that to happen with only in this status page entry... Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > + GEN8_CSB_WRITE_PTR(I915_READ(RING_CONTEXT_STATUS_PTR(engine))), > yesno(test_bit(TASKLET_STATE_SCHED, > &engine->execlists.tasklet.state)), > enableddisabled(!atomic_read(&engine->execlists.tasklet.count))); > @@ -1370,12 +1370,12 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine, > write += GEN8_CSB_ENTRIES; > while (read < write) { > idx = ++read % GEN8_CSB_ENTRIES; > - drm_printf(m, "\tExeclist CSB[%d]: 0x%08x [0x%08x in hwsp], context: %d [%d in hwsp]\n", > + drm_printf(m, "\tExeclist CSB[%d]: 0x%08x [mmio:0x%08x], context: %d [mmio:%d]\n", > idx, > - I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, idx)), > hws[idx * 2], > - I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, idx)), > - hws[idx * 2 + 1]); > + I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, idx)), > + hws[idx * 2 + 1], > + I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, idx))); > } > > rcu_read_lock(); > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Mika Kuoppala (2018-08-21 08:58:26) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Since we no longer maintain our read position in the CSB pointers > > register, it always returns 0 and not where we last read up to. As a > > result the CSB probing in the state dumper starts from 0, either missing > > entries or showing stale one. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > > --- > > drivers/gpu/drm/i915/intel_engine_cs.c | 24 ++++++++++++------------ > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index 8628567d8f6e..49b580c188eb 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -1345,20 +1345,20 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine, > > > > if (HAS_EXECLISTS(dev_priv)) { > > const u32 *hws = &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX]; > > - u32 ptr, read, write; > > unsigned int idx; > > + u8 read, write; > > > > drm_printf(m, "\tExeclist status: 0x%08x %08x\n", > > I915_READ(RING_EXECLIST_STATUS_LO(engine)), > > I915_READ(RING_EXECLIST_STATUS_HI(engine))); > > > > - ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine)); > > - read = GEN8_CSB_READ_PTR(ptr); > > - write = GEN8_CSB_WRITE_PTR(ptr); > > - drm_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws], tasklet queued? %s (%s)\n", > > - read, execlists->csb_head, > > - write, > > - intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)), > > + read = execlists->csb_head; > > + write = intel_read_status_page(engine, > > + intel_hws_csb_write_index(dev_priv)); > > + > > + drm_printf(m, "\tExeclist CSB read %d, write %d [mmio:%d], tasklet queued? %s (%s)\n", > > + read, write, > > In here I was thinking that we want to keep the write as 32bit so it > would show weirdness in here with %d. But the odds of that to happen > with only in this status page entry... Hmm, it was u8 for automatically truncating execlists->csb_write... Oh, this should have been write = READ_ONCE(*execlists->csb_write); Mind if I make that change? -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2018-08-21 08:58:26) >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >> > Since we no longer maintain our read position in the CSB pointers >> > register, it always returns 0 and not where we last read up to. As a >> > result the CSB probing in the state dumper starts from 0, either missing >> > entries or showing stale one. >> > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_engine_cs.c | 24 ++++++++++++------------ >> > 1 file changed, 12 insertions(+), 12 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >> > index 8628567d8f6e..49b580c188eb 100644 >> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> > @@ -1345,20 +1345,20 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine, >> > >> > if (HAS_EXECLISTS(dev_priv)) { >> > const u32 *hws = &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX]; >> > - u32 ptr, read, write; >> > unsigned int idx; >> > + u8 read, write; >> > >> > drm_printf(m, "\tExeclist status: 0x%08x %08x\n", >> > I915_READ(RING_EXECLIST_STATUS_LO(engine)), >> > I915_READ(RING_EXECLIST_STATUS_HI(engine))); >> > >> > - ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine)); >> > - read = GEN8_CSB_READ_PTR(ptr); >> > - write = GEN8_CSB_WRITE_PTR(ptr); >> > - drm_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws], tasklet queued? %s (%s)\n", >> > - read, execlists->csb_head, >> > - write, >> > - intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)), >> > + read = execlists->csb_head; >> > + write = intel_read_status_page(engine, >> > + intel_hws_csb_write_index(dev_priv)); >> > + >> > + drm_printf(m, "\tExeclist CSB read %d, write %d [mmio:%d], tasklet queued? %s (%s)\n", >> > + read, write, >> >> In here I was thinking that we want to keep the write as 32bit so it >> would show weirdness in here with %d. But the odds of that to happen >> with only in this status page entry... > > Hmm, it was u8 for automatically truncating execlists->csb_write... > Oh, this should have been write = READ_ONCE(*execlists->csb_write); > > Mind if I make that change? Please do. -Mika
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 8628567d8f6e..49b580c188eb 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -1345,20 +1345,20 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine, if (HAS_EXECLISTS(dev_priv)) { const u32 *hws = &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX]; - u32 ptr, read, write; unsigned int idx; + u8 read, write; drm_printf(m, "\tExeclist status: 0x%08x %08x\n", I915_READ(RING_EXECLIST_STATUS_LO(engine)), I915_READ(RING_EXECLIST_STATUS_HI(engine))); - ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine)); - read = GEN8_CSB_READ_PTR(ptr); - write = GEN8_CSB_WRITE_PTR(ptr); - drm_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws], tasklet queued? %s (%s)\n", - read, execlists->csb_head, - write, - intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)), + read = execlists->csb_head; + write = intel_read_status_page(engine, + intel_hws_csb_write_index(dev_priv)); + + drm_printf(m, "\tExeclist CSB read %d, write %d [mmio:%d], tasklet queued? %s (%s)\n", + read, write, + GEN8_CSB_WRITE_PTR(I915_READ(RING_CONTEXT_STATUS_PTR(engine))), yesno(test_bit(TASKLET_STATE_SCHED, &engine->execlists.tasklet.state)), enableddisabled(!atomic_read(&engine->execlists.tasklet.count))); @@ -1370,12 +1370,12 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine, write += GEN8_CSB_ENTRIES; while (read < write) { idx = ++read % GEN8_CSB_ENTRIES; - drm_printf(m, "\tExeclist CSB[%d]: 0x%08x [0x%08x in hwsp], context: %d [%d in hwsp]\n", + drm_printf(m, "\tExeclist CSB[%d]: 0x%08x [mmio:0x%08x], context: %d [mmio:%d]\n", idx, - I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, idx)), hws[idx * 2], - I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, idx)), - hws[idx * 2 + 1]); + I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, idx)), + hws[idx * 2 + 1], + I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, idx))); } rcu_read_lock();
Since we no longer maintain our read position in the CSB pointers register, it always returns 0 and not where we last read up to. As a result the CSB probing in the state dumper starts from 0, either missing entries or showing stale one. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)