Re: [PATCH 44/55] drm/i915: Track requests inside each intel_ring

Joonas Lahtinen
2016-07-26T10:10:19Z

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> By tracking each request occupying space inside an individual
> intel_ring, we can greatly simplify the logic of tracking available
> space and not worry about other timelines. (Each ring is an ordered
> timeline of committed requests.)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Not such an amazing simplification in this patch, but I assume later.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas

> ---
>  drivers/gpu/drm/i915/i915_gem_request.c |  2 ++
>  drivers/gpu/drm/i915/i915_gem_request.h |  3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 15 ++++-----------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  2 ++
>  4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 0216d6c093da..f1c37b7891cb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -174,6 +174,7 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
>    * Note this requires that we are always called in request
>    * completion order.
>    */
> + list_del(&request->ring_link);
>   request->ring->last_retired_head = request->postfix;
>
>   /* Walk through the active list, calling retire on each. This allows
> @@ -472,6 +473,7 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>   request->previous_seqno = engine->last_submitted_seqno;
>   smp_store_mb(engine->last_submitted_seqno, request->fence.seqno);
>   list_add_tail(&request->link, &engine->request_list);
> + list_add_tail(&request->ring_link, &ring->request_list);
>
>   /* Record the position of the start of the request so that
>    * should we detect the updated seqno part-way through the
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index f0b91207aaa4..b1ee37896feb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -109,6 +109,9 @@ struct drm_i915_gem_request {
>   /** engine->request_list entry for this request */
>   struct list_head link;
>
> + /** ring->request_list entry for this request */
> + struct list_head ring_link;
> +
>   struct drm_i915_file_private *file_priv;
>   /** file_priv list entry for this request */
>   struct list_head client_list;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 5e0ba9416bd9..af76869c8db2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2059,6 +2059,8 @@ intel_engine_create_ring(struct intel_engine_cs *engine, int size)
>   ring->engine = engine;
>   list_add(&ring->link, &engine->buffers);
>
> + INIT_LIST_HEAD(&ring->request_list);
> +
>   ring->size = size;
>   /* Workaround an erratum on the i830 which causes a hang if
>    * the TAIL pointer points to within the last 2 cachelines
> @@ -2281,7 +2283,6 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>  static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
>  {
>   struct intel_ring *ring = req->ring;
> - struct intel_engine_cs *engine = req->engine;
>   struct drm_i915_gem_request *target;
>
>   intel_ring_update_space(ring);
> @@ -2299,17 +2300,9 @@ static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
>    */
>   GEM_BUG_ON(!req->reserved_space);
>
> - list_for_each_entry(target, &engine->request_list, link) {
> + list_for_each_entry(target, &ring->request_list, ring_link) {
>   unsigned space;
>
> - /*
> -  * 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->ring != ring)
> - continue;
> -
>   /* Would completion of this request free enough space? */
>   space = __intel_ring_space(target->postfix, ring->tail,
>      ring->size);
> @@ -2317,7 +2310,7 @@ static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
>   break;
>   }
>
> - if (WARN_ON(&target->link == &engine->request_list))
> + if (WARN_ON(&target->ring_link == &ring->request_list))
>   return -ENOSPC;
>
>   return i915_wait_request(target);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 51c059d8c917..2681106948a5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -90,6 +90,8 @@ struct intel_ring {
>   struct intel_engine_cs *engine;
>   struct list_head link;
>
> + struct list_head request_list;
> +
>   u32 head;
>   u32 tail;
>   int space;
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation