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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 02, 2016 at 11:30:18AM +0100, John Harrison wrote:
> On 22/08/2016 09:03, Chris Wilson wrote:
> >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@xxxxxxxxxxxxxxxxxx>
> >---
> >  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) {
> Why check for request->file_priv again? The block above will exit if
> it is null. There surely can't be a race with remove_from_client
> being called concurrently with add_to_client? Especially as
> add_to_client no longer takes the spin_lock anyway.

We can however allow i915_gem_release() to be called concurrently. It
doesn't require struct_mutex.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux