Re: [PATCH v6 15/25] drm/i915: Preallocate enough space for the average request

Tvrtko Ursulin
2016-04-27T13:15:30Z

On 26/04/16 21:06, Chris Wilson wrote:
> Rather than being interrupted when we run out of space halfway through
> the request, and having to restart from the beginning (and returning to
> userspace), flush a little more free space when we prepare the request.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 7 +++++++
> drivers/gpu/drm/i915/intel_ringbuffer.c | 16 +++++++++++++++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 01517dd7069b..b5c2c1931a5f 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -700,6 +700,12 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> {
> int ret;
>
> + /* Flush enough space to reduce the likelihood of waiting after
> + * we start building the request - in which case we will just
> + * have to repeat work.
> + */
> + request->reserved_space += MIN_SPACE_FOR_ADD_REQUEST;
> +
> request->ringbuf = request->ctx->engine[request->engine->id].ringbuf;
>
> if (i915.enable_guc_submission) {
> @@ -725,6 +731,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> if (ret)
> goto err_unpin;
>
> + request->reserved_space -= MIN_SPACE_FOR_ADD_REQUEST;

Without any previous experience with ringbuf space reservation and
related, this does make sense to me. :)

So why add and subtract and not just increase MIN_SPACE_FOR_ADD_REQUEST?
Or if MIN_SPACE_FOR_ADD_REQUEST is completely unrelated to the
subsequent stuff to go in, and perhaps only represent the typical driver
prologue & epilogue, why increase the reserved size temporarily by that
amount and not something else?

Regards,

Tvrtko

> return 0;
>
> err_unpin:
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1193372f74fd..1285605f25c7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2332,8 +2332,22 @@ int intel_engine_idle(struct intel_engine_cs *engine)
>
> int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
> {
> + int ret;
> +
> + /* Flush enough space to reduce the likelihood of waiting after
> + * we start building the request - in which case we will just
> + * have to repeat work.
> + */
> + request->reserved_space += MIN_SPACE_FOR_ADD_REQUEST;
> +
> request->ringbuf = request->engine->buffer;
> - return intel_ring_begin(request, 0);
> +
> + ret = intel_ring_begin(request, 0);
> + if (ret)
> + return ret;
> +
> + request->reserved_space -= MIN_SPACE_FOR_ADD_REQUEST;
> + return 0;
> }
>
> static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
>