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@xxxxxxxxxxxxxxxxxx>
> Cc: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
