Re: [PATCH 50/53] drm/i915: Update intel_logical_ring_begin() to take a request structure

Tomas Elf
2015-03-09T23:33:54Z

On 19/02/2015 17:18, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Now that everything above has been converted to use requests,
> intel_logical_ring_begin() can be updated to take a request instead of a
> ringbuf/context pair. This also means that it no longer needs to lazily allocate
> a request if no-one happens to have done it earlier.
>
> Note that this change makes the execlist signature the same as the legacy
> version. Thus the two functions could be merged into a ring->begin() wrapper if
> required.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 33 +++++++++++++++------------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index b40501d..2911cf6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -766,7 +766,7 @@ static int logical_ring_prepare(struct intel_ringbuffer *ringbuf,
> /**
> * intel_logical_ring_begin() - prepare the logical ringbuffer to accept some commands
> *
> - * @ringbuf: Logical ringbuffer.
> + * @request: The request to start some new work for
> * @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
> @@ -776,30 +776,27 @@ static int logical_ring_prepare(struct intel_ringbuffer *ringbuf,
> *
> * 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)
> +static int intel_logical_ring_begin(struct drm_i915_gem_request *req,
> + int num_dwords)
> {
> - struct drm_i915_gem_request *req;
> - struct intel_engine_cs *ring = ringbuf->ring;
> + struct intel_engine_cs *ring = req->ring;
> struct drm_device *dev = ring->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> int ret;
>
> + WARN_ON(req == NULL);

This WARN_ON will only fire after there's been a null pointer exception
in the req->ring dereference above. Please rearrange these so that it
warns before the null pointer exception.

Thanks,
Tomas

> +
> 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);
> + ret = logical_ring_prepare(req->ringbuf, req->ctx,
> + num_dwords * sizeof(uint32_t));
> if (ret)
> return ret;
>
> - ringbuf->space -= num_dwords * sizeof(uint32_t);
> + req->ringbuf->space -= num_dwords * sizeof(uint32_t);
> return 0;
> }
>
> @@ -885,7 +882,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>
> if (ring == &dev_priv->ring[RCS] &&
> instp_mode != dev_priv->relative_constants_mode) {
> - ret = intel_logical_ring_begin(ringbuf, params->ctx, 4);
> + ret = intel_logical_ring_begin(params->request, 4);
> if (ret)
> return ret;
>
> @@ -1091,7 +1088,7 @@ static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
> if (ret)
> return ret;
>
> - ret = intel_logical_ring_begin(ringbuf, req->ctx, w->count * 2 + 2);
> + ret = intel_logical_ring_begin(req, w->count * 2 + 2);
> if (ret)
> return ret;
>
> @@ -1173,7 +1170,7 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
> bool ppgtt = !(dispatch_flags & I915_DISPATCH_SECURE);
> int ret;
>
> - ret = intel_logical_ring_begin(ringbuf, req->ctx, 4);
> + ret = intel_logical_ring_begin(req, 4);
> if (ret)
> return ret;
>
> @@ -1231,7 +1228,7 @@ static int gen8_emit_flush(struct drm_i915_gem_request *request,
> uint32_t cmd;
> int ret;
>
> - ret = intel_logical_ring_begin(ringbuf, request->ctx, 4);
> + ret = intel_logical_ring_begin(request, 4);
> if (ret)
> return ret;
>
> @@ -1289,7 +1286,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
> flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
> }
>
> - ret = intel_logical_ring_begin(ringbuf, request->ctx, 6);
> + ret = intel_logical_ring_begin(request, 6);
> if (ret)
> return ret;
>
> @@ -1321,7 +1318,7 @@ static int gen8_emit_request(struct drm_i915_gem_request *request)
> u32 cmd;
> int ret;
>
> - ret = intel_logical_ring_begin(ringbuf, request->ctx, 6);
> + ret = intel_logical_ring_begin(request, 6);
> if (ret)
> return ret;
>
>