Re: [PATCH 15/17] drm/i915/bdw: collect semaphore error state

[Ben Widawsky <ben@bwidawsk.net>]

On Sat, Dec 14, 2013 at 09:38:57PM +0000, Chris Wilson wrote:
> On Sat, Dec 14, 2013 at 11:47:24AM -0800, Ben Widawsky wrote:
> > On Fri, Dec 13, 2013 at 08:16:03PM -0800, Ben Widawsky wrote:
> > > Since the semaphore information is in an object, just dump it, and let
> > > the user parse it later.
> > > 
> > > NOTE: The page being used for the semaphores are incoherent with the
> > > CPU. No matter what I do, I cannot figure out a way to read anything but
> > > 0s. Note that the semaphore waits are indeed working.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > >  
> > 
> > [snip]
> > 
> > > +static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
> > > +					struct drm_i915_error_state *error,
> > > +					struct intel_ring_buffer *ring)
> > > +{
> > > +	struct intel_ring_buffer *useless;
> > > +	int i;
> > > +
> > > +	if (!i915_semaphore_is_enabled(dev_priv->dev))
> > > +		return;
> > > +
> > > +	if (!error->semaphore_obj)
> > > +		error->semaphore_obj =
> > > +			i915_error_object_create(dev_priv,
> > > +						 dev_priv->semaphore_obj);
> > > +
> > > +	for_each_ring(useless, dev_priv, i) {
> > > +		u16 signal_offset = GEN8_SIGNAL_OFFSET(ring, i) / 4;
> > > +		u16 wait_offset = GEN8_WAIT_OFFSET(ring, i) / 4;
> > > +		u32 *tmp = error->semaphore_obj->pages[0];
> > > +
> > > +		error->semaphore_mboxes[ring->id][i] = tmp[signal_offset];
> > > +		error->semaphore_seqno[ring->id][i] = tmp[wait_offset];
> > > +	}
> > > +}
> > > +
> > 
> > Chris, I preemptively assume this rubs you the wrong way. I had been
> > trying to do the analogous of the I915_READ from previous gens. I felt
> > it was more consistent with reading of hardware state. If you want me to
> > simply use the ring->semaphore.*, I can. I just wanted to make my
> > intention clear.
> 
> Capturing the semaphore_obj itself looks a bit silly since we never do
> dump it into i915_error_state, but I actually like how you've reading back of
> the semaphore page for debugging hangs.

I don't understand this comment. The semaphore page is dumped, though I
snipped that, so maybe that's why you made the comment.

> And don't we also include our own semaphore seqno values in the
> i915_error_state as well? Oh, I see now. You've overwritten those
> values. Whoops. We do need to keep ring->sync_seqno[] in the debug
> output for a certain class of (driver) bug. So it looks like we could
> do with all three values (current signal, last wait, last known sync)
> in the error state.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center