Re: [PATCH 09/53] drm/i915: Add request to execbuf params and add explicit cleanup

Tomas Elf
2015-03-05T15:37:49Z

On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Rather than just having a local request variable in the execbuff code, the
> request pointer is now stored in the execbuff params structure. Also added
> explicit cleanup of the request (plus wiping the OLR to match) in the error
> case. This means that the execbuff code is no longer dependent upon the OLR
> keeping track of the request so as to not leak it when things do go wrong. Note
> that in the success case, the i915_add_request() at the end of the submission
> function will tidy up the request and clear the OLR.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++++++++++--
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 90223f208..678b190 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1653,6 +1653,7 @@ struct i915_execbuffer_params {
> struct intel_engine_cs *ring;
> struct drm_i915_gem_object *batch_obj;
> struct intel_context *ctx;
> + struct drm_i915_gem_request *request;
> };
>
> struct drm_i915_private {
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 37dcc6f..10462f6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1353,7 +1353,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> struct i915_address_space *vm;
> struct i915_execbuffer_params params_master; /* XXX: will be removed later */
> struct i915_execbuffer_params *params = &params_master;
> - struct drm_i915_gem_request *request;
> const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
> u32 dispatch_flags;
> int ret;
> @@ -1532,7 +1531,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm);
>
> /* Allocate a request for this batch buffer nice and early. */
> - ret = dev_priv->gt.alloc_request(ring, ctx, &request);
> + ret = dev_priv->gt.alloc_request(ring, ctx, &params->request);
> if (ret)
> goto err;
>
> @@ -1565,6 +1564,16 @@ err:
> i915_gem_context_unreference(ctx);
> eb_destroy(eb);
>
> + /*
> + * If the request was created but not successfully submitted then it
> + * must be freed again. If it was submitted then it is being tracked
> + * on the active request list and no clean up is required here.
> + */
> + if (ret && params->request) {
> + i915_gem_request_unreference(params->request);
> + ring->outstanding_lazy_request = NULL;
> + }
> +
> mutex_unlock(&dev->struct_mutex);
>
> pre_mutex_err:
>

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

Thanks,
Tomas