Re: [PATCH 51/55] drm/i915: Move the request/file and request/pid association to creation time

[Tomas Elf <tomas.elf@intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx