Re: [PATCH v6 12/25] drm/i915: Unify intel_ring_begin()

Joonas Lahtinen
2016-04-27T09:20:29Z

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@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>  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@linux.intel.com>

> + 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