Re: [PATCH 49/53] drm/i915: Make intel_logical_ring_begin() static

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

On 19/02/2015 17:18, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> The only usage of intel_logical_ring_begin() is within intel_lrc.c so it can be
> made static. To avoid a forward declaration at the top of the file, it and bunch
> of other functions have been shuffled upwards.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c |  394 +++++++++++++++++++-------------------
>   drivers/gpu/drm/i915/intel_lrc.h |    3 -
>   2 files changed, 197 insertions(+), 200 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5212b54..b40501d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -606,6 +606,203 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
>   	return logical_ring_invalidate_all_caches(req);
>   }
>
> +static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
> +				     int bytes)
> +{
> +	struct intel_engine_cs *ring = ringbuf->ring;
> +	struct drm_i915_gem_request *request;
> +	int ret;
> +
> +	if (intel_ring_space(ringbuf) >= bytes)
> +		return 0;
> +
> +	list_for_each_entry(request, &ring->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.
> +		 */
> +		struct intel_context *ctx = request->ctx;
> +		if (ctx->engine[ring->id].ringbuf != ringbuf)
> +			continue;
> +
> +		/* Would completion of this request free enough space? */
> +		if (__intel_ring_space(request->tail, ringbuf->tail,
> +				       ringbuf->size) >= bytes) {
> +			break;
> +		}
> +	}
> +
> +	if (&request->list == &ring->request_list)
> +		return -ENOSPC;
> +
> +	ret = i915_wait_request(request);
> +	if (ret)
> +		return ret;
> +
> +	i915_gem_retire_requests_ring(ring);
> +
> +	return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;
> +}
> +
> +/*
> + * intel_logical_ring_advance_and_submit() - advance the tail and submit the workload
> + * @ringbuf: Logical Ringbuffer to advance.
> + *
> + * The tail is updated in our logical ringbuffer struct, not in the actual context. What
> + * really happens during submission is that the context and current tail will be placed
> + * on a queue waiting for the ELSP to be ready to accept a new context submission. At that
> + * point, the tail *inside* the context is updated and the ELSP written to.
> + */
> +static void
> +intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf,
> +				      struct intel_context *ctx,
> +				      struct drm_i915_gem_request *request)
> +{
> +	struct intel_engine_cs *ring = ringbuf->ring;
> +
> +	intel_logical_ring_advance(ringbuf);
> +
> +	if (intel_ring_stopped(ring))
> +		return;
> +
> +	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;
> +
> +	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)
> +{
> +	uint32_t __iomem *virt;
> +	int rem = ringbuf->size - ringbuf->tail;
> +
> +	if (ringbuf->space < rem) {
> +		int ret = logical_ring_wait_for_space(ringbuf, ctx, rem);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
> +	virt = ringbuf->virtual_start + ringbuf->tail;
> +	rem /= 4;
> +	while (rem--)
> +		iowrite32(MI_NOOP, virt++);
> +
> +	ringbuf->tail = 0;
> +	intel_ring_update_space(ringbuf);
> +
> +	return 0;
> +}
> +
> +static int logical_ring_prepare(struct intel_ringbuffer *ringbuf,
> +				struct intel_context *ctx, int bytes)
> +{
> +	int ret;
> +
> +	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> +		ret = logical_ring_wrap_buffer(ringbuf, ctx);
> +		if (unlikely(ret))
> +			return ret;
> +	}
> +
> +	if (unlikely(ringbuf->space < bytes)) {
> +		ret = logical_ring_wait_for_space(ringbuf, ctx, bytes);
> +		if (unlikely(ret))
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * intel_logical_ring_begin() - prepare the logical ringbuffer to accept some commands
> + *
> + * @ringbuf: Logical ringbuffer.
> + * @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.
> + */
> +static int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
> +				    struct intel_context *ctx, int num_dwords)
> +{
> +	struct drm_i915_gem_request *req;
> +	struct intel_engine_cs *ring = ringbuf->ring;
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	ret = i915_gem_check_wedge(&dev_priv->gpu_error,
> +				   dev_priv->mm.interruptible);
> +	if (ret)
> +		return ret;
> +
> +	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
> +	if (ret)
> +		return ret;
> +
> +	/* Preallocate the olr before touching the ring */
> +	ret = intel_logical_ring_alloc_request(ring, ctx, &req);
> +	if (ret)
> +		return ret;
> +
> +	ringbuf->space -= num_dwords * sizeof(uint32_t);
> +	return 0;
> +}
> +
>   /**
>    * execlists_submission() - submit a batchbuffer for execution, Execlists style
>    * @dev: DRM device.
> @@ -783,30 +980,6 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
>   	return 0;
>   }
>
> -/*
> - * intel_logical_ring_advance_and_submit() - advance the tail and submit the workload
> - * @ringbuf: Logical Ringbuffer to advance.
> - *
> - * The tail is updated in our logical ringbuffer struct, not in the actual context. What
> - * really happens during submission is that the context and current tail will be placed
> - * on a queue waiting for the ELSP to be ready to accept a new context submission. At that
> - * point, the tail *inside* the context is updated and the ELSP written to.
> - */
> -static void
> -intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf,
> -				      struct intel_context *ctx,
> -				      struct drm_i915_gem_request *request)
> -{
> -	struct intel_engine_cs *ring = ringbuf->ring;
> -
> -	intel_logical_ring_advance(ringbuf);
> -
> -	if (intel_ring_stopped(ring))
> -		return;
> -
> -	execlists_context_queue(ring, ctx, ringbuf->tail, request);
> -}
> -
>   static int intel_lr_context_pin(struct intel_engine_cs *ring,
>   		struct intel_context *ctx)
>   {
> @@ -901,179 +1074,6 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring,
>   	return 0;
>   }
>
> -static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
> -				     int bytes)
> -{
> -	struct intel_engine_cs *ring = ringbuf->ring;
> -	struct drm_i915_gem_request *request;
> -	int ret;
> -
> -	if (intel_ring_space(ringbuf) >= bytes)
> -		return 0;
> -
> -	list_for_each_entry(request, &ring->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.
> -		 */
> -		struct intel_context *ctx = request->ctx;
> -		if (ctx->engine[ring->id].ringbuf != ringbuf)
> -			continue;
> -
> -		/* Would completion of this request free enough space? */
> -		if (__intel_ring_space(request->tail, ringbuf->tail,
> -				       ringbuf->size) >= bytes) {
> -			break;
> -		}
> -	}
> -
> -	if (&request->list == &ring->request_list)
> -		return -ENOSPC;
> -
> -	ret = i915_wait_request(request);
> -	if (ret)
> -		return ret;
> -
> -	i915_gem_retire_requests_ring(ring);
> -
> -	return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;
> -}
> -
> -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;
> -
> -	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)
> -{
> -	uint32_t __iomem *virt;
> -	int rem = ringbuf->size - ringbuf->tail;
> -
> -	if (ringbuf->space < rem) {
> -		int ret = logical_ring_wait_for_space(ringbuf, ctx, rem);
> -
> -		if (ret)
> -			return ret;
> -	}
> -
> -	virt = ringbuf->virtual_start + ringbuf->tail;
> -	rem /= 4;
> -	while (rem--)
> -		iowrite32(MI_NOOP, virt++);
> -
> -	ringbuf->tail = 0;
> -	intel_ring_update_space(ringbuf);
> -
> -	return 0;
> -}
> -
> -static int logical_ring_prepare(struct intel_ringbuffer *ringbuf,
> -				struct intel_context *ctx, int bytes)
> -{
> -	int ret;
> -
> -	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> -		ret = logical_ring_wrap_buffer(ringbuf, ctx);
> -		if (unlikely(ret))
> -			return ret;
> -	}
> -
> -	if (unlikely(ringbuf->space < bytes)) {
> -		ret = logical_ring_wait_for_space(ringbuf, ctx, bytes);
> -		if (unlikely(ret))
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -/**
> - * intel_logical_ring_begin() - prepare the logical ringbuffer to accept some commands
> - *
> - * @ringbuf: Logical ringbuffer.
> - * @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 intel_ringbuffer *ringbuf,
> -			     struct intel_context *ctx, int num_dwords)
> -{
> -	struct drm_i915_gem_request *req;
> -	struct intel_engine_cs *ring = ringbuf->ring;
> -	struct drm_device *dev = ring->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret;
> -
> -	ret = i915_gem_check_wedge(&dev_priv->gpu_error,
> -				   dev_priv->mm.interruptible);
> -	if (ret)
> -		return ret;
> -
> -	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
> -	if (ret)
> -		return ret;
> -
> -	/* Preallocate the olr before touching the ring */
> -	ret = intel_logical_ring_alloc_request(ring, ctx, &req);
> -	if (ret)
> -		return ret;
> -
> -	ringbuf->space -= num_dwords * sizeof(uint32_t);
> -	return 0;
> -}
> -
>   static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
>   {
>   	int ret, i;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 40848b0..dceecc6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -65,9 +65,6 @@ static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf,
>   	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
>   	ringbuf->tail += 4;
>   }
> -int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
> -			     struct intel_context *ctx,
> -			     int num_dwords);
>
>   /* Logical Ring Contexts */
>   void intel_lr_context_free(struct intel_context *ctx);
>

Reviewed-by: Tomas Elf <tomas.elf@intel.com>

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