[Intel-gfx] [PATCH 2/4] drm/i915: Removed duplicate members from submit_request
Daniel, Thomas
thomas.daniel at intel.com
Mon Dec 22 09:20:01 PST 2014
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf
> Of Nick Hoath
> Sent: Monday, December 22, 2014 9:37 AM
> To: intel-gfx at lists.freedesktop.org
> Cc: daniel.vetter at ffwll.ch
> Subject: [Intel-gfx] [PATCH 2/4] drm/i915: Removed duplicate members from
> submit_request
>
> Where there were duplicate variables for the tail, context and ring (engine)
> in the gem request and the execlist queue item, use the one from the
> request and remove the duplicate from the execlist queue item.
>
> Issue: VIZ-4274
>
> v1: Rebase
> Signed-off-by: Nick Hoath <nicholas.hoath at intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/intel_lrc.c | 20 +++++++++-----------
> drivers/gpu/drm/i915/intel_lrc.h | 4 ----
> 4 files changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index e515aad..d4cc482 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1968,11 +1968,11 @@ static int i915_execlists(struct seq_file *m, void
> *data)
> if (head_req) {
> struct drm_i915_gem_object *ctx_obj;
>
> - ctx_obj = head_req->ctx->engine[ring_id].state;
> + ctx_obj = head_req->request->ctx-
> >engine[ring_id].state;
> seq_printf(m, "\tHead request id: %u\n",
> intel_execlists_ctx_id(ctx_obj));
> seq_printf(m, "\tHead request tail: %u\n",
> - head_req->tail);
> + head_req->request->tail);
> }
>
> seq_putc(m, '\n');
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 2b6ecfd..8782a4d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2669,7 +2669,7 @@ static void i915_gem_reset_ring_cleanup(struct
> drm_i915_private *dev_priv,
> execlist_link);
> list_del(&submit_req->execlist_link);
> intel_runtime_pm_put(dev_priv);
> - i915_gem_context_unreference(submit_req->ctx);
> + i915_gem_context_unreference(submit_req->request-
> >ctx);
> kfree(submit_req);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index d9eaf2d..a18ea13 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -417,7 +417,7 @@ static void execlists_context_unqueue(struct
> intel_engine_cs *ring)
> execlist_link) {
> if (!req0) {
> req0 = cursor;
> - } else if (req0->ctx == cursor->ctx) {
> + } else if (req0->request->ctx == cursor->request->ctx) {
> /* Same ctx: ignore first request, as second request
> * will update tail past first request's workload */
> cursor->elsp_submitted = req0->elsp_submitted;
> @@ -433,9 +433,9 @@ static void execlists_context_unqueue(struct
> intel_engine_cs *ring)
>
> WARN_ON(req1 && req1->elsp_submitted);
>
> - execlists_submit_contexts(ring, req0->ctx, req0->tail,
> - req1 ? req1->ctx : NULL,
> - req1 ? req1->tail : 0);
> + execlists_submit_contexts(ring, req0->request->ctx, req0->request-
> >tail,
> + req1 ? req1->request->ctx : NULL,
> + req1 ? req1->request->tail : 0);
This substitution is wrong since the two tail values are not equal. Look at __i915_add_request() - the value to be assigned to drm_request.tail is sampled before the call to emit_request() or add_request() so doesn't include the commands written in those functions. Furthermore the assignment is done after the call to emit_request() so is not guaranteed to be available when execlists_context_unqueue() executes (e.g. when the execlist queue is empty).
Thomas.
>
> req0->elsp_submitted++;
> if (req1)
> @@ -455,7 +455,7 @@ static bool execlists_check_remove_request(struct
> intel_engine_cs *ring,
>
> if (head_req != NULL) {
> struct drm_i915_gem_object *ctx_obj =
> - head_req->ctx->engine[ring->id].state;
> + head_req->request->ctx->engine[ring-
> >id].state;
> if (intel_execlists_ctx_id(ctx_obj) == request_id) {
> WARN(head_req->elsp_submitted == 0,
> "Never submitted head request\n"); @@ -551,9
> +551,6 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
> if (to != ring->default_context)
> intel_lr_context_pin(ring, to);
>
> - req->ring = ring;
> - req->tail = tail;
> -
> if (!request) {
> /*
> * If there isn't a request associated with this submission,
> @@ -568,6 +565,7 @@ static int execlists_context_queue(struct
> intel_engine_cs *ring,
> }
> req->request = request;
> i915_gem_request_reference(request);
> + i915_gem_context_reference(req->request->ctx);
>
> intel_runtime_pm_get(dev_priv);
>
> @@ -584,7 +582,7 @@ static int execlists_context_queue(struct
> intel_engine_cs *ring,
> struct intel_ctx_submit_request,
> execlist_link);
>
> - if (to == tail_req->ctx) {
> + if (to == tail_req->request->ctx) {
> WARN(tail_req->elsp_submitted != 0,
> "More than 2 already-submitted reqs
> queued\n");
> list_del(&tail_req->execlist_link);
> @@ -774,14 +772,14 @@ void intel_execlists_retire_requests(struct
> intel_engine_cs *ring)
> spin_unlock_irqrestore(&ring->execlist_lock, flags);
>
> list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
> - struct intel_context *ctx = req->ctx;
> + struct intel_context *ctx = req->request->ctx;
> struct drm_i915_gem_object *ctx_obj =
> ctx->engine[ring->id].state;
>
> if (ctx_obj && (ctx != ring->default_context))
> intel_lr_context_unpin(ring, ctx);
> intel_runtime_pm_put(dev_priv);
> - i915_gem_context_unreference(req->ctx);
> + i915_gem_context_unreference(ctx);
> i915_gem_request_unreference(req->request);
> list_del(&req->execlist_link);
> kfree(req);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h
> b/drivers/gpu/drm/i915/intel_lrc.h
> index 7a82bc9..376c307 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -105,10 +105,6 @@ u32 intel_execlists_ctx_id(struct
> drm_i915_gem_object *ctx_obj);
> * All accesses to the queue are mediated by a spinlock (ring->execlist_lock).
> */
> struct intel_ctx_submit_request {
> - struct intel_context *ctx;
> - struct intel_engine_cs *ring;
> - u32 tail;
> -
> struct list_head execlist_link;
>
> int elsp_submitted;
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list