Re: [PATCH 08/17] drm/i915: Drop spinlocks around adding to the client request list

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Adding to the tail of the client request list as the only other user is
> in the throttle ioctl that iterates forwards over the list. It only
> needs protection against deletion of a request as it reads it, it simply
> won't see a new request added to the end of the list, or it would be too
> early and rejected. We can further reduce the number of spinlocks
> required when throttling by removing stale requests from the client_list
> as we throttle.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Revieved-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c            | 14 ++++++------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 +++++++-----
>  drivers/gpu/drm/i915/i915_gem_request.c    | 34 ++++++------------------------
>  drivers/gpu/drm/i915/i915_gem_request.h    |  4 +---
>  5 files changed, 23 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 086053fa2820..996744708f31 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -480,7 +480,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
>  		mutex_lock(&dev->struct_mutex);
>  		request = list_first_entry_or_null(&file_priv->mm.request_list,
>  						   struct drm_i915_gem_request,
> -						   client_list);
> +						   client_link);
>  		rcu_read_lock();
>  		task = pid_task(request && request->ctx->pid ?
>  				request->ctx->pid : file->pid,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7b8abda541e6..e432211e8b24 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3673,16 +3673,14 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
>  		return -EIO;
>  
>  	spin_lock(&file_priv->mm.lock);
> -	list_for_each_entry(request, &file_priv->mm.request_list, client_list) {
> +	list_for_each_entry(request, &file_priv->mm.request_list, client_link) {
>  		if (time_after_eq(request->emitted_jiffies, recent_enough))
>  			break;
>  
> -		/*
> -		 * Note that the request might not have been submitted yet.
> -		 * In which case emitted_jiffies will be zero.
> -		 */
> -		if (!request->emitted_jiffies)
> -			continue;
> +		if (target) {
> +			list_del(&target->client_link);
> +			target->file_priv = NULL;
> +		}
>  
>  		target = request;
>  	}
> @@ -4639,7 +4637,7 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
>  	 * file_priv.
>  	 */
>  	spin_lock(&file_priv->mm.lock);
> -	list_for_each_entry(request, &file_priv->mm.request_list, client_list)
> +	list_for_each_entry(request, &file_priv->mm.request_list, client_link)
>  		request->file_priv = NULL;
>  	spin_unlock(&file_priv->mm.lock);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 125fb38eff40..5689445b1cd3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1421,6 +1421,14 @@ out:
>  	return vma;
>  }
>  
> +static void
> +add_to_client(struct drm_i915_gem_request *req,
> +	      struct drm_file *file)
> +{
> +	req->file_priv = file->driver_priv;
> +	list_add_tail(&req->client_link, &req->file_priv->mm.request_list);
> +}
> +
>  static int
>  execbuf_submit(struct i915_execbuffer_params *params,
>  	       struct drm_i915_gem_execbuffer2 *args,
> @@ -1512,6 +1520,7 @@ execbuf_submit(struct i915_execbuffer_params *params,
>  	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>  
>  	i915_gem_execbuffer_move_to_active(vmas, params->request);
> +	add_to_client(params->request, params->file);
>  
>  	return 0;
>  }
> @@ -1808,10 +1817,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	 */
>  	params->request->batch = params->batch;
>  
> -	ret = i915_gem_request_add_to_client(params->request, file);
> -	if (ret)
> -		goto err_request;
> -
>  	/*
>  	 * Save assorted stuff away to pass through to *_submission().
>  	 * NB: This data should be 'persistent' and not local as it will
> @@ -1825,7 +1830,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	params->ctx                     = ctx;
>  
>  	ret = execbuf_submit(params, args, &eb->vmas);
> -err_request:
>  	__i915_add_request(params->request, ret == 0);
>  
>  err_batch_unpin:
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 1a215320cefb..bf62427a35b7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -115,42 +115,20 @@ const struct fence_ops i915_fence_ops = {
>  	.timeline_value_str = i915_fence_timeline_value_str,
>  };
>  
> -int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> -				   struct drm_file *file)
> -{
> -	struct drm_i915_private *dev_private;
> -	struct drm_i915_file_private *file_priv;
> -
> -	WARN_ON(!req || !file || req->file_priv);
> -
> -	if (!req || !file)
> -		return -EINVAL;
> -
> -	if (req->file_priv)
> -		return -EINVAL;
> -
> -	dev_private = req->i915;
> -	file_priv = file->driver_priv;
> -
> -	spin_lock(&file_priv->mm.lock);
> -	req->file_priv = file_priv;
> -	list_add_tail(&req->client_list, &file_priv->mm.request_list);
> -	spin_unlock(&file_priv->mm.lock);
> -
> -	return 0;
> -}
> -
>  static inline void
>  i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
>  {
> -	struct drm_i915_file_private *file_priv = request->file_priv;
> +	struct drm_i915_file_private *file_priv;
>  
> +	file_priv = request->file_priv;
>  	if (!file_priv)
>  		return;
>  
>  	spin_lock(&file_priv->mm.lock);
> -	list_del(&request->client_list);
> -	request->file_priv = NULL;
> +	if (request->file_priv) {
> +		list_del(&request->client_link);
> +		request->file_priv = NULL;
> +	}
>  	spin_unlock(&file_priv->mm.lock);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index 6c72bd8d9423..9d5a66bfc509 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -132,7 +132,7 @@ struct drm_i915_gem_request {
>  
>  	struct drm_i915_file_private *file_priv;
>  	/** file_priv list entry for this request */
> -	struct list_head client_list;
> +	struct list_head client_link;
>  
>  	/**
>  	 * The ELSP only accepts two elements at a time, so we queue
> @@ -167,8 +167,6 @@ static inline bool fence_is_i915(struct fence *fence)
>  struct drm_i915_gem_request * __must_check
>  i915_gem_request_alloc(struct intel_engine_cs *engine,
>  		       struct i915_gem_context *ctx);
> -int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> -				   struct drm_file *file);
>  void i915_gem_request_retire_upto(struct drm_i915_gem_request *req);
>  
>  static inline u32
> -- 
> 2.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx