Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
