Re: [PATCH 55/59] drm/i915: Remove fallback poll for ring buffer space

[Tomas Elf <tomas.elf@intel.com>]

On 19/03/2015 12:31, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> When the ring buffer is full, the driver finds an outstanding request that will
> free up sufficient space for the current operation and waits for it to complete.
> If no such request can be found, there is a fall back path of just polling until
> sufficient space is available.
>
> This path should not be required any more. It is a hangover from the bad days of
> OLR such that it was possible for the ring to be completely filled without ever
> having submitted a request. This can no longer happen as requests are now
> submitted in a timely manner. Hence the entire polling path is obsolete. As it
> also causes headaches in LRC land due to nesting faked requests, it is being
> removed entirely.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c        |   65 ++++---------------------------
>   drivers/gpu/drm/i915/intel_ringbuffer.c |   62 +++--------------------------
>   2 files changed, 13 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 60bcf9a..f21f449 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -622,8 +622,9 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   	return 0;
>   }
>
> -static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
> -				     int bytes)
> +static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
> +				       struct intel_context *ctx,
> +				       int bytes)
>   {
>   	struct intel_engine_cs *ring = ringbuf->ring;
>   	struct drm_i915_gem_request *request;
> @@ -652,8 +653,11 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
>   			break;
>   	}
>
> -	if (&request->list == &ring->request_list)
> +	/* It should always be possible to find a suitable request! */
> +	if (&request->list == &ring->request_list) {
> +		WARN_ON(true);

I agree with Thomas Daniel's earlier review comment that WARN_ON(true) 
is not very helpful. You could do something like:
	
	WARN_ON(ret = <expression>);
	if (ret)
		return -ENOSPC;

That way you don't have the redundancy of doing

	WARN_ON(expression);
	if (expression)
		return -ENOSPC;


and you don't have to do if (WARN_ON(expression)) if you don't like it.	

Thanks,
Tomas


>   		return -ENOSPC;
> +	}
>
>   	ret = i915_wait_request(request);
>   	if (ret)
> @@ -663,7 +667,7 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
>
>   	WARN_ON(intel_ring_space(ringbuf) < new_space);
>
> -	return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;
> +	return 0;
>   }
>
>   /*
> @@ -690,59 +694,6 @@ intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf,
>   	execlists_context_queue(ring, ctx, ringbuf->tail, request);
>   }
>
> -static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
> -				       struct intel_context *ctx,
> -				       int bytes)
> -{
> -	struct intel_engine_cs *ring = ringbuf->ring;
> -	struct drm_device *dev = ring->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long end;
> -	int ret;
> -
> -	/* The whole point of reserving space is to not wait! */
> -	WARN_ON(ringbuf->reserved_in_use);
> -
> -	ret = logical_ring_wait_request(ringbuf, bytes);
> -	if (ret != -ENOSPC)
> -		return ret;
> -
> -	/* Force the context submission in case we have been skipping it */
> -	intel_logical_ring_advance_and_submit(ringbuf, ctx, NULL);
> -
> -	/* With GEM the hangcheck timer should kick us out of the loop,
> -	 * leaving it early runs the risk of corrupting GEM state (due
> -	 * to running on almost untested codepaths). But on resume
> -	 * timers don't work yet, so prevent a complete hang in that
> -	 * case by choosing an insanely large timeout. */
> -	end = jiffies + 60 * HZ;
> -
> -	ret = 0;
> -	do {
> -		if (intel_ring_space(ringbuf) >= bytes)
> -			break;
> -
> -		msleep(1);
> -
> -		if (dev_priv->mm.interruptible && signal_pending(current)) {
> -			ret = -ERESTARTSYS;
> -			break;
> -		}
> -
> -		ret = i915_gem_check_wedge(&dev_priv->gpu_error,
> -					   dev_priv->mm.interruptible);
> -		if (ret)
> -			break;
> -
> -		if (time_after(jiffies, end)) {
> -			ret = -EBUSY;
> -			break;
> -		}
> -	} while (1);
> -
> -	return ret;
> -}
> -
>   static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf,
>   				    struct intel_context *ctx)
>   {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c5752c4..6099fce 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2063,7 +2063,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
>   	ring->buffer = NULL;
>   }
>
> -static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
> +static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>   {
>   	struct intel_ringbuffer *ringbuf = ring->buffer;
>   	struct drm_i915_gem_request *request;
> @@ -2082,8 +2082,11 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
>   			break;
>   	}
>
> -	if (&request->list == &ring->request_list)
> +	/* It should always be possible to find a suitable request! */
> +	if (&request->list == &ring->request_list) {
> +		WARN_ON(true);
>   		return -ENOSPC;
> +	}
>
>   	ret = i915_wait_request(request);
>   	if (ret)
> @@ -2096,61 +2099,6 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
>   	return 0;
>   }
>
> -static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
> -{
> -	struct drm_device *dev = ring->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_ringbuffer *ringbuf = ring->buffer;
> -	unsigned long end;
> -	int ret;
> -
> -	/* The whole point of reserving space is to not wait! */
> -	WARN_ON(ringbuf->reserved_in_use);
> -
> -	ret = intel_ring_wait_request(ring, n);
> -	if (ret != -ENOSPC)
> -		return ret;
> -
> -	/* force the tail write in case we have been skipping them */
> -	__intel_ring_advance(ring);
> -
> -	/* With GEM the hangcheck timer should kick us out of the loop,
> -	 * leaving it early runs the risk of corrupting GEM state (due
> -	 * to running on almost untested codepaths). But on resume
> -	 * timers don't work yet, so prevent a complete hang in that
> -	 * case by choosing an insanely large timeout. */
> -	end = jiffies + 60 * HZ;
> -
> -	ret = 0;
> -	trace_i915_ring_wait_begin(ring);
> -	do {
> -		if (intel_ring_space(ringbuf) >= n)
> -			break;
> -		ringbuf->head = I915_READ_HEAD(ring);
> -		if (intel_ring_space(ringbuf) >= n)
> -			break;
> -
> -		msleep(1);
> -
> -		if (dev_priv->mm.interruptible && signal_pending(current)) {
> -			ret = -ERESTARTSYS;
> -			break;
> -		}
> -
> -		ret = i915_gem_check_wedge(&dev_priv->gpu_error,
> -					   dev_priv->mm.interruptible);
> -		if (ret)
> -			break;
> -
> -		if (time_after(jiffies, end)) {
> -			ret = -EBUSY;
> -			break;
> -		}
> -	} while (1);
> -	trace_i915_ring_wait_end(ring);
> -	return ret;
> -}
> -
>   static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
>   {
>   	uint32_t __iomem *virt;
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx