[PATCH 1/4] drm/i915: Remove unused file arg from execbuf

[PATCH 1/4] drm/i915: Remove unused file arg from execbuf

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 934396c..bc5a5fa 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -475,7 +475,6 @@ i915_gem_execbuffer_unreserve_object(struct drm_i915_gem_object *obj)
 
 static int
 i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring,
-			    struct drm_file *file,
 			    struct list_head *objects,
 			    bool *need_relocs)
 {
@@ -663,7 +662,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 		goto err;
 
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
-	ret = i915_gem_execbuffer_reserve(ring, file, &eb->objects, &need_relocs);
+	ret = i915_gem_execbuffer_reserve(ring, &eb->objects, &need_relocs);
 	if (ret)
 		goto err;
 
@@ -980,7 +979,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
-	ret = i915_gem_execbuffer_reserve(ring, file, &eb->objects, &need_relocs);
+	ret = i915_gem_execbuffer_reserve(ring, &eb->objects, &need_relocs);
 	if (ret)
 		goto err;
 
-- 
1.8.1.5

[PATCH 2/4] drm/i915: Remove unneeded dev argument

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index bc5a5fa..354cecf 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -359,8 +359,7 @@ i915_gem_execbuffer_relocate_object_slow(struct drm_i915_gem_object *obj,
 }
 
 static int
-i915_gem_execbuffer_relocate(struct drm_device *dev,
-			     struct eb_objects *eb)
+i915_gem_execbuffer_relocate(struct eb_objects *eb)
 {
 	struct drm_i915_gem_object *obj;
 	int ret = 0;
@@ -985,7 +984,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	/* The objects are in their final locations, apply the relocations. */
 	if (need_relocs)
-		ret = i915_gem_execbuffer_relocate(dev, eb);
+		ret = i915_gem_execbuffer_relocate(eb);
 	if (ret) {
 		if (ret == -EFAULT) {
 			ret = i915_gem_execbuffer_relocate_slow(dev, args, file, ring,
-- 
1.8.1.5

Re: [PATCH 2/4] drm/i915: Remove unneeded dev argument

[Daniel Vetter]

On Wed, Mar 13, 2013 at 05:21:06PM -0700, Ben Widawsky wrote:
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Merged the first two patches for -next, thanks for the patches. I need to
discuss a bit more with you what exactly you're aiming for - as is with
the two patches and your quick description I fear way too many object
lifetime issues due to lack of refcounts are ahead ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/4] drm/i915: Make eb do more

[Ben Widawsky]

I've written a couple of versions of this patch, and it's always in prep
for some scheduler work I am doing. Only difference this time is a new
maintainer to never merge them.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 38 +++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 354cecf..99ebb36 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -36,10 +36,17 @@
 struct eb_objects {
 	struct list_head objects;
 	int and;
+
+	struct drm_file *file;
+	struct intel_ring_buffer *ring;
+	struct drm_i915_gem_execbuffer2 *args;
+	struct drm_i915_gem_exec_object2 *exec;
+
 	union {
 		struct drm_i915_gem_object *lut[0];
 		struct hlist_head buckets[0];
 	};
+	/* NB: don't put anything here */
 };
 
 static struct eb_objects *
@@ -82,11 +89,11 @@ eb_reset(struct eb_objects *eb)
 }
 
 static int
-eb_lookup_objects(struct eb_objects *eb,
-		  struct drm_i915_gem_exec_object2 *exec,
-		  const struct drm_i915_gem_execbuffer2 *args,
-		  struct drm_file *file)
+eb_lookup_objects(struct eb_objects *eb)
 {
+	struct drm_i915_gem_exec_object2 *exec = eb->exec;
+	struct drm_i915_gem_execbuffer2 *args = eb->args;
+	struct drm_file *file = eb->file;
 	int i;
 
 	spin_lock(&file->table_lock);
@@ -572,18 +579,17 @@ err:		/* Decrement pin count for bound objects */
 }
 
 static int
-i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
-				  struct drm_i915_gem_execbuffer2 *args,
-				  struct drm_file *file,
-				  struct intel_ring_buffer *ring,
-				  struct eb_objects *eb,
-				  struct drm_i915_gem_exec_object2 *exec)
+i915_gem_execbuffer_relocate_slow(struct eb_objects *eb)
 {
 	struct drm_i915_gem_relocation_entry *reloc;
 	struct drm_i915_gem_object *obj;
 	bool need_relocs;
 	int *reloc_offset;
 	int i, total, ret;
+	struct drm_device *dev = eb->ring->dev;
+	struct drm_i915_gem_execbuffer2 *args = eb->args;
+	struct intel_ring_buffer *ring = eb->ring;
+	struct drm_i915_gem_exec_object2 *exec = eb->exec;
 	int count = args->buffer_count;
 
 	/* We may process another execbuffer during the unlock... */
@@ -656,7 +662,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 
 	/* reacquire the objects */
 	eb_reset(eb);
-	ret = eb_lookup_objects(eb, exec, args, file);
+	ret = eb_lookup_objects(eb);
 	if (ret)
 		goto err;
 
@@ -966,8 +972,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		goto pre_mutex_err;
 	}
 
+	eb->args = args;
+	eb->exec = exec;
+	eb->file = file;
+	eb->ring = ring;
+
 	/* Look up object handles */
-	ret = eb_lookup_objects(eb, exec, args, file);
+	ret = eb_lookup_objects(eb);
 	if (ret)
 		goto err;
 
@@ -987,8 +998,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		ret = i915_gem_execbuffer_relocate(eb);
 	if (ret) {
 		if (ret == -EFAULT) {
-			ret = i915_gem_execbuffer_relocate_slow(dev, args, file, ring,
-								eb, exec);
+			ret = i915_gem_execbuffer_relocate_slow(eb);
 			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
 		}
 		if (ret)
-- 
1.8.1.5

Re: [PATCH 3/4] drm/i915: Make eb do more

[Jesse Barnes]

On Wed, 13 Mar 2013 17:21:07 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> I've written a couple of versions of this patch, and it's always in prep
> for some scheduler work I am doing. Only difference this time is a new
> maintainer to never merge them.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---

I think the commit message should reflect what's going on here,
"consolidate execbuf args into a struct" or somesuch.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/4] drm/i915: Make eb do more

[Ben Widawsky]

On Tue, Mar 19, 2013 at 12:41:51PM -0700, Jesse Barnes wrote:
> On Wed, 13 Mar 2013 17:21:07 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > I've written a couple of versions of this patch, and it's always in prep
> > for some scheduler work I am doing. Only difference this time is a new
> > maintainer to never merge them.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> 
> I think the commit message should reflect what's going on here,
> "consolidate execbuf args into a struct" or somesuch.

It was a pretty useless commit message, and I had already updated it
locally:
http://cgit.freedesktop.org/~bwidawsk/drm-intel/commit/?h=execbuf-clean&id=7d0dbbeed1334c49279910c3b6e7d792d28dce3d

As some backup to my gripe, here is the 17 month old version of the
patch:
http://cgit.freedesktop.org/~bwidawsk/drm-intel/commit/?h=eb_clean&id=04cf2b166f3e31f271c2dedf1791ccd8e0f922a0

> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 4/4] drm/i915: Extract object reserver/reloc/bind

[Ben Widawsky]

These operations are all quite similar and moving them to a separate
function provides a clean split if we want to do something other than
immediately binding the work into the ring.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 77 ++++++++++++++++--------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 99ebb36..35f6bf3 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -827,6 +827,47 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	return 0;
 }
 
