[Intel-gfx] [PATCH 05/51] drm/i915: Add return code check to i915_gem_execbuffer_retire_commands()

[Daniel Vetter]

On Fri, Feb 13, 2015 at 11:48:14AM +0000, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at 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.

I was so convinced we've had a commit somewhere explaining this, but
apparently not.

The deal is that after the dispatch call we have the batch commit and
there's no going back any more, which also means we can't return an error
code from the ioctl. So if you return -EIO or -ENOMEM that's a pretty bad
lie and you really have to ignore that error code.

Again I've tried to dig up the commit for that but that was lost in the
maze of the past 5 years of changes. We've had piles of older approaches
to deal with this issue:
- Don't even emit a request, just mark objects as gpu dirty. Only when
  waiting did we emit flushes and requests, which again again gave us a
  context to return the error. This resulted in horrible latency since
  flushes where wait too late and also all that book-keeping was not worth
  it at all. Don't ask ;-)
- Emit flushes right away, but if we fail to alloc the request set the
  outstanding lazy request bit. The job of the check_olr function used in
  waits was to notice that and retry the allocation.
- Preallocate the request, but that still leaves the possibility that the
  gpu dies. But since we've committed hangcheck will clean this up and we
  can just ignore the -EIO.

Given all that backstory: Why does add_request/retire_commands suddenly
need to fail?

Cheers, Daniel

> 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 at 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 e6d616b..143bc63 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2624,10 +2624,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 ca29290..90400d0d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -706,9 +706,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)
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch