diff mbox

[v7,5/5] drm/i915: Tidy up execbuffer command parsing code

Message ID 1418328792-28072-6-git-send-email-michael.h.nguyen@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michael H. Nguyen Dec. 11, 2014, 8:13 p.m. UTC
From: Brad Volkin <bradley.d.volkin@intel.com>

Move it to a separate function since the main do_execbuffer function
already has so much going on.

v2:
- Move pin/unpin calls inside i915_parse_cmds() (Chris W, v4 7/7
  feedback)

Issue: VIZ-4719
Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c     |   8 ++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 124 ++++++++++++++++-------------
 2 files changed, 77 insertions(+), 55 deletions(-)

Comments

Bloomfield, Jon Dec. 12, 2014, 9:26 a.m. UTC | #1
> -----Original Message-----
> From: Nguyen, Michael H
> Sent: Thursday, December 11, 2014 8:13 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Bloomfield, Jon; Brad Volkin
> Subject: [PATCH v7 5/5] drm/i915: Tidy up execbuffer command parsing code
> 
> From: Brad Volkin <bradley.d.volkin@intel.com>
> 
> Move it to a separate function since the main do_execbuffer function
> already has so much going on.
> 
> v2:
> - Move pin/unpin calls inside i915_parse_cmds() (Chris W, v4 7/7
>   feedback)
> 
> Issue: VIZ-4719
> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>

Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>?
Shuang He Dec. 13, 2014, 12:23 p.m. UTC | #2
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1-3              364/366              362/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_kms_flip_flip-vs-panning      DMESG_WARN(1, M26)PASS(8, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_plain-flip-fb-recreate-interruptible      DMESG_WARN(1, M26)PASS(5, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-panning      PASS(5, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(8, M26)PASS(26, M26M37)      PASS(1, M26)
Note: You need to pay more attention to line start with '*'
Daniel Vetter Dec. 15, 2014, 2:55 p.m. UTC | #3
On Fri, Dec 12, 2014 at 09:26:23AM +0000, Bloomfield, Jon wrote:
> > -----Original Message-----
> > From: Nguyen, Michael H
> > Sent: Thursday, December 11, 2014 8:13 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Bloomfield, Jon; Brad Volkin
> > Subject: [PATCH v7 5/5] drm/i915: Tidy up execbuffer command parsing code
> > 
> > From: Brad Volkin <bradley.d.volkin@intel.com>
> > 
> > Move it to a separate function since the main do_execbuffer function
> > already has so much going on.
> > 
> > v2:
> > - Move pin/unpin calls inside i915_parse_cmds() (Chris W, v4 7/7
> >   feedback)
> > 
> > Issue: VIZ-4719
> > Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
> 
> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>?

Vacuumed up entire series, thanks for patches&review.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index a698b47..0d757cd 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1050,10 +1050,17 @@  int i915_parse_cmds(struct intel_engine_cs *ring,
 	struct drm_i915_cmd_descriptor default_desc = { 0 };
 	bool oacontrol_set = false; /* OACONTROL tracking. See check_cmd() */
 
+	ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
+	if (ret) {
+		DRM_DEBUG_DRIVER("CMD: Failed to pin shadow batch\n");
+		return -1;
+	}
+
 	batch_base = copy_batch(shadow_batch_obj, batch_obj,
 				batch_start_offset, batch_len);
 	if (IS_ERR(batch_base)) {
 		DRM_DEBUG_DRIVER("CMD: Failed to copy batch\n");
+		i915_gem_object_ggtt_unpin(shadow_batch_obj);
 		return PTR_ERR(batch_base);
 	}
 
@@ -1124,6 +1131,7 @@  int i915_parse_cmds(struct intel_engine_cs *ring,
 	}
 
 	vunmap(batch_base);
+	i915_gem_object_ggtt_unpin(shadow_batch_obj);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 9af4053..3d36465 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1069,6 +1069,65 @@  i915_emit_box(struct intel_engine_cs *ring,
 	return 0;
 }
 
+static struct drm_i915_gem_object*
+i915_gem_execbuffer_parse(struct intel_engine_cs *ring,
+			  struct drm_i915_gem_exec_object2 *shadow_exec_entry,
+			  struct eb_vmas *eb,
+			  struct drm_i915_gem_object *batch_obj,
+			  u32 batch_start_offset,
+			  u32 batch_len,
+			  bool is_master,
+			  u32 *flags)
+{
+	struct drm_i915_private *dev_priv = to_i915(batch_obj->base.dev);
+	struct drm_i915_gem_object *shadow_batch_obj;
+	int ret;
+
+	shadow_batch_obj = i915_gem_batch_pool_get(&dev_priv->mm.batch_pool,
+						   batch_obj->base.size);
+	if (IS_ERR(shadow_batch_obj))
+		return shadow_batch_obj;
+
+	ret = i915_parse_cmds(ring,
+			      batch_obj,
+			      shadow_batch_obj,
+			      batch_start_offset,
+			      batch_len,
+			      is_master);
+	if (ret) {
+		if (ret == -EACCES)
+			return batch_obj;
+	} else {
+		struct i915_vma *vma;
+
+		memset(shadow_exec_entry, 0, sizeof(*shadow_exec_entry));
+
+		vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
+		vma->exec_entry = shadow_exec_entry;
+		vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE;
+		drm_gem_object_reference(&shadow_batch_obj->base);
+		list_add_tail(&vma->exec_list, &eb->vmas);
+
+		shadow_batch_obj->base.pending_read_domains =
+			batch_obj->base.pending_read_domains;
+
+		/*
+		 * Set the DISPATCH_SECURE bit to remove the NON_SECURE
+		 * bit from MI_BATCH_BUFFER_START commands issued in the
+		 * dispatch_execbuffer implementations. We specifically
+		 * don't want that set when the command parser is
+		 * enabled.
+		 *
+		 * FIXME: with aliasing ppgtt, buffers that should only
+		 * be in ggtt still end up in the aliasing ppgtt. remove
+		 * this check when that is fixed.
+		 */
+		if (USES_FULL_PPGTT(dev))
+			*flags |= I915_DISPATCH_SECURE;
+	}
+
+	return ret ? ERR_PTR(ret) : shadow_batch_obj;
+}
 
 int
 i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
@@ -1286,7 +1345,6 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct eb_vmas *eb;
 	struct drm_i915_gem_object *batch_obj;
-	struct drm_i915_gem_object *shadow_batch_obj = NULL;
 	struct drm_i915_gem_exec_object2 shadow_exec_entry;
 	struct intel_engine_cs *ring;
 	struct intel_context *ctx;
@@ -1406,62 +1464,18 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 
 	if (i915_needs_cmd_parser(ring)) {
-		shadow_batch_obj =
-			i915_gem_batch_pool_get(&dev_priv->mm.batch_pool,
-						batch_obj->base.size);
-		if (IS_ERR(shadow_batch_obj)) {
-			ret = PTR_ERR(shadow_batch_obj);
-			/* Don't try to clean up the obj in the error path */
-			shadow_batch_obj = NULL;
+		batch_obj = i915_gem_execbuffer_parse(ring,
+						      &shadow_exec_entry,
+						      eb,
+						      batch_obj,
+						      args->batch_start_offset,
+						      args->batch_len,
+						      file->is_master,
+						      &flags);
+		if (IS_ERR(batch_obj)) {
+			ret = PTR_ERR(batch_obj);
 			goto err;
 		}
-
-		ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
-		if (ret)
-			goto err;
-
-		ret = i915_parse_cmds(ring,
-				      batch_obj,
-				      shadow_batch_obj,
-				      args->batch_start_offset,
-				      args->batch_len,
-				      file->is_master);
-		i915_gem_object_ggtt_unpin(shadow_batch_obj);
-
-		if (ret) {
-			if (ret != -EACCES)
-				goto err;
-		} else {
-			struct i915_vma *vma;
-
-			memset(&shadow_exec_entry, 0,
-			       sizeof(shadow_exec_entry));
-
-			vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
-			vma->exec_entry = &shadow_exec_entry;
-			vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE;
-			drm_gem_object_reference(&shadow_batch_obj->base);
-			list_add_tail(&vma->exec_list, &eb->vmas);
-
-			shadow_batch_obj->base.pending_read_domains =
-				batch_obj->base.pending_read_domains;
-
-			batch_obj = shadow_batch_obj;
-
-			/*
-			 * Set the DISPATCH_SECURE bit to remove the NON_SECURE
-			 * bit from MI_BATCH_BUFFER_START commands issued in the
-			 * dispatch_execbuffer implementations. We specifically
-			 * don't want that set when the command parser is
-			 * enabled.
-			 *
-			 * FIXME: with aliasing ppgtt, buffers that should only
-			 * be in ggtt still end up in the aliasing ppgtt. remove
-			 * this check when that is fixed.
-			 */
-			if (USES_FULL_PPGTT(dev))
-				flags |= I915_DISPATCH_SECURE;
-		}
 	}
 
 	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;