Re: [PATCH 12/18] drm/i915: Defer request emission

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

On ke, 2016-09-14 at 07:52 +0100, Chris Wilson wrote:
> +static void gen8_emit_wa_tail(struct drm_i915_gem_request *request, u32 *out)
>  {
> -	struct intel_ring *ring = request->ring;
> -	int ret;
> -
> -	ret = intel_ring_begin(request, 6 + WA_TAIL_DWORDS);
> -	if (ret)
> -		return ret;
> +	out[0] = MI_NOOP;
> +	out[1] = MI_NOOP;

How about storing 'out' on the request and still using the emit
wrapper? I assume passing a separate variable might end up rather
fragile.

> -static int gen8_rcs_signal(struct drm_i915_gem_request *req)
> +static u32 *gen8_rcs_signal(struct drm_i915_gem_request *req, u32 *out)
>  {
> -	struct intel_ring *ring = req->ring;
>  	struct drm_i915_private *dev_priv = req->i915;
>  	struct intel_engine_cs *waiter;
>  	enum intel_engine_id id;
> -	int ret, num_rings;
> -
> -	num_rings = INTEL_INFO(dev_priv)->num_rings;
> -	ret = intel_ring_begin(req, (num_rings-1) * 8);
> -	if (ret)
> -		return ret;
>  
>  	for_each_engine_id(waiter, dev_priv, id) {
>  		u64 gtt_offset = req->engine->semaphore.signal_ggtt[id];
>  		if (gtt_offset == MI_SEMAPHORE_SYNC_INVALID)
>  			continue;
>  
> -		intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(6));
> -		intel_ring_emit(ring,
> -				PIPE_CONTROL_GLOBAL_GTT_IVB |
> -				PIPE_CONTROL_QW_WRITE |
> -				PIPE_CONTROL_CS_STALL);
> -		intel_ring_emit(ring, lower_32_bits(gtt_offset));
> -		intel_ring_emit(ring, upper_32_bits(gtt_offset));
> -		intel_ring_emit(ring, req->global_seqno);
> -		intel_ring_emit(ring, 0);
> -		intel_ring_emit(ring,
> -				MI_SEMAPHORE_SIGNAL |
> -				MI_SEMAPHORE_TARGET(waiter->hw_id));
> -		intel_ring_emit(ring, 0);
> +		*out++ = GFX_OP_PIPE_CONTROL(6);
> +		*out++ = (PIPE_CONTROL_GLOBAL_GTT_IVB |
> +			  PIPE_CONTROL_QW_WRITE |
> +			  PIPE_CONTROL_CS_STALL);
> +		*out++ = lower_32_bits(gtt_offset);
> +		*out++ = upper_32_bits(gtt_offset);
> +		*out++ = req->global_seqno;
> +		*out++ = 0;
> +		*out++ = (MI_SEMAPHORE_SIGNAL |
> +			  MI_SEMAPHORE_TARGET(waiter->hw_id));
> +		*out++ = 0;

And this is mixing a different approach in... Pick one and convert
others to it. I like having the wrapper for easier grepping of emitted
commands, but YMMV.

> +static void gen6_sema_emit_request(struct drm_i915_gem_request *req,
> +				   u32 *out)
>  {
> -	int ret;
> -
> -	ret = req->engine->semaphore.signal(req);
> -	if (ret)
> -		return ret;
> -
> -	return i9xx_emit_request(req);
> +	return i9xx_emit_request(req, req->engine->semaphore.signal(req, out));

Not sure if this chaining is beautiful, I'd split signal to its own
line of "out = ...".

Those addressed,

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

Regards, Joonas
-- 
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