Re: [PATCH 08/33] drm/i915: Move setting of request->batch into its single callsite

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> request->batch_obj is only set by execbuffer for the convenience of
> debugging hangs. By moving that operation to the callsite, we can
> simplify all other callers and future patches. We also move the
> complications of reference handling of the request->batch_obj next to
> where the active tracking is set up for the request.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++++++-
>  drivers/gpu/drm/i915/i915_gem_request.c    | 12 +-----------
>  drivers/gpu/drm/i915/i915_gem_request.h    |  8 +++-----
>  3 files changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index c494b79ded20..c8d13fea4b25 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1702,6 +1702,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		goto err_batch_unpin;
>  	}
>  
> +	/* Whilst this request exists, batch_obj will be on the
> +	 * active_list, and so will hold the active reference. Only when this
> +	 * request is retired will the the batch_obj be moved onto the
> +	 * inactive_list and lose its active reference. Hence we do not need
> +	 * to explicitly hold another reference here.
> +	 */

The comment here might or might not need revisiting. I can't say yet.

But when I tried to learn how the current code works, I found
that there are comments referencing __i915_gem_active_get_request_rcu()
which does not exist.

-Mika

> +	params->request->batch_obj = params->batch->obj;
> +
>  	ret = i915_gem_request_add_to_client(params->request, file);
>  	if (ret)
>  		goto err_request;
> @@ -1720,7 +1728,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  
>  	ret = execbuf_submit(params, args, &eb->vmas);
>  err_request:
> -	__i915_add_request(params->request, params->batch->obj, ret == 0);
> +	__i915_add_request(params->request, ret == 0);
>  
>  err_batch_unpin:
>  	/*
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index b7ffde002a62..c6f523e2879c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -461,9 +461,7 @@ static void i915_gem_mark_busy(const struct intel_engine_cs *engine)
>   * request is not being tracked for completion but the work itself is
>   * 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_i915_gem_object *obj,
> -			bool flush_caches)
> +void __i915_add_request(struct drm_i915_gem_request *request, bool flush_caches)
>  {
>  	struct intel_engine_cs *engine;
>  	struct intel_ring *ring;
> @@ -504,14 +502,6 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>  
>  	request->head = request_start;
>  
> -	/* Whilst this request exists, batch_obj will be on the
> -	 * active_list, and so will hold the active reference. Only when this
> -	 * request is retired will the the batch_obj be moved onto the
> -	 * inactive_list and lose its active reference. Hence we do not need
> -	 * to explicitly hold another reference here.
> -	 */
> -	request->batch_obj = obj;
> -
>  	/* Seal the request and mark it as pending execution. Note that
>  	 * we may inspect this state, without holding any locks, during
>  	 * hangcheck. Hence we apply the barrier to ensure that we do not
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index 721eb8cbce9b..d5176f9cc22f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -225,13 +225,11 @@ static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
>  	*pdst = src;
>  }
>  
> -void __i915_add_request(struct drm_i915_gem_request *req,
> -			struct drm_i915_gem_object *batch_obj,
> -			bool flush_caches);
> +void __i915_add_request(struct drm_i915_gem_request *req, bool flush_caches);
>  #define i915_add_request(req) \
> -	__i915_add_request(req, NULL, true)
> +	__i915_add_request(req, true)
>  #define i915_add_request_no_flush(req) \
> -	__i915_add_request(req, NULL, false)
> +	__i915_add_request(req, false)
>  
>  struct intel_rps_client;
>  #define NO_WAITBOOST ERR_PTR(-1)
> -- 
> 2.8.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx