[PATCH 1/4] drm/i915: Initialize hardware semaphore state on ring init

[PATCH 1/4] drm/i915: Initialize hardware semaphore state on ring init

[Mika Kuoppala]

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>
---
 drivers/gpu/drm/i915/i915_gem.c         |    6 +++---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   18 ++++++++++++++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 ++-
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d15c862..8dc5cc1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1954,7 +1954,7 @@ i915_gem_handle_seqno_wrap(struct drm_device *dev)
 
 	/* Finally reset hw state */
 	for_each_ring(ring, dev_priv, i) {
-		ret = intel_ring_handle_seqno_wrap(ring);
+		ret = intel_ring_init_seqno(ring, 0);
 		if (ret)
 			return ret;
 
@@ -3933,6 +3933,8 @@ i915_gem_init_hw(struct drm_device *dev)
 
 	i915_gem_init_swizzling(dev);
 
+	dev_priv->next_seqno = dev_priv->last_seqno = (u32)~0 - 0x1000;
+
 	ret = intel_init_render_ring_buffer(dev);
 	if (ret)
 		return ret;
@@ -3949,8 +3951,6 @@ i915_gem_init_hw(struct drm_device *dev)
 			goto cleanup_bsd_ring;
 	}
 
-	dev_priv->next_seqno = (u32)-1 - 0x1000;
-
 	/*
 	 * XXX: There was some w/a described somewhere suggesting loading
 	 * contexts before PPGTT.
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 69bbe7b..7e38511f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1171,6 +1171,10 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	if (IS_I830(ring->dev) || IS_845G(ring->dev))
 		ring->effective_size -= 128;
 
+	ret = intel_ring_init_seqno(ring, dev_priv->last_seqno);
+	if (ret)
+		goto err_unmap;
+
 	return 0;
 
 err_unmap:
@@ -1418,7 +1422,7 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 	return __intel_ring_begin(ring, num_dwords * sizeof(uint32_t));
 }
 
-int intel_ring_handle_seqno_wrap(struct intel_ring_buffer *ring)
+int intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno)
 {
 	int ret;
 
@@ -1427,7 +1431,7 @@ int intel_ring_handle_seqno_wrap(struct intel_ring_buffer *ring)
 	if (INTEL_INFO(ring->dev)->gen < 6)
 		return 0;
 
-	ret = __intel_ring_begin(ring, 6 * sizeof(uint32_t));
+	ret = __intel_ring_begin(ring, 10 * sizeof(uint32_t));
 	if (ret)
 		return ret;
 
@@ -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;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index b4a533e..cb5a663 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -196,7 +196,8 @@ static inline void intel_ring_emit(struct intel_ring_buffer *ring,
 }
 void intel_ring_advance(struct intel_ring_buffer *ring);
 int __must_check intel_ring_idle(struct intel_ring_buffer *ring);
-int __must_check intel_ring_handle_seqno_wrap(struct intel_ring_buffer *ring);
+int __must_check intel_ring_init_seqno(struct intel_ring_buffer *ring,
+				       u32 seqno);
 int intel_ring_flush_all_caches(struct intel_ring_buffer *ring);
 int intel_ring_invalidate_all_caches(struct intel_ring_buffer *ring);
 
-- 
1.7.9.5

[PATCH 2/4] drm/i915: Always clear semaphore mboxes on seqno wrap

[Mika Kuoppala]

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

Re: [PATCH 2/4] drm/i915: Always clear semaphore mboxes on seqno wrap

[Chris Wilson]

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.

This changelog does not make it clear why you want this patch. This is
for enabling a future feature, right?

Anyway, after initial misgivings because I thought you were trying to
fix a bug rather than add a feature, and provided that you improve the
changelog,

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: Always clear semaphore mboxes on seqno wrap

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> 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.
>
> This changelog does not make it clear why you want this patch. This is
> for enabling a future feature, right?
>
> Anyway, after initial misgivings because I thought you were trying to
> fix a bug rather than add a feature, and provided that you improve the
> changelog,

The intent is to always clear the mboxes and also set the hws page when
seqno is set. Both when wrapping and setting seqno from debugfs. 
The hws part is needed to fix a bug where initial seqno in hws page is
zero (after boot) and driver seqno is past the 2^31 boundary.
I guess i could have split the mboxes and hws page setting apart
saving the optimization but the gain would be little.

I think this is also issue on suspend/resume if we have passed 2^31 on
suspend and we come back with driver setting initial seqno to 1.

In both cases i915_seqno_passed(ring->get_seqno(), driver_seqno) will
return errorneous value.

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

Does r-b still stand after the explanation above?

-Mika

Re: [PATCH 2/4] drm/i915: Always clear semaphore mboxes on seqno wrap

[Chris Wilson]

On Tue, 18 Dec 2012 16:19:52 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > 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.
> >
> > This changelog does not make it clear why you want this patch. This is
> > for enabling a future feature, right?
> >
> > Anyway, after initial misgivings because I thought you were trying to
> > fix a bug rather than add a feature, and provided that you improve the
> > changelog,
> 
> The intent is to always clear the mboxes and also set the hws page when
> seqno is set. Both when wrapping and setting seqno from debugfs. 
> The hws part is needed to fix a bug where initial seqno in hws page is
> zero (after boot) and driver seqno is past the 2^31 boundary.
> I guess i could have split the mboxes and hws page setting apart
> saving the optimization but the gain would be little.
> 
> I think this is also issue on suspend/resume if we have passed 2^31 on
> suspend and we come back with driver setting initial seqno to 1.
> 
> In both cases i915_seqno_passed(ring->get_seqno(), driver_seqno) will
> return errorneous value.
> 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > -Chris
> 
> Does r-b still stand after the explanation above?

Not really, that's an explanation for the series (setting initial seqno
values was incomplete, we needed to adjust the hw side as well as our
bookkeeping). But it doesn't explain why you want to change
i915_gem_seqno_handle_wrap() in this patch, which as far as I can tell
is because you want to reuse it later for a more general purpose.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/4] drm/i915: Introduce i915_gem_seqno_init

[Mika Kuoppala]

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

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

[Mika Kuoppala]

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

Re: [PATCH 4/4] drm/i915: Make next_seqno debugs entry to use i915_gem_seqno_init

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Make next_seqno debugs entry to use i915_gem_seqno_init

[Mika Kuoppala]

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

Re: [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@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, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Make next_seqno debugs entry to use i915_gem_seqno_init

[Mika Kuoppala]

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

Re: [PATCH 4/4] drm/i915: Make next_seqno debugs entry to use i915_gem_seqno_init

[Chris Wilson]

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, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Make next_seqno debugs entry to use i915_gem_seqno_init

[Mika Kuoppala]

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