+/* Move the objects en-masse into the GTT, evicting if necessary. */
+static bool
+prepare_objects(struct eb_objects *eb, struct drm_i915_gem_object *batch_obj,
+		u32 flags)
+{
+	bool need_relocs = (eb->args->flags & I915_EXEC_NO_RELOC) == 0;
+	int ret;
+
+	ret = i915_gem_execbuffer_reserve(eb->ring, &eb->objects, &need_relocs);
+	if (ret)
+		return ret;
+
+	/* The objects are in their final locations, apply the relocations. */
+	if (need_relocs)
+		ret = i915_gem_execbuffer_relocate(eb);
+	if (ret) {
+		if (ret == -EFAULT) {
+			ret = i915_gem_execbuffer_relocate_slow(eb);
+			BUG_ON(!mutex_is_locked(&eb->ring->dev->struct_mutex));
+		}
+		if (ret)
+			return ret;
+	}
+
+	/* Set the pending read domains for the batch buffer to COMMAND */
+	if (batch_obj->base.pending_write_domain) {
+		DRM_DEBUG("Attempting to use self-modifying batch buffer\n");
+		return -EINVAL;
+	}
+	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
+
+	/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
+	 * batch" bit. Hence we need to pin secure batches into the global gtt.
+	 * hsw should have this fixed, but let's be paranoid and do it
+	 * unconditionally for now. */
+	if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
+		i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
+
+	return i915_gem_execbuffer_move_to_gpu(eb->ring, &eb->objects);
+}
+
 static int
 i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_file *file,
