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

[Ben Widawsky <benjamin.widawsky@intel.com>]

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