Re: [PATCH 05/53] drm/i915: Add return code check to i915_gem_execbuffer_retire_commands()

Tomas Elf
2015-03-05T14:45:15Z

On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> For some reason, the i915_add_request() call in
> i915_gem_execbuffer_retire_commands() was explicitly having its return code
> ignored. The _retire_commands() function itself was 'void'. Given that
> _add_request() can fail without dispatching the batch buffer, this seems odd.
>
> Also shrunk the parameter list to a single structure as everything it requires
> is available in the execbuff_params object.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 +----
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 +++++++---------
> drivers/gpu/drm/i915/intel_lrc.c | 3 +--
> 3 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fdd63ab..b350910 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2640,10 +2640,7 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> void i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> struct intel_engine_cs *ring);
> -void i915_gem_execbuffer_retire_commands(struct drm_device *dev,
> - struct drm_file *file,
> - struct intel_engine_cs *ring,
> - struct drm_i915_gem_object *obj);
> +int i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params);
> int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> struct drm_i915_gem_execbuffer2 *args,
> struct list_head *vmas);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 93b0ef0..ca85803 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -989,17 +989,15 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> }
> }
>
> -void
> -i915_gem_execbuffer_retire_commands(struct drm_device *dev,
> - struct drm_file *file,
> - struct intel_engine_cs *ring,
> - struct drm_i915_gem_object *obj)
> +int
> +i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
> {
> /* Unconditionally force add_request to emit a full flush. */
> - ring->gpu_caches_dirty = true;
> + params->ring->gpu_caches_dirty = true;
>
> /* Add a breadcrumb for the completion of the batch buffer */
> - (void)__i915_add_request(ring, file, obj);
> + return __i915_add_request(params->ring, params->file,
> + params->batch_obj);
> }
>
> static int
> @@ -1282,8 +1280,8 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags);
>
> i915_gem_execbuffer_move_to_active(vmas, ring);
> - i915_gem_execbuffer_retire_commands(params->dev, params->file, ring,
> - params->batch_obj);
> +
> + ret = i915_gem_execbuffer_retire_commands(params);
>
> error:
> kfree(cliprects);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index f6a24e8a2..dc474b4 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -715,9 +715,8 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
> trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags);
>
> i915_gem_execbuffer_move_to_active(vmas, ring);
> - i915_gem_execbuffer_retire_commands(params->dev, params->file, ring, params->batch_obj);
>
> - return 0;
> + return i915_gem_execbuffer_retire_commands(params);
> }
>
> void intel_execlists_retire_requests(struct intel_engine_cs *ring)
>

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

Thanks,
Tomas