From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mika Kuoppala Subject: Re: [PATCH 4/4] drm/i915: Make next_seqno debugs entry to use i915_gem_seqno_init Date: Tue, 18 Dec 2012 16:32:21 +0200 Message-ID: <87fw33e8i2.fsf@gaia.fi.intel.com> References: <1355762669-6806-1-git-send-email-mika.kuoppala@intel.com> <1355762669-6806-4-git-send-email-mika.kuoppala@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 5F293E62C7 for ; Tue, 18 Dec 2012 06:32:24 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org Chris Wilson writes: > On Mon, 17 Dec 2012 18:44:26 +0200, Mika Kuoppala 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 > > 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. > How about if we assert that the ring is idle, and just blitz the > registers and hws rather than go through the ring? > -Chris 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. 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. -Mika > -- > Chris Wilson, Intel Open Source Technology Centre