Re: [PATCH 24/31] drm/i915: Stop passing caller's num_dwords to engine->semaphore.signal()

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ma, 2016-07-25 at 08:44 +0100, Chris Wilson wrote:
> Rather than pass in the num_dwords that the caller wishes to use after
> the signal command packet, split the breadcrumb emission into two phases
> and have both the signal and breadcrumb individiually acquire space on
> the ring. This makes the interface simpler for the reader, and will
> simplify for patches.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 51 ++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  4 +--
>  2 files changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2fa7db5331c3..68df689a9d1e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1322,10 +1322,8 @@ static void render_ring_cleanup(struct intel_engine_cs *engine)
>  	intel_fini_pipe_control(engine);
>  }
>  
> -static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
> -			   unsigned int num_dwords)
> +static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req)
>  {
> -#define MBOX_UPDATE_DWORDS 8
>  	struct intel_ring *signaller = signaller_req->ring;
>  	struct drm_i915_private *dev_priv = signaller_req->i915;
>  	struct intel_engine_cs *waiter;
> @@ -1333,10 +1331,7 @@ static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
>  	int ret, num_rings;
>  
>  	num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask);
> -	num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS;
> -#undef MBOX_UPDATE_DWORDS
> -
> -	ret = intel_ring_begin(signaller_req, num_dwords);
> +	ret = intel_ring_begin(signaller_req, (num_rings-1) * 8);
>  	if (ret)
>  		return ret;
>  
> @@ -1360,14 +1355,13 @@ static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
>  				MI_SEMAPHORE_TARGET(waiter->hw_id));
>  		intel_ring_emit(signaller, 0);
>  	}
> +	intel_ring_advance(signaller);
>  
>  	return 0;
>  }
>  
> -static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
> -			   unsigned int num_dwords)
> +static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req)
>  {
> -#define MBOX_UPDATE_DWORDS 6
>  	struct intel_ring *signaller = signaller_req->ring;
>  	struct drm_i915_private *dev_priv = signaller_req->i915;
>  	struct intel_engine_cs *waiter;
> @@ -1375,10 +1369,7 @@ static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
>  	int ret, num_rings;
>  
>  	num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask);
> -	num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS;
> -#undef MBOX_UPDATE_DWORDS
> -
> -	ret = intel_ring_begin(signaller_req, num_dwords);
> +	ret = intel_ring_begin(signaller_req, (num_rings-1) * 6);
>  	if (ret)
>  		return ret;
>  
> @@ -1400,12 +1391,12 @@ static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
>  				MI_SEMAPHORE_TARGET(waiter->hw_id));
>  		intel_ring_emit(signaller, 0);
>  	}
> +	intel_ring_advance(signaller);
>  
>  	return 0;
>  }
>  
> -static int gen6_signal(struct drm_i915_gem_request *signaller_req,
> -		       unsigned int num_dwords)
> +static int gen6_signal(struct drm_i915_gem_request *signaller_req)
>  {
>  	struct intel_ring *signaller = signaller_req->ring;
>  	struct drm_i915_private *dev_priv = signaller_req->i915;
> @@ -1413,12 +1404,8 @@ static int gen6_signal(struct drm_i915_gem_request *signaller_req,
>  	enum intel_engine_id id;
>  	int ret, num_rings;
>  
> -#define MBOX_UPDATE_DWORDS 3
>  	num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask);
> -	num_dwords += round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2);
> -#undef MBOX_UPDATE_DWORDS
> -
> -	ret = intel_ring_begin(signaller_req, num_dwords);
> +	ret = intel_ring_begin(signaller_req, round_up((num_rings-1) * 3, 2));
>  	if (ret)
>  		return ret;
>  
> @@ -1436,6 +1423,7 @@ static int gen6_signal(struct drm_i915_gem_request *signaller_req,
>  	/* If num_dwords was rounded, make sure the tail pointer is correct */
>  	if (num_rings % 2 == 0)
>  		intel_ring_emit(signaller, MI_NOOP);
> +	intel_ring_advance(signaller);
>  
>  	return 0;
>  }
> @@ -1454,11 +1442,13 @@ static int gen6_emit_request(struct drm_i915_gem_request *req)
>  	struct intel_ring *ring = req->ring;
>  	int ret;
>  
> -	if (engine->semaphore.signal)
> -		ret = engine->semaphore.signal(req, 4);
> -	else
> -		ret = intel_ring_begin(req, 4);
> +	if (engine->semaphore.signal) {
> +		ret = engine->semaphore.signal(req);
> +		if (ret)
> +			return ret;
> +	}
>  
> +	ret = intel_ring_begin(req, 4);
>  	if (ret)
>  		return ret;
>  
> @@ -1479,10 +1469,13 @@ static int gen8_render_emit_request(struct drm_i915_gem_request *req)
>  	struct intel_ring *ring = req->ring;
>  	int ret;
>  
> -	if (engine->semaphore.signal)
> -		ret = engine->semaphore.signal(req, 8);
> -	else
> -		ret = intel_ring_begin(req, 8);
> +	if (engine->semaphore.signal) {
> +		ret = engine->semaphore.signal(req);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = intel_ring_begin(req, 8);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index fdf085495e3a..a137fd218c83 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -278,9 +278,7 @@ struct intel_engine_cs {
>  		int	(*sync_to)(struct drm_i915_gem_request *to_req,
>  				   struct intel_engine_cs *from,
>  				   u32 seqno);
> -		int	(*signal)(struct drm_i915_gem_request *signaller_req,
> -				  /* num_dwords needed by caller */
> -				  unsigned int num_dwords);
> +		int	(*signal)(struct drm_i915_gem_request *signaller_req);
>  	} semaphore;
>  
>  	/* Execlists */
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx