From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/4] drm/i915: Use eb for more stuff
Date: Thu, 13 Oct 2011 10:58:25 +0200 [thread overview]
Message-ID: <20111013085825.GF2938@phenom.ffwll.local> (raw)
In-Reply-To: <1318460182-21302-5-git-send-email-ben@bwidawsk.net>
On Wed, Oct 12, 2011 at 03:56:22PM -0700, Ben Widawsky wrote:
> This refactor is useful for some future work I'll be doing on the
> execbuffer path. In addition to being a pretty easy prerequisite, it
> also helped me track down the bug uncovered in the first patch.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 102 ++++++++++++++--------------
> 1 files changed, 51 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 182a2b9..b3beaae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -40,6 +40,16 @@ struct change_domains {
> uint32_t flips;
> };
>
> +struct eb_objects {
> + struct drm_i915_gem_object *batch_obj;
> + struct list_head objects;
> + int buffer_count;
> + int mode;
> + int and;
While you whack this code, can you do an s/and/end, I think that's just a
typo ...
> + struct hlist_head buckets[0];
> + /* DO NOT PUT ANYTHING HERE */
When you drop the array size, the compiler will enforce that for you (for
$compiler = gcc at least).
> +};
> +
> /*
> * Set the next domain for the specified object. This
> * may not actually perform the necessary flushing/invaliding though,
> @@ -207,11 +217,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_i915_gem_object *obj,
> cd->flush_rings |= ring->id;
> }
>
> -struct eb_objects {
> - int and;
> - struct hlist_head buckets[0];
> -};
> -
> static struct eb_objects *
> eb_create(int size)
> {
> @@ -225,6 +230,8 @@ eb_create(int size)
> if (eb == NULL)
> return eb;
>
> + INIT_LIST_HEAD(&eb->objects);
> +
> eb->and = count - 1;
> return eb;
> }
> @@ -232,12 +239,22 @@ eb_create(int size)
> static void
> eb_reset(struct eb_objects *eb)
> {
> + while (!list_empty(&eb->objects)) {
list_for_each_entry_safe
> + struct drm_i915_gem_object *obj;
> +
> + obj = list_first_entry(&eb->objects,
> + struct drm_i915_gem_object,
> + exec_list);
> + list_del_init(&obj->exec_list);
> + drm_gem_object_unreference(&obj->base);
> + }
> memset(eb->buckets, 0, (eb->and+1)*sizeof(struct hlist_head));
> }
>
> static void
> eb_add_object(struct eb_objects *eb, struct drm_i915_gem_object *obj)
> {
> + list_add_tail(&obj->exec_list, &eb->objects);
> hlist_add_head(&obj->exec_node,
> &eb->buckets[obj->exec_handle & eb->and]);
> }
> @@ -262,6 +279,16 @@ eb_get_object(struct eb_objects *eb, unsigned long handle)
> static void
> eb_destroy(struct eb_objects *eb)
> {
> + while (!list_empty(&eb->objects)) {
dito
> + struct drm_i915_gem_object *obj;
> +
> + obj = list_first_entry(&eb->objects,
> + struct drm_i915_gem_object,
> + exec_list);
> + list_del_init(&obj->exec_list);
> + drm_gem_object_unreference(&obj->base);
> + }
> +
> kfree(eb);
> }
>
> @@ -436,8 +463,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,
> - struct list_head *objects)
> + struct eb_objects *eb)
> {
> struct drm_i915_gem_object *obj;
> int ret = 0;
> @@ -450,7 +476,7 @@ i915_gem_execbuffer_relocate(struct drm_device *dev,
> * lockdep complains vehemently.
> */
> pagefault_disable();
> - list_for_each_entry(obj, objects, exec_list) {
> + list_for_each_entry(obj, &eb->objects, exec_list) {
> ret = i915_gem_execbuffer_relocate_object(obj, eb);
> if (ret)
> break;
> @@ -618,24 +644,18 @@ static int
> i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
> struct drm_file *file,
> struct intel_ring_buffer *ring,
> - struct list_head *objects,
> struct eb_objects *eb,
> struct drm_i915_gem_exec_object2 *exec,
> int count)
> {
> struct drm_i915_gem_relocation_entry *reloc;
> struct drm_i915_gem_object *obj;
> + struct list_head *objects = &eb->objects;
> int *reloc_offset;
> int i, total, ret;
>
> /* We may process another execbuffer during the unlock... */
> - while (!list_empty(objects)) {
> - obj = list_first_entry(objects,
> - struct drm_i915_gem_object,
> - exec_list);
> - list_del_init(&obj->exec_list);
> - drm_gem_object_unreference(&obj->base);
> - }
> + eb_reset(eb);
>
> mutex_unlock(&dev->struct_mutex);
>
> @@ -676,7 +696,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
> }
>
> /* reacquire the objects */
> - eb_reset(eb);
> for (i = 0; i < count; i++) {
> obj = to_intel_bo(drm_gem_object_lookup(dev, file,
> exec[i].handle));
> @@ -687,7 +706,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
> goto err;
> }
>
> - list_add_tail(&obj->exec_list, objects);
> obj->exec_handle = exec[i].handle;
> obj->exec_entry = &exec[i];
> eb_add_object(eb, obj);
> @@ -1004,14 +1022,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> struct drm_i915_gem_exec_object2 *exec)
> {
> drm_i915_private_t *dev_priv = dev->dev_private;
> - struct list_head objects;
> - struct eb_objects *eb;
> - struct drm_i915_gem_object *batch_obj;
> + struct eb_objects *eb = NULL;
> + struct drm_i915_gem_object *obj = NULL;
> struct drm_clip_rect *cliprects = NULL;
> struct intel_ring_buffer *ring;
> u32 exec_start, exec_len;
> u32 seqno;
> - int ret, mode, i;
> + int ret, i;
>
> if (!i915_gem_check_execbuffer(args)) {
> DRM_ERROR("execbuf with invalid offset/length\n");
> @@ -1091,11 +1108,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> goto pre_mutex_err;
> }
>
> - /* Look up object handles */
> - INIT_LIST_HEAD(&objects);
> for (i = 0; i < args->buffer_count; i++) {
> - struct drm_i915_gem_object *obj;
> -
> obj = to_intel_bo(drm_gem_object_lookup(dev, file,
> exec[i].handle));
> if (&obj->base == NULL) {
> @@ -1113,28 +1126,25 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> goto err;
> }
>
> - list_add_tail(&obj->exec_list, &objects);
> obj->exec_handle = exec[i].handle;
> obj->exec_entry = &exec[i];
> eb_add_object(eb, obj);
> }
>
> - /* take note of the batch buffer before we might reorder the lists */
> - batch_obj = list_entry(objects.prev,
> - struct drm_i915_gem_object,
> - exec_list);
> + /* The last object is the batch object */
> + eb->batch_obj = obj;
>
> /* Move the objects en-masse into the GTT, evicting if necessary. */
> - ret = i915_gem_execbuffer_reserve(ring, file, &objects);
> + ret = i915_gem_execbuffer_reserve(ring, file, &eb->objects);
> if (ret)
> goto err;
>
> /* The objects are in their final locations, apply the relocations. */
> - ret = i915_gem_execbuffer_relocate(dev, eb, &objects);
> + ret = i915_gem_execbuffer_relocate(dev, eb);
> if (ret) {
> if (ret == -EFAULT) {
> ret = i915_gem_execbuffer_relocate_slow(dev, file, ring,
> - &objects, eb,
> + eb,
> exec,
> args->buffer_count);
Looks like you've planned to replace args->buffer_count with
eb->buffer_count, but didn't see it through. Please do it, cause I like
it.
> BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> @@ -1143,20 +1153,20 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> goto err;
> }
>
> - mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> - ret = i915_gem_set_constant_offset(ring, mode);
> + eb->mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> + ret = i915_gem_set_constant_offset(ring, eb->mode);
The eb->mode refactor here otoh looks a bit superflous. Is this needed for
a future patch of yours?
> if (ret)
> goto err;
>
> /* Set the pending read domains for the batch buffer to COMMAND */
> - if (batch_obj->base.pending_write_domain) {
> + if (eb->batch_obj->base.pending_write_domain) {
> DRM_ERROR("Attempting to use self-modifying batch buffer\n");
> ret = -EINVAL;
> goto err;
> }
> - batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
> + eb->batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
>
> - ret = i915_gem_execbuffer_move_to_gpu(ring, &objects);
> + ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->objects);
> if (ret)
> goto err;
>
> @@ -1177,7 +1187,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>
> trace_i915_gem_ring_dispatch(ring, seqno);
>
> - exec_start = batch_obj->gtt_offset + args->batch_start_offset;
> + exec_start = eb->batch_obj->gtt_offset + args->batch_start_offset;
Dito for eb->batch_obj, only used in do_execbuffer, afaics. Again, if you
need this later on, maybe explain it in the commit msg?
> exec_len = args->batch_len;
> if (cliprects) {
> for (i = 0; i < args->num_cliprects; i++) {
> @@ -1197,21 +1207,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> goto err;
> }
>
> - i915_gem_execbuffer_move_to_active(&objects, ring, seqno);
> + i915_gem_execbuffer_move_to_active(&eb->objects, ring, seqno);
> i915_gem_execbuffer_retire_commands(dev, file, ring);
>
> err:
> eb_destroy(eb);
> - while (!list_empty(&objects)) {
> - struct drm_i915_gem_object *obj;
> -
> - obj = list_first_entry(&objects,
> - struct drm_i915_gem_object,
> - exec_list);
> - list_del_init(&obj->exec_list);
> - drm_gem_object_unreference(&obj->base);
> - }
> -
> mutex_unlock(&dev->struct_mutex);
>
> pre_mutex_err:
> --
> 1.7.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
next prev parent reply other threads:[~2011-10-13 8:57 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-10-12 4:43 [PATCH 0/5] execbuffer cleanups + fixes Ben Widawsky
2011-10-12 4:43 ` [PATCH 1/5] drm/i915: execbuffer simplification Ben Widawsky
2011-10-12 4:43 ` [PATCH 2/5] drm/i915: extract constant offset setting Ben Widawsky
2011-10-12 16:46 ` Ben Widawsky
2011-10-12 4:43 ` [PATCH 3/5] drm/i915: make eb structure do more Ben Widawsky
2011-10-12 4:43 ` [PATCH 4/5] drm/i915: relative_constants_mode race fix Ben Widawsky
2011-10-12 4:43 ` [PATCH 5/5] drm/i915: Force sync command ordering (Gen6+) Ben Widawsky
2011-10-12 8:01 ` Ben Widawsky
2011-10-12 9:51 ` Daniel Vetter
2011-10-12 16:59 ` [PATCH 0/5] execbuffer cleanups + fixes Ben Widawsky
2011-10-12 22:56 ` [PATCH 0/4] pre-v2 series Ben Widawsky
2011-10-12 22:56 ` [PATCH 1/4] drm/i915: relative_constants_mode race fix Ben Widawsky
2011-10-13 8:35 ` Daniel Vetter
2011-10-13 17:25 ` Ben Widawsky
2011-10-12 22:56 ` [PATCH 2/4] drm/i915: Force sync command ordering (Gen6+) Ben Widawsky
2011-10-13 8:44 ` Daniel Vetter
2011-10-12 22:56 ` [PATCH 3/4] drm/i915: extract constant offset setting Ben Widawsky
2011-10-13 8:46 ` Daniel Vetter
2011-10-13 17:14 ` Ben Widawsky
2011-10-12 22:56 ` [PATCH 4/4] drm/i915: Use eb for more stuff Ben Widawsky
2011-10-13 8:58 ` Daniel Vetter [this message]
2011-10-13 12:00 ` Chris Wilson
2011-10-13 12:49 ` Daniel Vetter
2011-10-13 17:31 ` Ben Widawsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20111013085825.GF2938@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=ben@bwidawsk.net \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox