Re: [PATCH] drm/i915: Make wa_tail_dwords flexible for future platforms.

[Ben Widawsky <benjamin.widawsky@intel.com>]

On Mon, Jan 25, 2016 at 11:29:19AM -0800, Rodrigo Vivi wrote:
> Commit 7c17d3773 (drm/i915: Use ordered seqno write interrupt generation
> on gen8+ execlists) moved two MI_NOOPs to the advance_and_submit functions
> and hardcoded the WA_TAIL_DWORDS. With this we don't have a clean way to
> implement or remove WaIdleLiteRestore for different platforms.
> 
> This patch aims to let it more flexible. So we just emit the NOOPs
> equivalent of what was initialized.
> 
> Also let's just include the platforms we know that needs this Wa,
> i.e gen8 and gen9 platforms.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I don't claim to understand the reason this patch ends up being required, but it
looks fine to me. I would have gone with a bool for the workaround instead of a
count of dwords - but it's up to you. Since you allude to already knowing what
future hardware does, we know we don't need a variable length here.

> ---
>  drivers/gpu/drm/i915/intel_lrc.c        | 31 +++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  2 ++
>  2 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index da97bc5..d0b253d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -764,18 +764,18 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>  {
>  	struct intel_ringbuffer *ringbuf = request->ringbuf;
>  	struct drm_i915_private *dev_priv = request->i915;
> +	int i;
>  
>  	intel_logical_ring_advance(ringbuf);
>  	request->tail = ringbuf->tail;
>  
>  	/*
> -	 * Here we add two extra NOOPs as padding to avoid
> +	 * Here we add extra NOOPs as padding to avoid
>  	 * lite restore of a context with HEAD==TAIL.
> -	 *
> -	 * Caller must reserve WA_TAIL_DWORDS for us!
>  	 */
> -	intel_logical_ring_emit(ringbuf, MI_NOOP);
> -	intel_logical_ring_emit(ringbuf, MI_NOOP);
> +	for (i = 0; i < ringbuf->wa_tail_dwords; i++)
> +		intel_logical_ring_emit(ringbuf, MI_NOOP);
> +
>  	intel_logical_ring_advance(ringbuf);
>  
>  	if (intel_ring_stopped(request->ring))
> @@ -876,6 +876,16 @@ int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
>  	if (ret)
>  		return ret;
>  
> +	if (IS_GEN8(req->ring->dev) || IS_GEN9(req->ring->dev))
> +		/*
> +		 * 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).
> +		 */
> +		req->ringbuf->wa_tail_dwords = 2;

This should be set at ring_init, not here.

> +
> +	num_dwords += req->ringbuf->wa_tail_dwords;
> +
>  	ret = logical_ring_prepare(req, num_dwords * sizeof(uint32_t));
>  	if (ret)
>  		return ret;
> @@ -1858,13 +1868,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
> -
>  static inline u32 hws_seqno_address(struct intel_engine_cs *engine)
>  {
>  	return engine->status_page.gfx_addr + I915_GEM_HWS_INDEX_ADDR;
> @@ -1875,7 +1878,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;
>  
> @@ -1899,7 +1902,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;
>  

I think it's a lot more straightforward to do the + ringbuf->wa_tail_dwords
here, so that ring_being can be as dumb as possible.

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 566b0ae..62b4e1b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -122,6 +122,8 @@ struct intel_ringbuffer {
>  	 * we can detect new retirements.
>  	 */
>  	u32 last_retired_head;
> +
> +	int wa_tail_dwords;
>  };
>  
>  struct	intel_context;
> -- 
> 2.4.3
> 

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx