| Message ID | 1424366285-29232-54-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/02/2015 17:18, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > 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> > --- > drivers/gpu/drm/i915/i915_drv.h | 8 +++-- > drivers/gpu/drm/i915/i915_gem.c | 49 +++++++++++++++++++--------- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 ++-- > 3 files changed, 44 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 3955bef..4a9248f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2199,6 +2199,9 @@ struct drm_i915_gem_request { > }; > > void i915_gem_request_free(struct kref *req_ref); > +void i915_gem_request_remove_from_client(struct drm_i915_gem_request *request); > +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) > @@ -2831,13 +2834,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); > int __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 8e7418b..660518d 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2401,7 +2401,6 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno) > } > > int __i915_add_request(struct drm_i915_gem_request *request, > - struct drm_file *file, > struct drm_i915_gem_object *obj, > bool flush_caches) > { > @@ -2463,19 +2462,6 @@ int __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); > > @@ -2490,7 +2476,34 @@ int __i915_add_request(struct drm_i915_gem_request *request, > return 0; > } > > -static inline void > +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; > +} > + > +inline void > i915_gem_request_remove_from_client(struct drm_i915_gem_request *request) > { > struct drm_i915_file_private *file_priv = request->file_priv; > @@ -2565,6 +2578,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; > @@ -4120,6 +4136,9 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file) > if (time_after_eq(request->emitted_jiffies, recent_enough)) > break; > > + if (!request->emitted_jiffies) > + continue; > + Please add a comment clarifying the purpose of the emitted_jiffies check above. Thanks, Tomas > 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 0eae592..739aaeb 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -996,8 +996,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 */ > - return __i915_add_request(params->request, params->file, > - params->batch_obj, true); > + return __i915_add_request(params->request, params->batch_obj, true); > } > > static int > @@ -1537,6 +1536,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > if (ret) > goto err; > > + ret = i915_gem_request_add_to_client(params->request, file); > + if (ret) > + goto err; > + > /* > * Save assorted stuff away to pass through to *_submission(). > * NB: This data should be 'persistent' and not local as it will >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3955bef..4a9248f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2199,6 +2199,9 @@ struct drm_i915_gem_request { }; void i915_gem_request_free(struct kref *req_ref); +void i915_gem_request_remove_from_client(struct drm_i915_gem_request *request); +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) @@ -2831,13 +2834,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); int __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 8e7418b..660518d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2401,7 +2401,6 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno) } int __i915_add_request(struct drm_i915_gem_request *request, - struct drm_file *file, struct drm_i915_gem_object *obj, bool flush_caches) { @@ -2463,19 +2462,6 @@ int __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); @@ -2490,7 +2476,34 @@ int __i915_add_request(struct drm_i915_gem_request *request, return 0; } -static inline void +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; +} + +inline void i915_gem_request_remove_from_client(struct drm_i915_gem_request *request) { struct drm_i915_file_private *file_priv = request->file_priv; @@ -2565,6 +2578,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; @@ -4120,6 +4136,9 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file) if (time_after_eq(request->emitted_jiffies, recent_enough)) break; + 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 0eae592..739aaeb 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -996,8 +996,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 */ - return __i915_add_request(params->request, params->file, - params->batch_obj, true); + return __i915_add_request(params->request, params->batch_obj, true); } static int @@ -1537,6 +1536,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, if (ret) goto err; + ret = i915_gem_request_add_to_client(params->request, file); + if (ret) + goto err; + /* * Save assorted stuff away to pass through to *_submission(). * NB: This data should be 'persistent' and not local as it will