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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 19/03/2015 12:31, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>

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@xxxxxxxxx>
---
  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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux