| Message ID | 1453913874-23076-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) >
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) { 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); + } 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 - 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)
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> --- drivers/gpu/drm/i915/intel_lrc.c | 41 ++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)