drm/i915: Move the request/file and request/pid association to creation time

In _i915_add_request(), the request is associated with a userland client.
Specifically it is linked to the 'file' structure and the current user process
is recorded. One problem here is that the current user process is not
necessarily the same as when the request was submitted to the driver. This is
especially true when the GPU scheduler arrives and decouples driver submission
from hardware submission. Note also that it is only in the case where the add
request comes from an execbuff call that there is a client to associate. Any
other add request call is kernel only so does not need to do it.

This patch moves the client association into a separate function. This is then
called from the execbuffer code path itself at a sensible time. It also removes
the now redundant 'file' pointer from the add request parameter list.

An extra cleanup of the client association is also added to the request clean up
code for the eventuality where the request is killed after association but
before being submitted (e.g. due to out of memory error somewhere). Once the
submission has happened, the request is on the request list and the regular
request list removal will clear the association. Note that this still needs to
happen at this point in time because the request might be kept floating around
much longer (due to someone holding a reference count) and the client should not
be worrying about this request after it has been retired.

For: VIZ-5115
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Reviewed-by: Tomas Elf <tomas.elf@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5aea6ddd509153ad003042f859990ebd2f570631..7d7339c8ec9633ab0780e395a0988566b920ce4b 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2212,6 +2212,8 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
                           struct drm_i915_gem_request **req_out);
 void i915_gem_request_cancel(struct drm_i915_gem_request *req);
 void i915_gem_request_free(struct kref *req_ref);
+int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
+                                  struct drm_file *file);
 
 static inline uint32_t
 i915_gem_request_get_seqno(struct drm_i915_gem_request *req)
@@ -2891,13 +2893,12 @@ void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_suspend(struct drm_device *dev);
 void __i915_add_request(struct drm_i915_gem_request *req,
-                       struct drm_file *file,
                        struct drm_i915_gem_object *batch_obj,
                        bool flush_caches);
 #define i915_add_request(req) \
-       __i915_add_request(req, NULL, NULL, true)
+       __i915_add_request(req, NULL, true)
 #define i915_add_request_no_flush(req) \
-       __i915_add_request(req, NULL, NULL, false)
+       __i915_add_request(req, NULL, false)
 int __i915_wait_request(struct drm_i915_gem_request *req,
                        unsigned reset_counter,
                        bool interruptible,

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d1193dcb87291f074a249d371cce58288d4f233b..10832c05e96d81524e00bdac7d9fa9df1dcfa7c9 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1331,6 +1331,33 @@ out:
        return ret;
 }
 
+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->ring->dev->dev_private;
+       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);
+
+       req->pid = get_pid(task_pid(current));
+
+       return 0;
+}
+
 static inline void
 i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
 {
@@ -1343,6 +1370,9 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
        list_del(&request->client_list);
        request->file_priv = NULL;
        spin_unlock(&file_priv->mm.lock);
+
+       put_pid(request->pid);
+       request->pid = NULL;
 }
 
 static void i915_gem_request_retire(struct drm_i915_gem_request *request)
@@ -1362,8 +1392,6 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
        list_del_init(&request->list);
        i915_gem_request_remove_from_client(request);
 
-       put_pid(request->pid);
-
        i915_gem_request_unreference(request);
 }
 
@@ -2468,7 +2496,6 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
  * going to happen on the hardware. This would be a Bad Thing(tm).
  */
 void __i915_add_request(struct drm_i915_gem_request *request,
-                       struct drm_file *file,
                        struct drm_i915_gem_object *obj,
                        bool flush_caches)
 {
@@ -2538,19 +2565,6 @@ void __i915_add_request(struct drm_i915_gem_request *request,
 
        request->emitted_jiffies = jiffies;
        list_add_tail(&request->list, &ring->request_list);
-       request->file_priv = NULL;
-
-       if (file) {
-               struct drm_i915_file_private *file_priv = file->driver_priv;
-
-               spin_lock(&file_priv->mm.lock);
-               request->file_priv = file_priv;
-               list_add_tail(&request->client_list,
-                             &file_priv->mm.request_list);
-               spin_unlock(&file_priv->mm.lock);
-
-               request->pid = get_pid(task_pid(current));
-       }
 
        trace_i915_gem_request_add(request);
 
@@ -2616,6 +2630,9 @@ void i915_gem_request_free(struct kref *req_ref)
                                                 typeof(*req), ref);
        struct intel_context *ctx = req->ctx;
 
+       if (req->file_priv)
+               i915_gem_request_remove_from_client(req);
+
        if (ctx) {
                if (i915.enable_execlists) {
                        struct intel_engine_cs *ring = req->ring;
@@ -4314,6 +4331,13 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
                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;
+
                target = request;
        }
        reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3aa2358c2b93593854294c5f96d50d22d642cd84..600db7441847bc0b6d8cd8d289b5b5e1c2f30784 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1066,7 +1066,7 @@ i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
        params->ring->gpu_caches_dirty = true;
 
        /* Add a breadcrumb for the completion of the batch buffer */
-       __i915_add_request(params->request, params->file, params->batch_obj, true);
+       __i915_add_request(params->request, params->batch_obj, true);
 }
 
 static int
@@ -1620,6 +1620,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
        if (ret)
                goto err_batch_unpin;
 
+       ret = i915_gem_request_add_to_client(params->request, file);
+       if (ret)
+               goto err_batch_unpin;
+
        /*
         * Save assorted stuff away to pass through to *_submission().
         * NB: This data should be 'persistent' and not local as it will