| Message ID | 1355762669-6806-4-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 17 Dec 2012 18:44:29 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: > As this debugs entry can be used to set arbitrary value to next_seqno, > use i915_gem_seqno_init to set the next_seqno. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 7047c4a..e669e2e 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -903,12 +903,17 @@ i915_next_seqno_write(struct file *filp, > if (ret) > return ret; > > - if (i915_seqno_passed(val, dev_priv->next_seqno)) { > - dev_priv->next_seqno = val; > - DRM_DEBUG_DRIVER("Advancing seqno to %u\n", val); > - } else { > - ret = -EINVAL; > - } > + ret = i915_gem_init_seqno(dev, val - 1); > + if (ret) > + return ret; > + > + dev_priv->next_seqno = val; > + > + /* Carefully set the last_seqno value so that > + * wrap detection still works */ > + dev_priv->last_seqno = val - 1; > + if (dev_priv->last_seqno == 0) > + dev_priv->last_seqno--; > > mutex_unlock(&dev->struct_mutex); > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx From: Chris Wilson <chris@chris-wilson.co.uk> Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915: Introduce i915_gem_seqno_init To: Mika Kuoppala <mika.kuoppala@linux.intel.com>, intel-gfx@lists.freedesktop.org In-Reply-To: <1355762669-6806-3-git-send-email-mika.kuoppala@intel.com> References: <1355762669-6806-1-git-send-email-mika.kuoppala@intel.com> <1355762669-6806-3-git-send-email-mika.kuoppala@intel.com> On Mon, 17 Dec 2012 18:44:28 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: > i915_gem_seqno_init can set seqno to arbitrary value. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++-- > drivers/gpu/drm/i915/i915_gem.c | 8 ++++---- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 514aee8..5968340 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1479,8 +1479,8 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2) > return (int32_t)(seq1 - seq2) >= 0; > } > > -extern int i915_gem_get_seqno(struct drm_device *dev, u32 *seqno); > - > +int __must_check i915_gem_get_seqno(struct drm_device *dev, u32 *seqno); > +int __must_check i915_gem_init_seqno(struct drm_device *dev, u32 seqno); > int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj); > int __must_check i915_gem_object_put_fence(struct drm_i915_gem_object *obj); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 0231fdb..13d2067 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1925,8 +1925,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) > WARN_ON(i915_verify_lists(dev)); > } > > -static int > -i915_gem_handle_seqno_wrap(struct drm_device *dev) > +int > +i915_gem_init_seqno(struct drm_device *dev, u32 seqno) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_ring_buffer *ring; > @@ -1942,7 +1942,7 @@ i915_gem_handle_seqno_wrap(struct drm_device *dev) > > /* Finally reset hw state */ > for_each_ring(ring, dev_priv, i) { > - ret = intel_ring_init_seqno(ring, 0); > + ret = intel_ring_init_seqno(ring, seqno); > if (ret) > return ret; > > @@ -1960,7 +1960,7 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno) > > /* reserve 0 for non-seqno */ > if (dev_priv->next_seqno == 0) { > - int ret = i915_gem_handle_seqno_wrap(dev); > + int ret = i915_gem_init_seqno(dev, 0); > if (ret) > return ret; > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx From: Chris Wilson <chris@chris-wilson.co.uk> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915: Always clear semaphore mboxes on seqno wrap To: Mika Kuoppala <mika.kuoppala@linux.intel.com>, intel-gfx@lists.freedesktop.org In-Reply-To: <1355762669-6806-2-git-send-email-mika.kuoppala@intel.com> References: <1355762669-6806-1-git-send-email-mika.kuoppala@intel.com> <1355762669-6806-2-git-send-email-mika.kuoppala@intel.com> On Mon, 17 Dec 2012 18:44:27 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: > We need to clear the hw semaphore values explicitly. > Even if sync_seqnos are zero, there might still be > invalid values in the hw semaphores when we wrap. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 8dc5cc1..0231fdb 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1932,18 +1932,6 @@ i915_gem_handle_seqno_wrap(struct drm_device *dev) > struct intel_ring_buffer *ring; > int ret, i, j; > > - /* The hardware uses various monotonic 32-bit counters, if we > - * detect that they will wraparound we need to idle the GPU > - * and reset those counters. > - */ > - ret = 0; > - for_each_ring(ring, dev_priv, i) { > - for (j = 0; j < ARRAY_SIZE(ring->sync_seqno); j++) > - ret |= ring->sync_seqno[j] != 0; > - } > - if (ret == 0) > - return ret; > - > /* Carefully retire all requests without writing to the rings */ > for_each_ring(ring, dev_priv, i) { > ret = intel_ring_idle(ring); > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx From: Chris Wilson <chris@chris-wilson.co.uk> Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915: Initialize hardware semaphore state on ring init To: Mika Kuoppala <mika.kuoppala@linux.intel.com>, intel-gfx@lists.freedesktop.org In-Reply-To: <1355762669-6806-1-git-send-email-mika.kuoppala@intel.com> References: <1355762669-6806-1-git-send-email-mika.kuoppala@intel.com> On Mon, 17 Dec 2012 18:44:26 +0200, Mika Kuoppala <mika.kuoppala@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@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? How about if we assert that the ring is idle, and just blitz the registers and hws rather than go through the ring? -Chris
On Mon, 17 Dec 2012 18:44:29 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: > As this debugs entry can be used to set arbitrary value to next_seqno, > use i915_gem_seqno_init to set the next_seqno. I see where you are going, but I don't like the direction you took. :) Can you try respinning and export a less fragile function from i915_gem.c to do the dirty deed? -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Mon, 17 Dec 2012 18:44:26 +0200, Mika Kuoppala <mika.kuoppala@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@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. > 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
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Mon, 17 Dec 2012 18:44:29 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: >> As this debugs entry can be used to set arbitrary value to next_seqno, >> use i915_gem_seqno_init to set the next_seqno. > > I see where you are going, but I don't like the direction you took. :) > > Can you try respinning and export a less fragile function from > i915_gem.c to do the dirty deed? Will do. -Mika
On Tue, 18 Dec 2012 16:32:21 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > On Mon, 17 Dec 2012 18:44:26 +0200, Mika Kuoppala <mika.kuoppala@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@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 <chris@chris-wilson.co.uk> writes: > On Tue, 18 Dec 2012 16:32:21 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >> > On Mon, 17 Dec 2012 18:44:26 +0200, Mika Kuoppala <mika.kuoppala@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@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. Yes, my explanation is bogus as intel_ring_begin can't wrap in gen6_add_request. The olr is already set before going in there. >> > 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. My initial attempt write mboxes and hws directly had an error in it. I wrote seqno to mboxes also. Correct approach is to set mboxes to zero and set hws page value to seqno. I will retry this approach to see if the syncs will fail. >> 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. Thanks for explaining the reasoning. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 7047c4a..e669e2e 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -903,12 +903,17 @@ i915_next_seqno_write(struct file *filp, if (ret) return ret; - if (i915_seqno_passed(val, dev_priv->next_seqno)) { - dev_priv->next_seqno = val; - DRM_DEBUG_DRIVER("Advancing seqno to %u\n", val); - } else { - ret = -EINVAL; - } + ret = i915_gem_init_seqno(dev, val - 1); + if (ret) + return ret; + + dev_priv->next_seqno = val; + + /* Carefully set the last_seqno value so that + * wrap detection still works */ + dev_priv->last_seqno = val - 1; + if (dev_priv->last_seqno == 0) + dev_priv->last_seqno--; mutex_unlock(&dev->struct_mutex);
As this debugs entry can be used to set arbitrary value to next_seqno, use i915_gem_seqno_init to set the next_seqno. Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)