@@ -842,7 +883,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	u32 exec_start, exec_len;
 	u32 mask, flags;
 	int ret, mode, i;
-	bool need_relocs;
 
 	if (!i915_gem_check_execbuffer(args))
 		return -EINVAL;
@@ -987,40 +1027,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			       struct drm_i915_gem_object,
 			       exec_list);
 
-	/* Move the objects en-masse into the GTT, evicting if necessary. */
-	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
-	ret = i915_gem_execbuffer_reserve(ring, &eb->objects, &need_relocs);
-	if (ret)
-		goto err;
-
-	/* The objects are in their final locations, apply the relocations. */
-	if (need_relocs)
-		ret = i915_gem_execbuffer_relocate(eb);
-	if (ret) {
-		if (ret == -EFAULT) {
-			ret = i915_gem_execbuffer_relocate_slow(eb);
-			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
-		}
-		if (ret)
-			goto err;
-	}
-
-	/* Set the pending read domains for the batch buffer to COMMAND */
-	if (batch_obj->base.pending_write_domain) {
-		DRM_DEBUG("Attempting to use self-modifying batch buffer\n");
-		ret = -EINVAL;
-		goto err;
-	}
-	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
-
-	/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
-	 * batch" bit. Hence we need to pin secure batches into the global gtt.
-	 * hsw should have this fixed, but let's be paranoid and do it
-	 * unconditionally for now. */
-	if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
-		i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
-
-	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->objects);
+	ret = prepare_objects(eb, batch_obj, flags);
 	if (ret)
 		goto err;
 
-- 
1.8.1.5

Re: [PATCH 1/4] drm/i915: Remove unused file arg from execbuf

[Chris Wilson]

I was hoping for something a little more magical. I couldn't spot any
mistakes - the only thing missing is a hint as to why these are useful.

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/4] drm/i915: Remove unused file arg from execbuf

[Ben Widawsky]

On Thu, Mar 14, 2013 at 08:56:41AM +0000, Chris Wilson wrote:
> I was hoping for something a little more magical. I couldn't spot any
> mistakes - the only thing missing is a hint as to why these are useful.
> 
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

I'm pretty sure you asked for magic the last time sent this patch.
Anyway, I can probably talk a little bit without disclosing the secretz.

Primarily the goal is to be able to easily queue up the work. Packing
more into the eb struct gives a nice compact /thing/ which I can put
into a list for later execution.

Now, the deferred execution has it's own pitfalls, and I'd prefer not to
get too far into that since I think these cleanups are decent on their
own.

> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center