Re: [Intel-gfx] [PATCH 04/13] drm/i915: Use vma->exec_entry as our double-entry placeholder

[Chris Wilson]

On Fri, Mar 31, 2017 at 12:29:23PM +0300, Joonas Lahtinen wrote:
> Did you intend to rename too, or where did the title come from?

It's accurate. We have vma->exec_list (later vma->exec_link) that is the
vma's location on the execbufer list, and we have vma->exec_entry which
is the vma's execobj. Currently we use list_empty(>exec_list) to
determine if this vma is already in use in an execbuf ioctl (which is
two pointer loads, and two pointer sets to mark as unused) vs just
checking !vma->exec_entry which is simply. The caveat is to remember to
clear vma->exec_entry -- but that was already taken care of when I
rebased a later patch to fix softpinning.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/13] drm/i915: Use vma->exec_entry as our double-entry placeholder

[Joonas Lahtinen]

Did you intend to rename too, or where did the title come from?

On ke, 2017-03-29 at 16:56 +0100, Chris Wilson wrote:
> This has the benefit of not requiring us to manipulate the
> vma->exec_link list when tearing down the execbuffer, and is a
> marginally cheaper test to detect the user error.
> 
> Signed-off-by: Chris Wilson 



> @@ -85,7 +85,6 @@ vma_create(struct drm_i915_gem_object *obj,
>   if (vma == NULL)
>   return ERR_PTR(-ENOMEM);
>  
> - INIT_LIST_HEAD(>exec_list);

Dunno if it would be worth poisoning the pointer, maybe not.

With correct title;

Reviewed-by: Joonas Lahtinen 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 04/13] drm/i915: Use vma->exec_entry as our double-entry placeholder

[Chris Wilson]

This has the benefit of not requiring us to manipulate the
vma->exec_link list when tearing down the execbuffer, and is a
marginally cheaper test to detect the user error.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_gem_evict.c  | 17 ++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 77 --
 drivers/gpu/drm/i915/i915_vma.c|  1 -
 3 files changed, 44 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c 
b/drivers/gpu/drm/i915/i915_gem_evict.c
index 2da3a94fc9f3..ed34f54baef9 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -59,9 +59,6 @@ mark_free(struct drm_mm_scan *scan,
if (i915_vma_is_pinned(vma))
return false;
 
-   if (WARN_ON(!list_empty(>exec_list)))
-   return false;
-
if (flags & PIN_NONFAULT && !list_empty(>obj->userfault_link))
return false;
 
@@ -160,8 +157,6 @@ i915_gem_evict_something(struct i915_address_space *vm,
list_for_each_entry_safe(vma, next, _list, exec_list) {
ret = drm_mm_scan_remove_block(, >node);
BUG_ON(ret);
-
-   INIT_LIST_HEAD(>exec_list);
}
 
/* Can we unpin some objects such as idle hw contents,
@@ -210,17 +205,12 @@ i915_gem_evict_something(struct i915_address_space *vm,
if (drm_mm_scan_remove_block(, >node))
__i915_vma_pin(vma);
else
-   list_del_init(>exec_list);
+   list_del(>exec_list);
}
 
/* Unbinding will emit any required flushes */
ret = 0;
-   while (!list_empty(_list)) {
-   vma = list_first_entry(_list,
-  struct i915_vma,
-  exec_list);
-
-   list_del_init(>exec_list);
+   list_for_each_entry_safe(vma, next, _list, exec_list) {
__i915_vma_unpin(vma);
if (ret == 0)
ret = i915_vma_unbind(vma);
@@ -316,7 +306,7 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
}
 
/* Overlap of objects in the same batch? */
-   if (i915_vma_is_pinned(vma) || !list_empty(>exec_list)) {
+   if (i915_vma_is_pinned(vma)) {
ret = -ENOSPC;
if (vma->exec_entry &&
vma->exec_entry->flags & EXEC_OBJECT_PINNED)
@@ -337,7 +327,6 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
}
 
list_for_each_entry_safe(vma, next, _list, exec_list) {
-   list_del_init(>exec_list);
__i915_vma_unpin(vma);
if (ret == 0)
ret = i915_vma_unbind(vma);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 626be396c327..d365379c166b 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -109,13 +109,40 @@ eb_create(struct i915_execbuffer *eb)
eb->and = -eb->args->buffer_count;
}
 
-   INIT_LIST_HEAD(>vmas);
return 0;
 }
 
+static inline void
+__eb_unreserve_vma(struct i915_vma *vma,
+  const struct drm_i915_gem_exec_object2 *entry)
+{
+   if (unlikely(entry->flags & __EXEC_OBJECT_HAS_FENCE))
+   i915_vma_unpin_fence(vma);
+
+   if (entry->flags & __EXEC_OBJECT_HAS_PIN)
+   __i915_vma_unpin(vma);
+}
+
+static void
+eb_unreserve_vma(struct i915_vma *vma)
+{
+   struct drm_i915_gem_exec_object2 *entry = vma->exec_entry;
+
+   __eb_unreserve_vma(vma, entry);
+   entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN);
+}
+
 static void
 eb_reset(struct i915_execbuffer *eb)
 {
+   struct i915_vma *vma;
+
+   list_for_each_entry(vma, >vmas, exec_list) {
+   eb_unreserve_vma(vma);
+   i915_vma_put(vma);
+   vma->exec_entry = NULL;
+   }
+
if (eb->and >= 0)
memset(eb->buckets, 0, (eb->and+1)*sizeof(struct hlist_head));
 }
@@ -147,6 +174,8 @@ eb_lookup_vmas(struct i915_execbuffer *eb)
struct list_head objects;
int i, ret;
 
+   INIT_LIST_HEAD(>vmas);
+
INIT_LIST_HEAD();
spin_lock(>file->table_lock);
/* Grab a reference to the object and release the lock so we can lookup
@@ -253,40 +282,23 @@ static struct i915_vma *eb_get_vma(struct i915_execbuffer 
*eb, unsigned long han
}
 }
 
-static void
-eb_unreserve_vma(struct i915_vma *vma)
-{
-   struct drm_i915_gem_exec_object2 *entry;
-
-   if (!drm_mm_node_allocated(>node))
-   return;
-
-   entry = vma->exec_entry;
-
-   if (entry->flags & __EXEC_OBJECT_HAS_FENCE)
-   i915_vma_unpin_fence(vma);
-
-