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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx