diff mbox

[2/4] drm/i915: Removed duplicate members from submit_request

Message ID 1419241044-929-3-git-send-email-nicholas.hoath@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nick Hoath Dec. 22, 2014, 9:37 a.m. UTC 
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@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(-)

Comments

Thomas Daniel Dec. 22, 2014, 5:20 p.m. UTC | #1 
> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf

> Of Nick Hoath

> Sent: Monday, December 22, 2014 9:37 AM

> To: intel-gfx@lists.freedesktop.org

> Cc: daniel.vetter@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@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@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

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);
 
 	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;