On 02/18/2016 06:27 AM, john.c.harri...@intel.com wrote:
> From: John Harrison <john.c.harri...@intel.com>
> 
> Ring space is reserved when constructing a request to ensure that the
> subsequent 'add_request()' call cannot fail due to waiting for space
> on a busy or broken GPU. However, the scheduler jumps in to the middle
> of the execbuffer process between request creation and request
> submission. Thus it needs to cancel the reserved space when the
> request is simply added to the scheduler's queue and not yet
> submitted. Similarly, it needs to re-reserve the space when it finally
> does want to send the batch buffer to the hardware.
> 
> v3: Updated to use locally cached request pointer.
> 
> v5: Updated due to changes to earlier patches in series - for runtime
> PM calls and splitting bypass mode into a separate function.
> 
> For: VIZ-1587
> Signed-off-by: John Harrison <john.c.harri...@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++++++++++++++------
>  drivers/gpu/drm/i915/i915_scheduler.c      |  4 ++++
>  drivers/gpu/drm/i915/intel_lrc.c           | 13 +++++++++++--
>  3 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 09c5ce9..11bea8d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1295,18 +1295,22 @@ int i915_gem_ringbuffer_submission_final(struct 
> i915_execbuffer_params *params)
>       /* The mutex must be acquired before calling this function */
>       WARN_ON(!mutex_is_locked(&params->dev->struct_mutex));
>  
> +     ret = intel_ring_reserve_space(req);
> +     if (ret)
> +             goto error;
> +
>       /*
>        * Unconditionally invalidate gpu caches and ensure that we do flush
>        * any residual writes from the previous batch.
>        */
>       ret = intel_ring_invalidate_all_caches(req);
>       if (ret)
> -             return ret;
> +             goto error;
>  
>       /* Switch to the correct context for the batch */
>       ret = i915_switch_context(req);
>       if (ret)
> -             return ret;
> +             goto error;
>  
>       WARN(params->ctx->ppgtt && params->ctx->ppgtt->pd_dirty_rings & 
> (1<<ring->id),
>            "%s didn't clear reload\n", ring->name);
> @@ -1315,7 +1319,7 @@ int i915_gem_ringbuffer_submission_final(struct 
> i915_execbuffer_params *params)
>           params->instp_mode != dev_priv->relative_constants_mode) {
>               ret = intel_ring_begin(req, 4);
>               if (ret)
> -                     return ret;
> +                     goto error;
>  
>               intel_ring_emit(ring, MI_NOOP);
>               intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> @@ -1329,7 +1333,7 @@ int i915_gem_ringbuffer_submission_final(struct 
> i915_execbuffer_params *params)
>       if (params->args_flags & I915_EXEC_GEN7_SOL_RESET) {
>               ret = i915_reset_gen7_sol_offsets(params->dev, req);
>               if (ret)
> -                     return ret;
> +                     goto error;
>       }
>  
>       exec_len   = params->args_batch_len;
> @@ -1343,13 +1347,17 @@ int i915_gem_ringbuffer_submission_final(struct 
> i915_execbuffer_params *params)
>                                       exec_start, exec_len,
>                                       params->dispatch_flags);
>       if (ret)
> -             return ret;
> +             goto error;
>  
>       trace_i915_gem_ring_dispatch(req, params->dispatch_flags);
>  
>       i915_gem_execbuffer_retire_commands(params);
>  
> -     return 0;
> +error:
> +     if (ret)
> +             intel_ring_reserved_space_cancel(req->ringbuf);
> +
> +     return ret;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c 
> b/drivers/gpu/drm/i915/i915_scheduler.c
> index 3986890..a3ffd04 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -483,6 +483,8 @@ static int i915_scheduler_queue_execbuffer_bypass(struct 
> i915_scheduler_queue_en
>       struct i915_scheduler *scheduler = dev_priv->scheduler;
>       int ret;
>  
> +     intel_ring_reserved_space_cancel(qe->params.request->ringbuf);
> +
>       scheduler->flags[qe->params.ring->id] |= i915_sf_submitting;
>       ret = dev_priv->gt.execbuf_final(&qe->params);
>       scheduler->flags[qe->params.ring->id] &= ~i915_sf_submitting;
> @@ -539,6 +541,8 @@ int i915_scheduler_queue_execbuffer(struct 
> i915_scheduler_queue_entry *qe)
>       node->stamp  = jiffies;
>       i915_gem_request_reference(node->params.request);
>  
> +     intel_ring_reserved_space_cancel(node->params.request->ringbuf);
> +
>       WARN_ON(node->params.request->scheduler_qe);
>       node->params.request->scheduler_qe = node;
>  
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> b/drivers/gpu/drm/i915/intel_lrc.c
> index ff4565f..f4bab82 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -978,13 +978,17 @@ int intel_execlists_submission_final(struct 
> i915_execbuffer_params *params)
>       /* The mutex must be acquired before calling this function */
>       WARN_ON(!mutex_is_locked(&params->dev->struct_mutex));
>  
> +     ret = intel_logical_ring_reserve_space(req);
> +     if (ret)
> +             goto err;
> +
>       /*
>        * Unconditionally invalidate gpu caches and ensure that we do flush
>        * any residual writes from the previous batch.
>        */
>       ret = logical_ring_invalidate_all_caches(req);
>       if (ret)
> -             return ret;
> +             goto err;
>  
>       if (ring == &dev_priv->ring[RCS] &&
>           params->instp_mode != dev_priv->relative_constants_mode) {
> @@ -1006,13 +1010,18 @@ int intel_execlists_submission_final(struct 
> i915_execbuffer_params *params)
>  
>       ret = ring->emit_bb_start(req, exec_start, params->dispatch_flags);
>       if (ret)
> -             return ret;
> +             goto err;
>  
>       trace_i915_gem_ring_dispatch(req, params->dispatch_flags);
>  
>       i915_gem_execbuffer_retire_commands(params);
>  
>       return 0;
> +
> +err:
> +     intel_ring_reserved_space_cancel(params->request->ringbuf);
> +
> +     return ret;
>  }
>  
>  void intel_execlists_retire_requests(struct intel_engine_cs *ring)
> 

I had to double check that it was ok to cancel space if the reserve failed (I 
guess it is), so seems ok.

Reviewed-by: Jesse Barnes <jbar...@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to