Re: [PATCH] drm/i915: Correct CSB probing for engine state dumper

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx