[Intel-gfx] [PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[Stuart Summers]

On Thu, 2019-08-08 at 00:12 +0100, Chris Wilson wrote:
> Quoting Stuart Summers (2019-08-08 00:00:17)
> > On Wed, 2019-08-07 at 23:01 +0100, Chris Wilson wrote:
> > > Quoting Stuart Summers (2019-08-07 22:48:55)
> > > > On Wed, 2019-08-07 at 22:29 +0100, Chris Wilson wrote:
> > > > > Quoting Stuart Summers (2019-08-07 21:55:55)
> > > > > > User applications might need to verify hardware
> > > > > > configuration
> > > > > > of the MOCS entries. To facilitate this debug, add a new
> > > > > > debugfs
> > > > > > entry to allow a dump of the MOCS state to verify expected
> > > > > > values
> > > > > > are set by i915.
> > > > > 
> > > > > User applications + debugfs? It's not an avenue for ABI.
> > > > > 
> > > > > If you really want to provide the settings back to userspace,
> > > > > look at
> > > > > something like an i915_query or sysfs.
> > > > > 
> > > > > Or if you just mean igt, then add a Testcase:
> > > > > 
> > > > > If you just need to validate that we are setting and
> > > > > restoring
> > > > > them,
> > > > > selftests.
> > > > > 
> > > > > If you need them for debugging errors, add them to the error
> > > > > state.
> > > > 
> > > > This was probably poorly worded, you're right. I'll update the
> > > > commit
> > > > message to be more specific.
> > > > 
> > > > I do want this for debugging, but not sure error state is the
> > > > right
> > > > place. This is for debugging performance issues, so no specific
> > > > failures. If you feel sysfs or i915_query are more correct
> > > > here, I
> > > > can
> > > > look at adding this there instead. Is there a reason we don't
> > > > want
> > > > this
> > > > in debugfs specifically?
> > > 
> > > No, it was just the wording implied to me you had a use case for
> > > clients, not just debugging the kernel.
> > > 
> > > Adding it to the error state (see i915_gpu_info) is not too bad
> > > an
> > > idea
> > > if you need a sledgehammer to inspect the GPU state while a batch
> > > is
> > > executing, but really it just sounds like you want to automate
> > > checking
> > > the mocs registers against "ideal" state. They should be static,
> > > so
> > > once
> > > they are set, so long as we are confident and check that they do
> > > not
> > > change nor can be scribbled over by userspace, you only need to
> > > scan
> > > the
> > > source :)
> > > 
> > > I will add that I wish we took a more complete snapshot of
> > > interesting
> > > registers for the error state.
> > 
> > I guess my question is about intent of the error state. I can add
> > it
> > there, but do we want this to indicate any register state we might
> > want
> > to investigate, even if the registers are "correct", but just need
> > review based on current behavior?
> 
> It was created for debugging userspace batches (later added to hang
> detection as a means of automatically grabbing the hopefully relevant
> batch). As such it's a motley collection of information that at some
> point proved useful. If you can make use of it, and find it more
> useful
> to have the mocs registers in the same snapshot as the user batch,
> please do include it. (Fwiw, I would like to extend the error state
> with
> a bunch of { offset:0xfoo, value:0xbar } given a set of tables
> listing
> the interesting regs. There just hasn't been an urgent need. Also on
> that wishlist is devcoredump.)

Ok perfect, thanks for the history here! I'll rework this into the
error state. If the intent is a catch-all where we can easily see the
state of the GPU at any given time, I agree having a large list of
registers we dump here for review would be really interesting.

Thanks,
Stuart

> -Chris