Re: [PATCH] drm/i915/execlists: Move WA_TAIL_DWORDS to callee

[Dave Gordon <david.s.gordon@intel.com>]

On 27/01/16 16:57, Chris Wilson wrote:
> Currently emit-request starts writing to the ring and reserves space for
> a workaround to be emitted later whilst submitting the request. It is
> easier to read if the caller only allocates sufficient space for its
> access (then the reader can quickly verify that the ring begin allocates
> the exact space for the number of dwords emitted) and closes the access
> to the ring. During submit, if we need to add the workaround, we can
> reacquire ring access, in the assurance that we reserved space for
> ourselves when beginning the request.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---

Generally, yes, but ...

>   drivers/gpu/drm/i915/intel_lrc.c | 41 ++++++++++++++++++++--------------------
>   1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index da97bc5666b5..74fcf0f8d97a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -760,23 +760,27 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
>    * point, the tail *inside* the context is updated and the ELSP written to.
>    */
>   static int
> -intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
> +intel_logical_ring_submit(struct drm_i915_gem_request *request)

The comment above this function still has the old name

>   {
>   	struct intel_ringbuffer *ringbuf = request->ringbuf;
>   	struct drm_i915_private *dev_priv = request->i915;
>
> -	intel_logical_ring_advance(ringbuf);
>   	request->tail = ringbuf->tail;
>
>   	/*
> -	 * Here we add two extra NOOPs as padding to avoid
> -	 * lite restore of a context with HEAD==TAIL.
> -	 *
> -	 * Caller must reserve WA_TAIL_DWORDS for us!
> +	 * Reserve space for 2 NOOPs at the end of each request to be
> +	 * used as a workaround for not being allowed to do lite
> +	 * restore with HEAD==TAIL (WaIdleLiteRestore).
>   	 */
> -	intel_logical_ring_emit(ringbuf, MI_NOOP);
> -	intel_logical_ring_emit(ringbuf, MI_NOOP);
> -	intel_logical_ring_advance(ringbuf);
> +	if (1 /* need WaIdleLiteRestore */) {
> +		int ret = intel_logical_ring_begin(request, 2);
> +		if (ret)
> +			return ret;
> +
> +		intel_logical_ring_emit(ringbuf, MI_NOOP);
> +		intel_logical_ring_emit(ringbuf, MI_NOOP);
> +		intel_logical_ring_advance(ringbuf);
> +	}

How about keeping the generalisation of emitting WA_TAIL_DWORDS of NOOPs 
(and the test can be if this is greater than 0) ...

>
>   	if (intel_ring_stopped(request->ring))
>   		return 0;
> @@ -1858,13 +1862,6 @@ static void bxt_a_set_seqno(struct intel_engine_cs *ring, u32 seqno)
>   	intel_flush_status_page(ring, I915_GEM_HWS_INDEX);
>   }
>
> -/*
> - * Reserve space for 2 NOOPs at the end of each request to be
> - * used as a workaround for not being allowed to do lite
> - * restore with HEAD==TAIL (WaIdleLiteRestore).
> - */
> -#define WA_TAIL_DWORDS 2
> -

... and keeping the define of WA_TAIL_DWORDS (but preferably moved to 
the top of the file), and changing intel_logical_ring_reserve_space() to 
add this many dwords to the space reserved.

That should make clear the connection between:
1. reserving the space (intel_logical_ring_reserve_space)
2. filling it with NOOPs (intel_logical_ring_submit)
3. using the space (execlists_context_unqueue)

because they would each mention WA_TAIL_DWORDS and WaIdleLiteRestore, 
and it will be obvious that it really is just a fix for a specific issue 
with execlist submission.

BTW the comment in execlists_context_unqueue() is (or will be) wrong 
about where the padding is added.

All the remaining changes below look good :)

.Dave.

>   static inline u32 hws_seqno_address(struct intel_engine_cs *engine)
>   {
>   	return engine->status_page.gfx_addr + I915_GEM_HWS_INDEX_ADDR;
> @@ -1875,7 +1872,7 @@ static int gen8_emit_request(struct drm_i915_gem_request *request)
>   	struct intel_ringbuffer *ringbuf = request->ringbuf;
>   	int ret;
>
> -	ret = intel_logical_ring_begin(request, 6 + WA_TAIL_DWORDS);
> +	ret = intel_logical_ring_begin(request, 6);
>   	if (ret)
>   		return ret;
>
> @@ -1891,7 +1888,9 @@ static int gen8_emit_request(struct drm_i915_gem_request *request)
>   	intel_logical_ring_emit(ringbuf, i915_gem_request_get_seqno(request));
>   	intel_logical_ring_emit(ringbuf, MI_USER_INTERRUPT);
>   	intel_logical_ring_emit(ringbuf, MI_NOOP);
> -	return intel_logical_ring_advance_and_submit(request);
> +	intel_logical_ring_advance(ringbuf);
> +
> +	return intel_logical_ring_submit(request);
>   }
>
>   static int gen8_emit_request_render(struct drm_i915_gem_request *request)
> @@ -1899,7 +1898,7 @@ static int gen8_emit_request_render(struct drm_i915_gem_request *request)
>   	struct intel_ringbuffer *ringbuf = request->ringbuf;
>   	int ret;
>
> -	ret = intel_logical_ring_begin(request, 6 + WA_TAIL_DWORDS);
> +	ret = intel_logical_ring_begin(request, 6);
>   	if (ret)
>   		return ret;
>
> @@ -1916,7 +1915,9 @@ static int gen8_emit_request_render(struct drm_i915_gem_request *request)
>   	intel_logical_ring_emit(ringbuf, 0);
>   	intel_logical_ring_emit(ringbuf, i915_gem_request_get_seqno(request));
>   	intel_logical_ring_emit(ringbuf, MI_USER_INTERRUPT);
> -	return intel_logical_ring_advance_and_submit(request);
> +	intel_logical_ring_advance(ringbuf);
> +
> +	return intel_logical_ring_submit(request);
>   }
>
>   static int intel_lr_context_render_state_init(struct drm_i915_gem_request *req)
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx