Message ID | 1432917856-12261-52-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 29/05/2015 17:44, 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 | 7 ++-- > drivers/gpu/drm/i915/i915_gem.c | 56 ++++++++++++++++++++-------- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++- > 3 files changed, 49 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index f9b6517..18bfc84 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2199,6 +2199,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) > @@ -2864,13 +2866,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 5aa0ad0..b8fe931 100644 > --- 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; > @@ -4320,6 +4337,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 e868ac1..52139c6 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1058,7 +1058,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 > @@ -1612,6 +1612,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 > Reviewed-by: Tomas Elf <tomas.elf@intel.com> Thanks, Tomas
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f9b6517..18bfc84 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2199,6 +2199,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) @@ -2864,13 +2866,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 5aa0ad0..b8fe931 100644 --- 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; @@ -4320,6 +4337,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 e868ac1..52139c6 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1058,7 +1058,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 @@ -1612,6 +1612,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