[PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Rodrigo Vivi]

semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
This optimization is to remove the ring itself from the list and the logic to do that
is at intel_ring_sync_index as below:

/*
 * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
 * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
 * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
 * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
 * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
*/

Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9faebbc..36a7960 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 					struct intel_engine_cs *ring,
 					struct drm_i915_error_ring *ering)
 {
-	struct intel_engine_cs *useless;
+	struct intel_engine_cs *to;
 	int i;
 
 	if (!i915_semaphore_is_enabled(dev_priv->dev))
@@ -776,13 +776,14 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 						 dev_priv->semaphore_obj,
 						 &dev_priv->gtt.base);
 
-	for_each_ring(useless, dev_priv, i) {
+	for_each_ring(to, dev_priv, i) {
 		u16 signal_offset =
 			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
 		u32 *tmp = error->semaphore_obj->pages[0];
+		int idx = intel_ring_sync_index(ring, to);
 
-		ering->semaphore_mboxes[i] = tmp[signal_offset];
-		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
+		ering->semaphore_mboxes[idx] = tmp[signal_offset];
+		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
 	}
 }
 
-- 
1.9.3

Re: [PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Ben Widawsky]

On Thu, Jul 17, 2014 at 05:35:20AM -0700, Rodrigo Vivi wrote:
> semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
> This optimization is to remove the ring itself from the list and the logic to do that
> is at intel_ring_sync_index as below:
> 
> /*
>  * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
>  * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
>  * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
>  * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
>  * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
> */
> 
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 9faebbc..36a7960 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
>  					struct intel_engine_cs *ring,
>  					struct drm_i915_error_ring *ering)
>  {
> -	struct intel_engine_cs *useless;
> +	struct intel_engine_cs *to;
>  	int i;
>  
>  	if (!i915_semaphore_is_enabled(dev_priv->dev))
> @@ -776,13 +776,14 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
>  						 dev_priv->semaphore_obj,
>  						 &dev_priv->gtt.base);
>  
> -	for_each_ring(useless, dev_priv, i) {
> +	for_each_ring(to, dev_priv, i) {
>  		u16 signal_offset =
>  			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
>  		u32 *tmp = error->semaphore_obj->pages[0];
> +		int idx = intel_ring_sync_index(ring, to);
>  
> -		ering->semaphore_mboxes[i] = tmp[signal_offset];
> -		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
> +		ering->semaphore_mboxes[idx] = tmp[signal_offset];
> +		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
>  	}
>  }
>  

Actually, this patch does the opposite of my original intent. Since the
semaphore object should have the proper spacing, my original intent was
what your first patch was: Fix semaphore_seqno and semaphore_mboxes
sizes (with probably a comment in the error state why we do this).

That patch is simpler and gives potentially more useful information
(like if we write to the wrong offset in the semaphore page, we won't
see it here). UNFORTUNATELY it does not seem to correspond with how we
actually print out the error state. So I think unless you want to fix up
the other spots, your other patch is incorrect.

This patch looks correct to me, and should fix the bug with the least
amount of churn. In addition, assuming we have no bugs, it shows things
more concisely in the error state. Perhaps just add my concern about
missing capture certain offsets within the semaphore object in the
commit message and call it good.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Damien Lespiau]

On Thu, Jul 17, 2014 at 02:31:14PM -0700, Ben Widawsky wrote:
> > -	for_each_ring(useless, dev_priv, i) {
> > +	for_each_ring(to, dev_priv, i) {
> >  		u16 signal_offset =
> >  			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
> >  		u32 *tmp = error->semaphore_obj->pages[0];
> > +		int idx = intel_ring_sync_index(ring, to);
> >  
> > -		ering->semaphore_mboxes[i] = tmp[signal_offset];
> > -		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
> > +		ering->semaphore_mboxes[idx] = tmp[signal_offset];
> > +		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
> >  	}
> >  }
> >  
> This patch looks correct to me.

We're still looping over all the rings aren't we? we'll override the
array when ring == to?

-- 
Damien

[PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Rodrigo Vivi]

semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
This optimization is to remove the ring itself from the list and the logic to do that
is at intel_ring_sync_index as below:

/*
 * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
 * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
 * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
 * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
 * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
*/

v2: Skip when from == to (Damien).

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9faebbc..6608bee 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 					struct intel_engine_cs *ring,
 					struct drm_i915_error_ring *ering)
 {
-	struct intel_engine_cs *useless;
+	struct intel_engine_cs *to;
 	int i;
 
 	if (!i915_semaphore_is_enabled(dev_priv->dev))
@@ -776,13 +776,17 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 						 dev_priv->semaphore_obj,
 						 &dev_priv->gtt.base);
 
-	for_each_ring(useless, dev_priv, i) {
+	for_each_ring(to, dev_priv, i) {
 		u16 signal_offset =
 			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
 		u32 *tmp = error->semaphore_obj->pages[0];
+		int idx = intel_ring_sync_index(ring, to);
 
-		ering->semaphore_mboxes[i] = tmp[signal_offset];
-		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
+		if (ring->id == to->id)
+			return;
+
+		ering->semaphore_mboxes[idx] = tmp[signal_offset];
+		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
 	}
 }
 
-- 
1.9.3

Re: [PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Damien Lespiau]

On Fri, Jul 18, 2014 at 01:39:29AM -0700, Rodrigo Vivi wrote:
> semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
> This optimization is to remove the ring itself from the list and the logic to do that
> is at intel_ring_sync_index as below:
> 
> /*
>  * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
>  * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
>  * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
>  * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
>  * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
> */
> 
> v2: Skip when from == to (Damien).
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 9faebbc..6608bee 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
>  					struct intel_engine_cs *ring,
>  					struct drm_i915_error_ring *ering)
>  {
> -	struct intel_engine_cs *useless;
> +	struct intel_engine_cs *to;
>  	int i;
>  
>  	if (!i915_semaphore_is_enabled(dev_priv->dev))
> @@ -776,13 +776,17 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
>  						 dev_priv->semaphore_obj,
>  						 &dev_priv->gtt.base);
>  
> -	for_each_ring(useless, dev_priv, i) {
> +	for_each_ring(to, dev_priv, i) {
>  		u16 signal_offset =
>  			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
>  		u32 *tmp = error->semaphore_obj->pages[0];
> +		int idx = intel_ring_sync_index(ring, to);
>  
> -		ering->semaphore_mboxes[i] = tmp[signal_offset];
> -		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
> +		if (ring->id == to->id)
> +			return;

continue; ? you need to skip "ring", but you also need to fill the array
when to->id > ring->id.

I guess you should also be able to short-circuit the iteration sooner as
well, no need to do the computations. I believe if "(ring == to)" would
work as well.

-- 
Damien

[PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Rodrigo Vivi]

semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
This optimization is to remove the ring itself from the list and the logic to do that
is at intel_ring_sync_index as below:

/*
 * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
 * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
 * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
 * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
 * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
*/

v2: Skip when from == to (Damien).
v3: avoid computing idx when from == to (Damien).
    use ring == to instead of ring->id == to->id (Damien).
    use continue instead of return (Rodrigo).

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9faebbc..1efcf1f 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -764,8 +764,8 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 					struct intel_engine_cs *ring,
 					struct drm_i915_error_ring *ering)
 {
-	struct intel_engine_cs *useless;
-	int i;
+	struct intel_engine_cs *to;
+	int i, idx;
 
 	if (!i915_semaphore_is_enabled(dev_priv->dev))
 		return;
@@ -776,13 +776,18 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 						 dev_priv->semaphore_obj,
 						 &dev_priv->gtt.base);
 
-	for_each_ring(useless, dev_priv, i) {
+	for_each_ring(to, dev_priv, i) {
 		u16 signal_offset =
 			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
 		u32 *tmp = error->semaphore_obj->pages[0];
 
-		ering->semaphore_mboxes[i] = tmp[signal_offset];
-		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
+		if (ring == to)
+			continue;
+
+		idx = intel_ring_sync_index(ring, to);
+
+		ering->semaphore_mboxes[idx] = tmp[signal_offset];
+		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
 	}
 }
 
-- 
1.9.3

Re: [PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Damien Lespiau]

On Fri, Jul 18, 2014 at 02:05:16AM -0700, Rodrigo Vivi wrote:
> semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
> This optimization is to remove the ring itself from the list and the logic to do that
> is at intel_ring_sync_index as below:
> 
> /*
>  * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
>  * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
>  * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
>  * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
>  * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
> */
> 
> v2: Skip when from == to (Damien).
> v3: avoid computing idx when from == to (Damien).
>     use ring == to instead of ring->id == to->id (Damien).
>     use continue instead of return (Rodrigo).
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I guess there are still some details that look weird:

  - idx's scope could be reduced to the loop
  - there's still some code that is executed on the skipped iteration
    that doesn't need to be.

But I believe we shouldn't overflow now at least, so, whether you can be
bothered with those last 2 comments or not:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 9faebbc..1efcf1f 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -764,8 +764,8 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
>  					struct intel_engine_cs *ring,
>  					struct drm_i915_error_ring *ering)
>  {
> -	struct intel_engine_cs *useless;
> -	int i;
> +	struct intel_engine_cs *to;
> +	int i, idx;
>  
>  	if (!i915_semaphore_is_enabled(dev_priv->dev))
>  		return;
> @@ -776,13 +776,18 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
>  						 dev_priv->semaphore_obj,
>  						 &dev_priv->gtt.base);
>  
> -	for_each_ring(useless, dev_priv, i) {
> +	for_each_ring(to, dev_priv, i) {
>  		u16 signal_offset =
>  			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
>  		u32 *tmp = error->semaphore_obj->pages[0];
>  
> -		ering->semaphore_mboxes[i] = tmp[signal_offset];
> -		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
> +		if (ring == to)
> +			continue;
> +
> +		idx = intel_ring_sync_index(ring, to);
> +
> +		ering->semaphore_mboxes[idx] = tmp[signal_offset];
> +		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
>  	}
>  }
>  
> -- 
> 1.9.3
> 

[PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Rodrigo Vivi]

semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
This optimization is to remove the ring itself from the list and the logic to do that
is at intel_ring_sync_index as below:

/*
 * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
 * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
 * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
 * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
 * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
*/

v2: Skip when from == to (Damien).
v3: avoid computing idx when from == to (Damien).
    use ring == to instead of ring->id == to->id (Damien).
    use continue instead of return (Rodrigo).
v4: avoid all unecessary computation (Damien).
    reduce idx to loop scope (Damien).

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9faebbc..0b3f694 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 					struct intel_engine_cs *ring,
 					struct drm_i915_error_ring *ering)
 {
-	struct intel_engine_cs *useless;
+	struct intel_engine_cs *to;
 	int i;
 
 	if (!i915_semaphore_is_enabled(dev_priv->dev))
@@ -776,13 +776,20 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 						 dev_priv->semaphore_obj,
 						 &dev_priv->gtt.base);
 
-	for_each_ring(useless, dev_priv, i) {
-		u16 signal_offset =
-			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
-		u32 *tmp = error->semaphore_obj->pages[0];
+	for_each_ring(to, dev_priv, i) {
+		int idx;
+		u16 signal_offset;
+		u32 *tmp;
 
-		ering->semaphore_mboxes[i] = tmp[signal_offset];
-		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
+		if (ring == to)
+			continue;
+
+		signal_offset = (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
+		tmp = error->semaphore_obj->pages[0];
+		idx = intel_ring_sync_index(ring, to);
+
+		ering->semaphore_mboxes[idx] = tmp[signal_offset];
+		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
 	}
 }
 
-- 
1.9.3

Re: [PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Ben Widawsky]

On Fri, Jul 18, 2014 at 02:19:40AM -0700, Rodrigo Vivi wrote:
> semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
> This optimization is to remove the ring itself from the list and the logic to do that
> is at intel_ring_sync_index as below:
> 
> /*
>  * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
>  * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
>  * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
>  * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
>  * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
> */
> 
> v2: Skip when from == to (Damien).
> v3: avoid computing idx when from == to (Damien).
>     use ring == to instead of ring->id == to->id (Damien).
>     use continue instead of return (Rodrigo).
> v4: avoid all unecessary computation (Damien).
>     reduce idx to loop scope (Damien).
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 9faebbc..0b3f694 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
>  					struct intel_engine_cs *ring,
>  					struct drm_i915_error_ring *ering)
>  {
> -	struct intel_engine_cs *useless;
> +	struct intel_engine_cs *to;
>  	int i;
>  
>  	if (!i915_semaphore_is_enabled(dev_priv->dev))
> @@ -776,13 +776,20 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
>  						 dev_priv->semaphore_obj,
>  						 &dev_priv->gtt.base);
>  
> -	for_each_ring(useless, dev_priv, i) {
> -		u16 signal_offset =
> -			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
> -		u32 *tmp = error->semaphore_obj->pages[0];
> +	for_each_ring(to, dev_priv, i) {
> +		int idx;
> +		u16 signal_offset;
> +		u32 *tmp;
>  
> -		ering->semaphore_mboxes[i] = tmp[signal_offset];
> -		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
> +		if (ring == to)
> +			continue;
> +
> +		signal_offset = (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
> +		tmp = error->semaphore_obj->pages[0];
> +		idx = intel_ring_sync_index(ring, to);
> +
> +		ering->semaphore_mboxes[idx] = tmp[signal_offset];
> +		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
>  	}
>  }
>  

Just elaborate on previous email from your v1, now that you've fixed the
error state printing, I would have rather not skip the check and instead
expand the array. This would help us find stray, or unexpected writes
with either future bugs, or buggy hardware.

I'm not asking for a v5 (but I did ask you to make a note of what we
miss in the commit message when I responded to v1... but that's okay
too). I am simply explaining the earlier mail in case it was unclear. If
a v5 *does* need to happen anyway, that is still my preference, but I
don't care too much.

(Also, I think v2 in your commit message was (Ben), not (Damien) but
perhaps I missed a conversation somewhere)

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

P.S.
I'd be in favor of adding BUG_ON(idx >= I915_NUM_RINGS) before return in
intel_ring_sync_index().

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Daniel Vetter]

On Fri, Jul 18, 2014 at 07:00:40PM -0700, Ben Widawsky wrote:
> On Fri, Jul 18, 2014 at 02:19:40AM -0700, Rodrigo Vivi wrote:
> > semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
> > This optimization is to remove the ring itself from the list and the logic to do that
> > is at intel_ring_sync_index as below:
> > 
> > /*
> >  * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
> >  * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
> >  * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
> >  * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
> >  * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
> > */
> > 
> > v2: Skip when from == to (Damien).
> > v3: avoid computing idx when from == to (Damien).
> >     use ring == to instead of ring->id == to->id (Damien).
> >     use continue instead of return (Rodrigo).
> > v4: avoid all unecessary computation (Damien).
> >     reduce idx to loop scope (Damien).
> > 
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gpu_error.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 9faebbc..0b3f694 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
> >  					struct intel_engine_cs *ring,
> >  					struct drm_i915_error_ring *ering)
> >  {
> > -	struct intel_engine_cs *useless;
> > +	struct intel_engine_cs *to;
> >  	int i;
> >  
> >  	if (!i915_semaphore_is_enabled(dev_priv->dev))
> > @@ -776,13 +776,20 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
> >  						 dev_priv->semaphore_obj,
> >  						 &dev_priv->gtt.base);
> >  
> > -	for_each_ring(useless, dev_priv, i) {
> > -		u16 signal_offset =
> > -			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
> > -		u32 *tmp = error->semaphore_obj->pages[0];
> > +	for_each_ring(to, dev_priv, i) {
> > +		int idx;
> > +		u16 signal_offset;
> > +		u32 *tmp;
> >  
> > -		ering->semaphore_mboxes[i] = tmp[signal_offset];
> > -		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
> > +		if (ring == to)
> > +			continue;
> > +
> > +		signal_offset = (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
> > +		tmp = error->semaphore_obj->pages[0];
> > +		idx = intel_ring_sync_index(ring, to);
> > +
> > +		ering->semaphore_mboxes[idx] = tmp[signal_offset];
> > +		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
> >  	}
> >  }
> >  
> 
> Just elaborate on previous email from your v1, now that you've fixed the
> error state printing, I would have rather not skip the check and instead
> expand the array. This would help us find stray, or unexpected writes
> with either future bugs, or buggy hardware.
> 
> I'm not asking for a v5 (but I did ask you to make a note of what we
> miss in the commit message when I responded to v1... but that's okay
> too). I am simply explaining the earlier mail in case it was unclear. If
> a v5 *does* need to happen anyway, that is still my preference, but I
> don't care too much.
> 
> (Also, I think v2 in your commit message was (Ben), not (Damien) but
> perhaps I missed a conversation somewhere)

We could do this as a follow-up, merged the current version to dinq.
> 
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> P.S.
> I'd be in favor of adding BUG_ON(idx >= I915_NUM_RINGS) before return in
> intel_ring_sync_index().

BUG_ON considered harmful, please use WARN_ON instead. But not sure how
much this is worth it here really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch