Re: [RFC 19/21] drm/i915: Convert semaphores to handle requests not seqnos

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 06, 2014 at 03:15:23PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com

This smells funky on a quick look. I'd expect some reference counting in
here, or at least an explanatation for why it's not needed.

But even without that I'm not sure we want to track requests at the
semaphore level. After all hw semaphores work with seqno numbers and not
requests, and depending upon how the scheduler materializes seqnos
shoveling requests through these functions makes no sense at all.

I'm maybe missing something big here, but with the thin commit message I
can't tell. So I vote to just drop this one for now.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |    3 ++-
>  drivers/gpu/drm/i915/i915_gem.c         |   14 ++++++--------
>  drivers/gpu/drm/i915/i915_gpu_error.c   |   12 ++++++++----
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   10 ++++++----
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    4 ++--
>  5 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e764af9..df53515 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2604,7 +2604,8 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
>  	seq_puts(m, "\nSync seqno:\n");
>  	for_each_ring(ring, dev_priv, i) {
>  		for (j = 0; j < num_rings; j++) {
> -			seq_printf(m, "  0x%08x ", ring->semaphore.sync_seqno[j]);
> +			seq_printf(m, "  0x%08x ",
> +				   i915_gem_request_get_seqno(ring->semaphore.sync_req[j]));
>  		}
>  		seq_putc(m, '\n');
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 2e1ebad..d40dad7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2275,8 +2275,8 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
>  	for_each_ring(ring, dev_priv, i) {
>  		intel_ring_init_seqno(ring, seqno);
>  
> -		for (j = 0; j < ARRAY_SIZE(ring->semaphore.sync_seqno); j++)
> -			ring->semaphore.sync_seqno[j] = 0;
> +		for (j = 0; j < ARRAY_SIZE(ring->semaphore.sync_req); j++)
> +			ring->semaphore.sync_req[j] = NULL;
>  	}
>  
>  	return 0;
> @@ -2877,7 +2877,6 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>  		     struct intel_engine_cs *to)
>  {
>  	struct intel_engine_cs *from = obj->ring;
> -	u32 seqno;
>  	int ret, idx;
>  
>  	if (from == NULL || to == from)
> @@ -2888,10 +2887,10 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>  
>  	idx = intel_ring_sync_index(from, to);
>  
> -	seqno = i915_gem_request_get_seqno(obj->last_read_req);
>  	/* Optimization: Avoid semaphore sync when we are sure we already
>  	 * waited for an object with higher seqno */
> -	if (seqno <= from->semaphore.sync_seqno[idx]) /* <--- broken?! needs to use i915_seqno_passed()??? */
> +	if (i915_gem_request_get_seqno(obj->last_read_req) <=
> +		i915_gem_request_get_seqno(from->semaphore.sync_req[idx])) /* <--- broken?! needs to use i915_seqno_passed()??? */
>  		return 0;
>  
>  	ret = i915_gem_check_olr(obj->last_read_req);
> @@ -2899,14 +2898,13 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>  		return ret;
>  
>  	trace_i915_gem_ring_sync_to(from, to, obj->last_read_req);
> -	ret = to->semaphore.sync_to(to, from, seqno);
> +	ret = to->semaphore.sync_to(to, from, obj->last_read_req);
>  	if (!ret)
>  		/* We use last_read_req because sync_to()
>  		 * might have just caused seqno wrap under
>  		 * the radar.
>  		 */
> -		from->semaphore.sync_seqno[idx] =
> -				i915_gem_request_get_seqno(obj->last_read_req);
> +		from->semaphore.sync_req[idx] = obj->last_read_req;
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 1b58390..9545d96 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -822,7 +822,8 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
>  		idx = intel_ring_sync_index(ring, to);
>  
>  		ering->semaphore_mboxes[idx] = tmp[signal_offset];
> -		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
> +		ering->semaphore_seqno[idx] =
> +		      i915_gem_request_get_seqno(ring->semaphore.sync_req[idx]);
>  	}
>  }
>  
> @@ -832,13 +833,16 @@ static void gen6_record_semaphore_state(struct drm_i915_private *dev_priv,
>  {
>  	ering->semaphore_mboxes[0] = I915_READ(RING_SYNC_0(ring->mmio_base));
>  	ering->semaphore_mboxes[1] = I915_READ(RING_SYNC_1(ring->mmio_base));
> -	ering->semaphore_seqno[0] = ring->semaphore.sync_seqno[0];
> -	ering->semaphore_seqno[1] = ring->semaphore.sync_seqno[1];
> +	ering->semaphore_seqno[0] =
> +			i915_gem_request_get_seqno(ring->semaphore.sync_req[0]);
> +	ering->semaphore_seqno[1] =
> +			i915_gem_request_get_seqno(ring->semaphore.sync_req[1]);
>  
>  	if (HAS_VEBOX(dev_priv->dev)) {
>  		ering->semaphore_mboxes[2] =
>  			I915_READ(RING_SYNC_2(ring->mmio_base));
> -		ering->semaphore_seqno[2] = ring->semaphore.sync_seqno[2];
> +		ering->semaphore_seqno[2] =
> +			i915_gem_request_get_seqno(ring->semaphore.sync_req[2]);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c8ec78c..0a3c24a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1042,7 +1042,7 @@ static inline bool i915_gem_has_seqno_wrapped(struct drm_device *dev,
>  static int
>  gen8_ring_sync(struct intel_engine_cs *waiter,
>  	       struct intel_engine_cs *signaller,
> -	       u32 seqno)
> +	       struct drm_i915_gem_request *req)
>  {
>  	struct drm_i915_private *dev_priv = waiter->dev->dev_private;
>  	int ret;
> @@ -1055,7 +1055,7 @@ gen8_ring_sync(struct intel_engine_cs *waiter,
>  				MI_SEMAPHORE_GLOBAL_GTT |
>  				MI_SEMAPHORE_POLL |
>  				MI_SEMAPHORE_SAD_GTE_SDD);
> -	intel_ring_emit(waiter, seqno);
> +	intel_ring_emit(waiter, i915_gem_request_get_seqno(req));
>  	intel_ring_emit(waiter,
>  			lower_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
>  	intel_ring_emit(waiter,
> @@ -1067,18 +1067,20 @@ gen8_ring_sync(struct intel_engine_cs *waiter,
>  static int
>  gen6_ring_sync(struct intel_engine_cs *waiter,
>  	       struct intel_engine_cs *signaller,
> -	       u32 seqno)
> +	       struct drm_i915_gem_request *req)
>  {
>  	u32 dw1 = MI_SEMAPHORE_MBOX |
>  		  MI_SEMAPHORE_COMPARE |
>  		  MI_SEMAPHORE_REGISTER;
>  	u32 wait_mbox = signaller->semaphore.mbox.wait[waiter->id];
>  	int ret;
> +	u32 seqno;
>  
>  	/* Throughout all of the GEM code, seqno passed implies our current
>  	 * seqno is >= the last seqno executed. However for hardware the
>  	 * comparison is strictly greater than.
>  	 */
> +	seqno = i915_gem_request_get_seqno(req);
>  	seqno -= 1;
>  
>  	WARN_ON(wait_mbox == MI_SEMAPHORE_SYNC_INVALID);
> @@ -1794,7 +1796,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  	INIT_LIST_HEAD(&ring->execlist_queue);
>  	ringbuf->size = 32 * PAGE_SIZE;
>  	ringbuf->ring = ring;
> -	memset(ring->semaphore.sync_seqno, 0, sizeof(ring->semaphore.sync_seqno));
> +	memset(ring->semaphore.sync_req, 0, sizeof(ring->semaphore.sync_req));
>  
>  	init_waitqueue_head(&ring->irq_queue);
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 5475046..64a4346 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -211,7 +211,7 @@ struct  intel_engine_cs {
>  	 *  ie. transpose of f(x, y)
>  	 */
>  	struct {
> -		u32	sync_seqno[I915_NUM_RINGS-1];
> +		struct drm_i915_gem_request *sync_req[I915_NUM_RINGS-1];
>  
>  		union {
>  			struct {
> @@ -226,7 +226,7 @@ struct  intel_engine_cs {
>  		/* AKA wait() */
>  		int	(*sync_to)(struct intel_engine_cs *ring,
>  				   struct intel_engine_cs *to,
> -				   u32 seqno);
> +				   struct drm_i915_gem_request *req);
>  		int	(*signal)(struct intel_engine_cs *signaller,
>  				  /* num_dwords needed by caller */
>  				  unsigned int num_dwords);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch