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

[Chris Wilson <chris@chris-wilson.co.uk>]

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