On ti, 2016-04-26 at 21:06 +0100, Chris Wilson wrote:
> Combine the near identical implementations of intel_logical_ring_begin()
> and intel_ring_begin() - the only difference is that the logical wait
> has to check for a matching ring (which is assumed by legacy).
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 146 ++-----------------------
> drivers/gpu/drm/i915/intel_lrc.h | 1 -
> drivers/gpu/drm/i915/intel_mocs.c | 12 +--
> drivers/gpu/drm/i915/intel_ringbuffer.c | 182 ++++++++++++--------------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 -
> 5 files changed, 81 insertions(+), 263 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 2b7e6bbc017b..ba87c94928e7 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -721,48 +721,6 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> return ret;
> }
>
> -static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
> - int bytes)
> -{
> - struct intel_ringbuffer *ringbuf = req->ringbuf;
> - struct intel_engine_cs *engine = req->engine;
> - struct drm_i915_gem_request *target;
> - unsigned space;
> - int ret;
> -
> - if (intel_ring_space(ringbuf) >= bytes)
> - return 0;
> -
> - /* The whole point of reserving space is to not wait! */
> - WARN_ON(ringbuf->reserved_in_use);
> -
> - list_for_each_entry(target, &engine->request_list, list) {
> - /*
> - * The request queue is per-engine, so can contain requests
> - * from multiple ringbuffers. Here, we must ignore any that
> - * aren't from the ringbuffer we're considering.
> - */
> - if (target->ringbuf != ringbuf)
> - continue;
> -
> - /* Would completion of this request free enough space? */
> - space = __intel_ring_space(target->postfix, ringbuf->tail,
> - ringbuf->size);
> - if (space >= bytes)
> - break;
> - }
> -
> - if (WARN_ON(&target->list == &engine->request_list))
> - return -ENOSPC;
> -
> - ret = i915_wait_request(target);
> - if (ret)
> - return ret;
> -
> - ringbuf->space = space;
> - return 0;
> -}
> -
> /*
> * intel_logical_ring_advance_and_submit() - advance the tail and submit the workload
> * @request: Request to advance the logical ringbuffer of.
> @@ -814,92 +772,6 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
> return 0;
> }
>
> -static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
> -{
> - uint32_t __iomem *virt;
> - int rem = ringbuf->size - ringbuf->tail;
> -
> - virt = ringbuf->virtual_start + ringbuf->tail;
> - rem /= 4;
> - while (rem--)
> - iowrite32(MI_NOOP, virt++);
> -
> - ringbuf->tail = 0;
> - intel_ring_update_space(ringbuf);
> -}
> -
> -static int logical_ring_prepare(struct drm_i915_gem_request *req, int bytes)
> -{
> - struct intel_ringbuffer *ringbuf = req->ringbuf;
> - int remain_usable = ringbuf->effective_size - ringbuf->tail;
> - int remain_actual = ringbuf->size - ringbuf->tail;
> - int ret, total_bytes, wait_bytes = 0;
> - bool need_wrap = false;
> -
> - if (ringbuf->reserved_in_use)
> - total_bytes = bytes;
> - else
> - total_bytes = bytes + ringbuf->reserved_size;
> -
> - if (unlikely(bytes > remain_usable)) {
> - /*
> - * Not enough space for the basic request. So need to flush
> - * out the remainder and then wait for base + reserved.
> - */
> - wait_bytes = remain_actual + total_bytes;
> - need_wrap = true;
> - } else {
> - if (unlikely(total_bytes > remain_usable)) {
> - /*
> - * The base request will fit but the reserved space
> - * falls off the end. So don't need an immediate wrap
> - * and only need to effectively wait for the reserved
> - * size space from the start of ringbuffer.
> - */
> - wait_bytes = remain_actual + ringbuf->reserved_size;
> - } else if (total_bytes > ringbuf->space) {
> - /* No wrapping required, just waiting. */
> - wait_bytes = total_bytes;
> - }
> - }
> -
> - if (wait_bytes) {
> - ret = logical_ring_wait_for_space(req, wait_bytes);
> - if (unlikely(ret))
> - return ret;
> -
> - if (need_wrap)
> - __wrap_ring_buffer(ringbuf);
> - }
> -
> - return 0;
> -}
> -
> -/**
> - * intel_logical_ring_begin() - prepare the logical ringbuffer to accept some commands
> - *
> - * @req: The request to start some new work for
> - * @num_dwords: number of DWORDs that we plan to write to the ringbuffer.
> - *
> - * The ringbuffer might not be ready to accept the commands right away (maybe it needs to
> - * be wrapped, or wait a bit for the tail to be updated). This function takes care of that
> - * and also preallocates a request (every workload submission is still mediated through
> - * requests, same as it did with legacy ringbuffer submission).
> - *
> - * Return: non-zero if the ringbuffer is not ready to be written to.
> - */
> -int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
> -{
> - int ret;
> -
> - ret = logical_ring_prepare(req, num_dwords * sizeof(uint32_t));
> - if (ret)
> - return ret;
> -
> - req->ringbuf->space -= num_dwords * sizeof(uint32_t);
> - return 0;
> -}
> -
> int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request)
> {
> /*
> @@ -912,7 +784,7 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request)
> */
> intel_ring_reserved_space_reserve(request->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
>
> - return intel_logical_ring_begin(request, 0);
> + return intel_ring_begin(request, 0);
> }
>
> /**
> @@ -982,7 +854,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>
> if (engine == &dev_priv->engine[RCS] &&
> instp_mode != dev_priv->relative_constants_mode) {
> - ret = intel_logical_ring_begin(params->request, 4);
> + ret = intel_ring_begin(params->request, 4);
> if (ret)
> return ret;
>
> @@ -1178,7 +1050,7 @@ static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
> if (ret)
> return ret;
>
> - ret = intel_logical_ring_begin(req, w->count * 2 + 2);
> + ret = intel_ring_begin(req, w->count * 2 + 2);
> if (ret)
> return ret;
>
> @@ -1669,7 +1541,7 @@ static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
> const int num_lri_cmds = GEN8_LEGACY_PDPES * 2;
> int i, ret;
>
> - ret = intel_logical_ring_begin(req, num_lri_cmds * 2 + 2);
> + ret = intel_ring_begin(req, num_lri_cmds * 2 + 2);
> if (ret)
> return ret;
>
> @@ -1716,7 +1588,7 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
> req->ctx->ppgtt->pd_dirty_rings &= ~intel_engine_flag(req->engine);
> }
>
> - ret = intel_logical_ring_begin(req, 4);
> + ret = intel_ring_begin(req, 4);
> if (ret)
> return ret;
>
> @@ -1778,7 +1650,7 @@ static int gen8_emit_flush(struct drm_i915_gem_request *request,
> uint32_t cmd;
> int ret;
>
> - ret = intel_logical_ring_begin(request, 4);
> + ret = intel_ring_begin(request, 4);
> if (ret)
> return ret;
>
> @@ -1846,7 +1718,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
> vf_flush_wa = true;
> }
>
> - ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6);
> + ret = intel_ring_begin(request, vf_flush_wa ? 12 : 6);
> if (ret)
> return ret;
>
> @@ -1920,7 +1792,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_ring_begin(request, 6 + WA_TAIL_DWORDS);
> if (ret)
> return ret;
>
> @@ -1944,7 +1816,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, 8 + WA_TAIL_DWORDS);
> + ret = intel_ring_begin(request, 8 + WA_TAIL_DWORDS);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 461f1ef9b5c1..60a7385bc531 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -63,7 +63,6 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
> void intel_logical_ring_stop(struct intel_engine_cs *engine);
> void intel_logical_ring_cleanup(struct intel_engine_cs *engine);
> int intel_logical_rings_init(struct drm_device *dev);
> -int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords);
>
> int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
> /**
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 23b8545ad6b0..6ba4bf7f2a89 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -239,11 +239,9 @@ static int emit_mocs_control_table(struct drm_i915_gem_request *req,
> if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
> return -ENODEV;
>
> - ret = intel_logical_ring_begin(req, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
> - if (ret) {
> - DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
These debugs disappear in silence. Could be worthy a note in the commit
message.
> + ret = intel_ring_begin(req, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
> + if (ret)
> return ret;
> - }
>
> intel_logical_ring_emit(ringbuf,
> MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES));
> @@ -305,11 +303,9 @@ static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
> if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
> return -ENODEV;
>
> - ret = intel_logical_ring_begin(req, 2 + GEN9_NUM_MOCS_ENTRIES);
> - if (ret) {
> - DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
This too.
> + ret = intel_ring_begin(req, 2 + GEN9_NUM_MOCS_ENTRIES);
> + if (ret)
> return ret;
> - }
>
> intel_logical_ring_emit(ringbuf,
> MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2));
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 61c120aed11e..1d3d2ea3e9bc 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -53,12 +53,6 @@ void intel_ring_update_space(struct intel_ringbuffer *ringbuf)
> ringbuf->tail, ringbuf->size);
> }
>
> -int intel_ring_space(struct intel_ringbuffer *ringbuf)
> -{
> - intel_ring_update_space(ringbuf);
> - return ringbuf->space;
> -}
> -
> bool intel_engine_stopped(struct intel_engine_cs *engine)
> {
> struct drm_i915_private *dev_priv = engine->dev->dev_private;
> @@ -2318,51 +2312,6 @@ void intel_cleanup_engine(struct intel_engine_cs *engine)
> engine->dev = NULL;
> }
>
> -static int ring_wait_for_space(struct intel_engine_cs *engine, int n)
> -{
> - struct intel_ringbuffer *ringbuf = engine->buffer;
> - struct drm_i915_gem_request *request;
> - unsigned space;
> - int ret;
> -
> - if (intel_ring_space(ringbuf) >= n)
> - return 0;
> -
> - /* The whole point of reserving space is to not wait! */
> - WARN_ON(ringbuf->reserved_in_use);
> -
> - list_for_each_entry(request, &engine->request_list, list) {
> - space = __intel_ring_space(request->postfix, ringbuf->tail,
> - ringbuf->size);
> - if (space >= n)
> - break;
> - }
> -
> - if (WARN_ON(&request->list == &engine->request_list))
> - return -ENOSPC;
> -
> - ret = i915_wait_request(request);
> - if (ret)
> - return ret;
> -
> - ringbuf->space = space;
> - return 0;
> -}
> -
> -static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
> -{
> - uint32_t __iomem *virt;
> - int rem = ringbuf->size - ringbuf->tail;
> -
> - virt = ringbuf->virtual_start + ringbuf->tail;
> - rem /= 4;
> - while (rem--)
> - iowrite32(MI_NOOP, virt++);
> -
> - ringbuf->tail = 0;
> - intel_ring_update_space(ringbuf);
> -}
> -
> int intel_engine_idle(struct intel_engine_cs *engine)
> {
> struct drm_i915_gem_request *req;
> @@ -2404,63 +2353,74 @@ int intel_ring_reserve_space(struct drm_i915_gem_request *request)
>
> void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size)
> {
> - WARN_ON(ringbuf->reserved_size);
> - WARN_ON(ringbuf->reserved_in_use);
> -
> + GEM_BUG_ON(ringbuf->reserved_size);
> ringbuf->reserved_size = size;
> }
>
> void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf)
> {
> - WARN_ON(ringbuf->reserved_in_use);
> -
> + GEM_BUG_ON(!ringbuf->reserved_size);
> ringbuf->reserved_size = 0;
> - ringbuf->reserved_in_use = false;
> }
>
> void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf)
> {
> - WARN_ON(ringbuf->reserved_in_use);
> -
> - ringbuf->reserved_in_use = true;
> - ringbuf->reserved_tail = ringbuf->tail;
> + GEM_BUG_ON(!ringbuf->reserved_size);
> + ringbuf->reserved_size = 0;
> }
>
Above two functions are now the same, I'd remove the _cancel variant.
> void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
> {
> - WARN_ON(!ringbuf->reserved_in_use);
> - if (ringbuf->tail > ringbuf->reserved_tail) {
> - WARN(ringbuf->tail > ringbuf->reserved_tail + ringbuf->reserved_size,
> - "request reserved size too small: %d vs %d!\n",
> - ringbuf->tail - ringbuf->reserved_tail, ringbuf->reserved_size);
> - } else {
> + GEM_BUG_ON(ringbuf->reserved_size);
> +}
> +
> +static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
> +{
> + struct intel_ringbuffer *ringbuf = req->ringbuf;
> + struct intel_engine_cs *engine = req->engine;
> + struct drm_i915_gem_request *target;
> +
> + intel_ring_update_space(ringbuf);
> + if (ringbuf->space >= bytes)
> + return 0;
> +
> + /* The whole point of reserving space is to not wait! */
> + GEM_BUG_ON(!ringbuf->reserved_size);
reserved_size = 0 would indicate we're at __i915_add_request, but the
comment and test are not clearest. reserved_size being zero does not
directly indicate "aha, reserved bytes are being used", it could very
well be no reserved_size was requested. But right.
> +
> + list_for_each_entry(target, &engine->request_list, list) {
> + unsigned space;
> +
> /*
> - * The ring was wrapped while the reserved space was in use.
> - * That means that some unknown amount of the ring tail was
> - * no-op filled and skipped. Thus simply adding the ring size
> - * to the tail and doing the above space check will not work.
> - * Rather than attempt to track how much tail was skipped,
> - * it is much simpler to say that also skipping the sanity
> - * check every once in a while is not a big issue.
> + * The request queue is per-engine, so can contain requests
> + * from multiple ringbuffers. Here, we must ignore any that
> + * aren't from the ringbuffer we're considering.
> */
> + if (target->ringbuf != ringbuf)
> + continue;
> +
> + /* Would completion of this request free enough space? */
> + space = __intel_ring_space(target->postfix, ringbuf->tail,
> + ringbuf->size);
> + if (space >= bytes)
> + break;
> }
>
> - ringbuf->reserved_size = 0;
> - ringbuf->reserved_in_use = false;
> + if (WARN_ON(&target->list == &engine->request_list))
> + return -ENOSPC;
> +
> + return i915_wait_request(target);
> }
>
> -static int __intel_ring_prepare(struct intel_engine_cs *engine, int bytes)
> +int intel_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
> {
> - struct intel_ringbuffer *ringbuf = engine->buffer;
> - int remain_usable = ringbuf->effective_size - ringbuf->tail;
> + struct intel_ringbuffer *ringbuf = req->ringbuf;
> int remain_actual = ringbuf->size - ringbuf->tail;
> - int ret, total_bytes, wait_bytes = 0;
> + int remain_usable = ringbuf->effective_size - ringbuf->tail;
> + int bytes = num_dwords * sizeof(u32);
> + int total_bytes, wait_bytes;
> bool need_wrap = false;
>
> - if (ringbuf->reserved_in_use)
> - total_bytes = bytes;
> - else
> - total_bytes = bytes + ringbuf->reserved_size;
> + total_bytes = bytes + ringbuf->reserved_size;
>
> if (unlikely(bytes > remain_usable)) {
> /*
> @@ -2469,44 +2429,38 @@ static int __intel_ring_prepare(struct intel_engine_cs *engine, int bytes)
> */
> wait_bytes = remain_actual + total_bytes;
> need_wrap = true;
> - } else {
> - if (unlikely(total_bytes > remain_usable)) {
> - /*
> - * The base request will fit but the reserved space
> - * falls off the end. So don't need an immediate wrap
> - * and only need to effectively wait for the reserved
> - * size space from the start of ringbuffer.
> - */
> - wait_bytes = remain_actual + ringbuf->reserved_size;
> - } else if (total_bytes > ringbuf->space) {
> - /* No wrapping required, just waiting. */
> - wait_bytes = total_bytes;
> - }
> - }
> + } else if (unlikely(total_bytes > remain_usable)) {
> + /*
> + * The base request will fit but the reserved space
> + * falls off the end. So we don't need an immediate wrap
> + * and only need to effectively wait for the reserved
> + * size space from the start of ringbuffer.
> + */
> + wait_bytes = remain_actual + ringbuf->reserved_size;
> + } else
Should have braces here in the final else.
> + /* No wrapping required, just waiting. */
> + wait_bytes = total_bytes;
>
> - if (wait_bytes) {
> - ret = ring_wait_for_space(engine, wait_bytes);
> + if (wait_bytes > ringbuf->space) {
> + int ret = wait_for_space(req, wait_bytes);
> if (unlikely(ret))
> return ret;
>
> - if (need_wrap)
> - __wrap_ring_buffer(ringbuf);
> + intel_ring_update_space(ringbuf);
> }
>
> - return 0;
> -}
> + if (unlikely(need_wrap)) {
> + GEM_BUG_ON(remain_actual > ringbuf->space);
> + GEM_BUG_ON(ringbuf->tail + remain_actual > ringbuf->size);
>
> -int intel_ring_begin(struct drm_i915_gem_request *req,
> - int num_dwords)
> -{
> - struct intel_engine_cs *engine = req->engine;
> - int ret;
> -
> - ret = __intel_ring_prepare(engine, num_dwords * sizeof(uint32_t));
> - if (ret)
> - return ret;
> + memset(ringbuf->virtual_start + ringbuf->tail,
> + 0, remain_actual);
I'd like to see MI_NOOP or at least a comment that this is not a casual
clear to 0.
With above addressed,
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> + ringbuf->tail = 0;
> + ringbuf->space -= remain_actual;
> + }
>
> - engine->buffer->space -= num_dwords * sizeof(uint32_t);
> + ringbuf->space -= bytes;
> + GEM_BUG_ON(ringbuf->space < 0);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 2ade194bbea9..201e7752d765 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -108,8 +108,6 @@ struct intel_ringbuffer {
> int size;
> int effective_size;
> int reserved_size;
> - int reserved_tail;
> - bool reserved_in_use;
>
> /** We track the position of the requests in the ring buffer, and
> * when each is retired we increment last_retired_head as the GPU
> @@ -459,7 +457,6 @@ static inline void intel_ring_advance(struct intel_engine_cs *engine)
> }
> int __intel_ring_space(int head, int tail, int size);
> void intel_ring_update_space(struct intel_ringbuffer *ringbuf);
> -int intel_ring_space(struct intel_ringbuffer *ringbuf);
> bool intel_engine_stopped(struct intel_engine_cs *engine);
>
> int __must_check intel_engine_idle(struct intel_engine_cs *engine);
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
