Message ID | 1426768264-16996-55-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 19/03/2015 12:30, 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 | 53 ++++++++++++++++++++-------- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 +++- > 3 files changed, 48 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 839c185..764ee4f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2127,6 +2127,9 @@ 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); > +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) > @@ -2752,13 +2755,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 9ff9bda..ecdae34 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2324,7 +2324,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) > { > @@ -2392,19 +2391,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); > > @@ -2420,7 +2406,34 @@ void __i915_add_request(struct drm_i915_gem_request *request, > intel_ring_reserved_space_end(ringbuf); > } > > -static inline void > +int i915_gem_request_add_to_client(struct drm_i915_gem_request *req, > + struct drm_file *file) Considering that add_to_client is only used from i915_gem_execbuffer.c we could move the definition there and make it static. On the other hand it makes sense from a semantic point of view to leave it in i915_gem.c and make the function public. Even though it is only called from i915_gem_execbuffer and from nowhere else. Semantics vs. scope. I could be convinced of either virtue. Any other opinions on this for either case? > +{ > + 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) i915_gem_request_remove_from_client used to be static and it should remain static considering that it's only called from within i915_gem.c . Thanks, Tomas > { > struct drm_i915_file_private *file_priv = request->file_priv; > @@ -2495,6 +2508,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,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 c512979..8efc7f8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1060,7 +1060,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 > @@ -1603,6 +1603,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 >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 839c185..764ee4f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2127,6 +2127,9 @@ 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); +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) @@ -2752,13 +2755,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 9ff9bda..ecdae34 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2324,7 +2324,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) { @@ -2392,19 +2391,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); @@ -2420,7 +2406,34 @@ void __i915_add_request(struct drm_i915_gem_request *request, intel_ring_reserved_space_end(ringbuf); } -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; @@ -2495,6 +2508,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,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 c512979..8efc7f8 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1060,7 +1060,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 @@ -1603,6 +1603,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