[Intel-gfx] [PATCH 4/4] drm/i915: Make next_seqno debugs entry to use i915_gem_seqno_init

[Chris Wilson]

On Tue, 18 Dec 2012 16:32:21 +0200, Mika Kuoppala <mika.kuoppala at linux.intel.com> wrote:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > On Mon, 17 Dec 2012 18:44:26 +0200, Mika Kuoppala <mika.kuoppala at linux.intel.com> wrote:
> >> Hardware status page needs to have proper seqno set
> >> as our initial seqno can be arbitrary. If initial seqno is close
> >> to wrap boundary on init and i915_seqno_passed() (31bit space)
> >> refers to hw status page which contains zero, errorneous result
> >> will be returned.
> >> 
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=58230
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> >
> > Looks good baring the last chunk.
> >
> >
> >> @@ -1435,8 +1439,18 @@ int intel_ring_handle_seqno_wrap(struct intel_ring_buffer *ring)
> >>  	 * post-wrap semaphore waits completing immediately. Clear them. */
> >>  	update_mboxes(ring, ring->signal_mbox[0]);
> >>  	update_mboxes(ring, ring->signal_mbox[1]);
> >> +	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
> >> +	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
> >> +	intel_ring_emit(ring, seqno);
> >> +	intel_ring_emit(ring, MI_USER_INTERRUPT);
> >>  	intel_ring_advance(ring);
> >>  
> >> +	/* Wait until mboxes have actually cleared before pushing
> >> +	 * anything to the rings */
> >> +	ret = ring_wait_for_space(ring, ring->size - 8);
> >> +	if (ret)
> >> +		return ret;
> >
> > I don't this is well justified. Can you clearly explain the situation
> > where it is required?
> 
> As the ring_add_request can it self cause seqno wrap due to
> intel_ring_begin, and the fact that it will update the *other* rings
> mboxes, we need to wait until all the rings have proceed with 
> clearing the mboxes.

What? What ring_add_request? The entire GPU is idle at this point.
 
> > How about if we assert that the ring is idle, and just blitz the
> > registers and hws rather than go through the ring?
> 
> I have tried this but failed. I think the problem is ring_add_request.
> It will still inject seqnos before the wrap boundary if intel_ring_begin
> in itself just wrapped. This is why we need to clear mboxes and set 
> the hws page through rings.

There are no outstanding requests at this point. The purpose of the loop
over all the rings calling idle in i915_gem_seqno_handle_wrap() is to
make sure that not only is the GPU entirely idle, but all ring->olr are
reset to 0 before we then reset the hw state on each ring.
 
> I have a patch which allocates seqnos explicitly early in
> i915_gem_do_execbuffer, gets rid of outstanding_lazy_request and 
> related i915_gem_check_olr completely thus making the wrap handling much more 
> simpler as we don't need to be careful in ring_sync nor ring_add_request
> as no cross wrap boundary stuff can no longer happen. But I got
> the impression that you don't like this approach.

No. Because requests need to be associated with anything that touches
the ring, which may themselves not be associated with an execbuffer. The
desire is to use the rings for more pipelining of state changes, not
less. The request is our most basic bookkeeping element of the ring,
they track how much of the ring we have used and provide for a unified
wait mechanism.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre