drm/i915: Remove node only when allocated
Subsystems (3)
PATCH v2 1/4 drm/i915: Remove node only when allocated
VMAs can be created and not bound. One may think of it as lazy cleanup, and safely gloss over the conditions which manufacture it. In either case, when the object backing the i915 vma is destroyed, we must cleanup the vma without stumbling into a bunch of pitfalls that assume the vma is bound.
NOTE: I was pretty certain the above condition could only happen when we introduced the use of VMAs being looked up at execbuf, and already existing. Paulo has hit this though, so I must be missing something. As I believe the patch is correct anyway, therefore I won't scratch my head too hard.
v2: use goto destroy as a compromise (Chris)
--- drivers/gpu/drm/i915/i915_gem.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/gpu/drm/i915/i915_gem.c +5 -0
@@ -2606,6 +2606,9 @@ int i915_vma_unbind(struct i915_vma *vma)if (list_empty (&vma->vma_link))return 0 ; +if (!drm_mm_node_allocated (&vma->node)) + goto destroy; +if (obj->pin_count)return -EBUSY ;
@@ -2643,6 +2646,8 @@ int i915_vma_unbind(struct i915_vma *vma) obj->map_and_fenceable = true;drm_mm_remove_node (&vma->node); + +destroy:i915_gem_vma_destroy (vma);/* Since the unbound list is global, only move to that list if
Signature · 1 line
1.8.3.4
PATCH 3/4 drm: WARN when removing unallocated node
The conditional is usually a recoverable driver bug, and so WARNing, and preventing the drm_mm code from doing potential damage (BUG) is desirable.
This issue was hit and fixed twice while developing the i915 multiple address space code. The first fix is the patch just before this, and is hit on an not frequently occuring error path. Another was fixed during patch iteration, so it's hard to see from the patch:
commit c6cfb325677ea6305fb19acf3a4d14ea267f923e
Author: Ben Widawsky <[email protected]> Date: Fri Jul 5 14:41:06 2013 -0700
drm/i915: Embed drm_mm_node in i915 gem obj
Quoted 1 line
From the intel-gfx mailing list, we discussed this:
References: <[email protected]>
--- drivers/gpu/drm/drm_mm.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/gpu/drm/drm_mm.c +3 -0
@@ -254,6 +254,9 @@ void drm_mm_remove_node(struct drm_mm_node *node)struct drm_mm *mm = node->mm;struct drm_mm_node *prev_node; +if (WARN_ON (!node->allocated)) +return ; +BUG_ON (node->scanned_block || node->scanned_prev_free || node->scanned_next_free);
Signature · 1 line
1.8.3.4
Re: [Intel-gfx] [PATCH 3/4] drm: WARN when removing unallocated node
Quoted 15 lines · Ben Widawsky
The conditional is usually a recoverable driver bug, and so WARNing, and preventing the
drm_mmcode from doing potential damage (BUG) is desirable.This issue was hit and fixed twice while developing the i915 multiple address space code. The first fix is the patch just before this, and is hit on an not frequently occuring error path. Another was fixed during patch iteration, so it's hard to see from the patch:
commit
c6cfb325677ea6305fb19acf3a4d14ea267f923eAuthor: Ben Widawsky <[email protected]> Date: Fri Jul 5 14:41:06 2013 -0700drm/i915: Embed
drm_mm_nodein i915 gem obj
Quoted 7 lines
From the intel-gfx mailing list, we discussed this:
References: <[email protected]>Cc Dave Airlie <[email protected]>CC <[email protected]>Acked-by Chris Wilson <[email protected]>Signed-off-by Ben Widawsky <[email protected]>
Patches 2&3 of this series are merged to dinq, thanks. -Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH v3 2/4 drm/i915: cleanup map&fence in bind
In reply to Ben WidawskyCleanup the map and fenceable setting during bind to make more sense, and not check i915_is_ggtt() 2 unnecessary times
v2: Move the bools into the if block (Chris) - There are ways to tidy this function (fence calculations for instance) even further, but they are quite invasive, so I am punting on those unless specifically asked.
v3: Add newline between variable declaration and logic (Chris)
--- drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/i915_gem.c +9 -10
@@ -3106,7 +3106,6 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,struct drm_device *dev = obj->base.dev;drm_i915_private_t *dev_priv = dev->dev_private;u32 size, fence_size, fence_alignment, unfenced_alignment; -bool mappable, fenceable;size_t gtt_max = map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total;struct i915_vma *vma;
@@ -3191,18 +3190,18 @@ search_free:list_move_tail (&obj->global_list, &dev_priv->mm.bound_list);list_add_tail (&vma->mm_list, &vm->inactive_list); - fenceable = -i915_is_ggtt (vm) && -i915_gem_obj_ggtt_size (obj) == fence_size && - (i915_gem_obj_ggtt_offset (obj) & (fence_alignment -1 )) ==0 ; +if (i915_is_ggtt (vm)) { +bool mappable, fenceable; - mappable = -i915_is_ggtt (vm) && - vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; + fenceable = +i915_gem_obj_ggtt_size (obj) == fence_size && + (i915_gem_obj_ggtt_offset (obj) & (fence_alignment -1 )) ==0 ; + + mappable = + vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; -/* Map and fenceable only changes if the VM is the global GGTT */ -if (i915_is_ggtt (vm)) obj->map_and_fenceable = mappable && fenceable; + }WARN_ON (map_and_fenceable && !obj->map_and_fenceable);
Signature · 1 line
1.8.3.4
Re: [PATCH 2/4] [v3] drm/i915: cleanup map&fence in bind
Quoted 35 lines · Ben Widawsky
Cleanup the map and fenceable setting during bind to make more sense, and not check
i915_is_ggtt()2 unnecessary timesv2: Move the bools into the if block (Chris) - There are ways to tidy this function (fence calculations for instance) even further, but they are quite invasive, so I am punting on those unless specifically asked.
v3: Add newline between variable declaration and logic (Chris)
Recommended-by Chris Wilson <[email protected]>Reviewed-by Chris Wilson <[email protected]>Signed-off-by Ben Widawsky <[email protected]>--- drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)drivers/gpu/drm/i915/i915_gem.c +0
-4@@ -3106,7 +3106,6 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,struct drm_device *dev = obj->base.dev;drm_i915_private_t *dev_priv = dev->dev_private;u32 size, fence_size, fence_alignment, unfenced_alignment; -bool mappable, fenceable;size_t gtt_max = map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total;struct i915_vma *vma;@@ -3191,18 +3190,18 @@ search_free:list_move_tail (&obj->global_list, &dev_priv->mm.bound_list);list_add_tail (&vma->mm_list, &vm->inactive_list); - fenceable = -i915_is_ggtt (vm) && - i915_gem_obj_ggtt_size(obj) == fence_size &&
drivers/gpu/drm/i915/i915_gem.c search_free: · line 3198
Quoted 10 lines
drivers/gpu/drm/i915/i915_gem.c +5
-4- (i915_gem_obj_ggtt_offset (obj) & (fence_alignment -1 )) ==0 ; +if (i915_is_ggtt (vm)) { +bool mappable, fenceable; - mappable = -i915_is_ggtt (vm) && - vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; + fenceable = +i915_gem_obj_ggtt_size (obj) == fence_size && + (i915_gem_obj_ggtt_offset (obj) & (fence_alignment -1 )) ==0 ;
Why not go the full mounty and also use vma->node here? Would also make checkpatch a bit more happy. I'll do a follow-up commit. -Daniel
Quoted 18 lines
drivers/gpu/drm/i915/i915_gem.c +4
-2+ + mappable = + vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; -/* Map and fenceable only changes if the VM is the global GGTT */ -if (i915_is_ggtt (vm)) obj->map_and_fenceable = mappable && fenceable; + }WARN_ON (map_and_fenceable && !obj->map_and_fenceable);Signature · 6 lines
1.8.3.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH 2/4] [v3] drm/i915: cleanup map&fence in bind
Quoted 40 lines · Daniel Vetter
Quoted 39 lines · Ben Widawsky
Cleanup the map and fenceable setting during bind to make more sense, and not check
i915_is_ggtt()2 unnecessary timesv2: Move the bools into the if block (Chris) - There are ways to tidy this function (fence calculations for instance) even further, but they are quite invasive, so I am punting on those unless specifically asked.
v3: Add newline between variable declaration and logic (Chris)
Recommended-by Chris Wilson <[email protected]>Reviewed-by Chris Wilson <[email protected]>Signed-off-by Ben Widawsky <[email protected]>--- drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)drivers/gpu/drm/i915/i915_gem.c +2
-5@@ -3106,7 +3106,6 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,struct drm_device *dev = obj->base.dev;drm_i915_private_t *dev_priv = dev->dev_private;u32 size, fence_size, fence_alignment, unfenced_alignment; -bool mappable, fenceable;size_t gtt_max = map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total;struct i915_vma *vma;@@ -3191,18 +3190,18 @@ search_free:list_move_tail (&obj->global_list, &dev_priv->mm.bound_list);list_add_tail (&vma->mm_list, &vm->inactive_list); - fenceable = -i915_is_ggtt (vm) && -i915_gem_obj_ggtt_size (obj) == fence_size && - (i915_gem_obj_ggtt_offset (obj) & (fence_alignment -1 )) ==0 ; +if (i915_is_ggtt (vm)) { +bool mappable, fenceable;
Quoted 11 lines
drivers/gpu/drm/i915/i915_gem.c search_free: · line 3198
Quoted 6 lines
drivers/gpu/drm/i915/i915_gem.c +3
-3- mappable = -i915_is_ggtt (vm) && - vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; + fenceable = +i915_gem_obj_ggtt_size (obj) == fence_size && + (i915_gem_obj_ggtt_offset (obj) & (fence_alignment -1 )) ==0 ;Why not go the full mounty and also use
vma->nodehere? Would also make checkpatch a bit more happy. I'll do a follow-up commit. -Daniel
You've already done it, so it's moot. The idea I had was to use "ggtt" as much as possible when it can only every be ggtt. I think this would be helpful for both clarity, and debug.
The extra indirection would be immeasurable.
Again, you've already changed it, so meh.
Signature · 1 line
Ben Widawsky, Intel Open Source Technology Center
Re: [PATCH 2/4] [v3] drm/i915: cleanup map&fence in bind
Quoted 53 lines · Ben Widawsky
Quoted 40 lines · Daniel Vetter
Quoted 39 lines · Ben Widawsky
Cleanup the map and fenceable setting during bind to make more sense, and not check
i915_is_ggtt()2 unnecessary timesv2: Move the bools into the if block (Chris) - There are ways to tidy this function (fence calculations for instance) even further, but they are quite invasive, so I am punting on those unless specifically asked.
v3: Add newline between variable declaration and logic (Chris)
Recommended-by Chris Wilson <[email protected]>Reviewed-by Chris Wilson <[email protected]>Signed-off-by Ben Widawsky <[email protected]>--- drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)drivers/gpu/drm/i915/i915_gem.c +2
-5@@ -3106,7 +3106,6 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,struct drm_device *dev = obj->base.dev;drm_i915_private_t *dev_priv = dev->dev_private;u32 size, fence_size, fence_alignment, unfenced_alignment; -bool mappable, fenceable;size_t gtt_max = map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total;struct i915_vma *vma;@@ -3191,18 +3190,18 @@ search_free:list_move_tail (&obj->global_list, &dev_priv->mm.bound_list);list_add_tail (&vma->mm_list, &vm->inactive_list); - fenceable = -i915_is_ggtt (vm) && -i915_gem_obj_ggtt_size (obj) == fence_size && - (i915_gem_obj_ggtt_offset (obj) & (fence_alignment -1 )) ==0 ; +if (i915_is_ggtt (vm)) { +bool mappable, fenceable;Quoted 11 lines
drivers/gpu/drm/i915/i915_gem.c search_free: · line 3198
Quoted 6 lines
drivers/gpu/drm/i915/i915_gem.c +3
-3- mappable = -i915_is_ggtt (vm) && - vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; + fenceable = +i915_gem_obj_ggtt_size (obj) == fence_size && + (i915_gem_obj_ggtt_offset (obj) & (fence_alignment -1 )) ==0 ;Why not go the full mounty and also use
vma->nodehere? Would also make checkpatch a bit more happy. I'll do a follow-up commit. -Daniel
Quoted 3 lines
You've already done it, so it's moot. The idea I had was to use "ggtt" as much as possible when it can only every be ggtt. I think this would be helpful for both clarity, and debug.
I disagree here and I think the extra indirection hampers code readability
- I always end up checking your little functions to make sure they actually fish out the right value from the right vma. So my plan is that once this all lands I'll fully rip them out.
This wasn't ever about performance, although I admit that unnecessary looping in our gem code does irk me a bit ;-) But again mostly from a clarify pov.
For marking ggtt-only paths I think Chris' suggestion to enclose such code in if (is_ggtt(vm)) {} blocks which you've implemented here is much better for clarity.
Cheers, Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH v4 4/4 drm/i915: Convert execbuf code to use vmas
In reply to Ben WidawskyFrom: Ben Widawsky <[email protected]>
In order to transition more of our code over to using a VMA instead of an <OBJ, VM> pair - we must have the vma accessible at execbuf time. Up until now, we've only had a VMA when actually binding an object.
The previous patch helped handle the distinction on bound vs. unbound. This patch will help us catch leaks, and other issues before we actually shuffle a bunch of stuff around.
This attempts to convert all the execbuf code to speak in vmas. Since the execbuf code is very self contained it was a nice isolated conversion.
The meat of the code is about turning eb_objects into eb_vma, and then wiring up the rest of the code to use vmas instead of obj, vm pairs.
Unfortunately, to do this, we must move the exec_list link from the obj structure. This list is reused in the eviction code, so we must also modify the eviction code to make this work.
WARNING: This patch makes an already hotly profiled path slower. The cost is unavoidable. In reply to this mail, I will attach the extra data.
v2: Release table lock early, and two a 2 phase vma lookup to avoid having to use a GFP_ATOMIC. (Chris)
v3: s/obj_exec_list/obj_exec_link/
Updates to address
commit 6d2b888569d366beb4be72cacfde41adee2c25e1
Author: Chris Wilson <[email protected]> Date: Wed Aug 7 18:30:54 2013 +0100
drm/i915: List objects allocated from stolen memory in debugfs
v4: Use obj = vma->obj for neatness in some places (Chris) need_reloc_mappable() should return false if ppgtt (Chris)
--- drivers/gpu/drm/i915/i915_debugfs.c | 12 +- drivers/gpu/drm/i915/i915_drv.h | 25 ++- drivers/gpu/drm/i915/i915_gem.c | 28 ++- drivers/gpu/drm/i915/i915_gem_evict.c | 31 ++- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 319 ++++++++++++++++------------- 5 files changed, 232 insertions(+), 183 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +6 -6
@@ -195,9 +195,9 @@ static int obj_rank_by_stolen(void *priv, struct list_head *A, struct list_head *B) { struct drm_i915_gem_object *a = - container_of(A, struct drm_i915_gem_object, exec_list); + container_of(A, struct drm_i915_gem_object, obj_exec_link);struct drm_i915_gem_object *b = - container_of(B, struct drm_i915_gem_object, exec_list); + container_of(B, struct drm_i915_gem_object, obj_exec_link);return a->stolen->start - b->stolen->start; }
@@ -221,7 +221,7 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)if (obj->stolen ==NULL )continue ; -list_add (&obj->exec_list, &stolen); +list_add (&obj->obj_exec_link, &stolen); total_obj_size += obj->base.size; total_gtt_size +=i915_gem_obj_ggtt_size (obj);
@@ -231,7 +231,7 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)if (obj->stolen ==NULL )continue ; -list_add (&obj->exec_list, &stolen); +list_add (&obj->obj_exec_link, &stolen); total_obj_size += obj->base.size; count++;
@@ -239,11 +239,11 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)list_sort (NULL , &stolen, obj_rank_by_stolen);seq_puts (m,"Stolen:\n" );while (!list_empty (&stolen)) { - obj =list_first_entry (&stolen,typeof (*obj), exec_list); + obj =list_first_entry (&stolen,typeof (*obj), obj_exec_link);seq_puts (m," " );describe_obj (m, obj);seq_putc (m,'\n' ); -list_del_init (&obj->exec_list); +list_del_init (&obj->obj_exec_link); }mutex_unlock (&dev->struct_mutex);
drivers/gpu/drm/i915/i915_drv.h +16 -9
@@ -563,6 +563,17 @@ struct i915_vma {struct list_head mm_list;struct list_head vma_link;/* Link in the object's VMA list */ + +/** This vma's place in the batchbuffer or on the eviction list */ +struct list_head exec_list; + +/** +* Used for performing relocations during execbuffer insertion. +*/ +struct hlist_node exec_node; +unsigned long exec_handle; +struct drm_i915_gem_exec_object2 *exec_entry; + };struct i915_ctx_hang_stats {
@@ -1312,8 +1323,8 @@ struct drm_i915_gem_object {struct list_head global_list;struct list_head ring_list; -/** This object's place in the batchbuffer or on the eviction list */ -struct list_head exec_list; +/** Used in execbuf to temporarily hold a ref */ +struct list_head obj_exec_link;/** * This is set if the object is on the active lists (has pending
@@ -1399,13 +1410,6 @@ struct drm_i915_gem_object {void *dma_buf_vmapping;int vmapping_count; -/** -* Used for performing relocations during execbuffer insertion. -*/ -struct hlist_node exec_node; -unsigned long exec_handle; -struct drm_i915_gem_exec_object2 *exec_entry; -struct intel_ring_buffer *ring;/** Breadcrumb of last rendering to the buffer. */
@@ -1907,6 +1911,9 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, struct i915_address_space *vm);struct i915_vma *i915_gem_obj_to_vma (struct drm_i915_gem_object *obj,struct i915_address_space *vm); +struct i915_vma * +i915_gem_obj_lookup_or_create_vma (struct drm_i915_gem_object *obj, +struct i915_address_space *vm);/* Some GGTT VM helpers */ #define obj_to_ggtt (obj) \ (&((struct drm_i915_private *)(obj)->base.dev->dev_private)->gtt.base)
drivers/gpu/drm/i915/i915_gem.c +20 -8
@@ -3111,8 +3111,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,struct i915_vma *vma;int ret; -if (WARN_ON (!list_empty (&obj->vma_list))) -return -EBUSY ; +BUG_ON (!i915_is_ggtt (vm)); fence_size = i915_gem_get_gtt_size(dev, obj->base.size,
@@ -3152,16 +3151,15 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,i915_gem_object_pin_pages (obj); -/* FIXME: For now we only ever use 1 VMA per object */ -BUG_ON (!i915_is_ggtt (vm)); -WARN_ON (!list_empty (&obj->vma_list)); - - vma =i915_gem_vma_create (obj, vm); + vma =i915_gem_obj_lookup_or_create_vma (obj, vm);if (IS_ERR (vma)) { ret =PTR_ERR (vma); goto err_unpin; } +/* For now we only ever use 1 vma per object */ +WARN_ON (!list_is_singular (&obj->vma_list)); + search_free: ret = drm_mm_insert_node_in_range_generic(&vm->mm, &vma->node, size, alignment,
@@ -3981,7 +3979,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, {INIT_LIST_HEAD (&obj->global_list);INIT_LIST_HEAD (&obj->ring_list); -INIT_LIST_HEAD (&obj->exec_list); +INIT_LIST_HEAD (&obj->obj_exec_link);INIT_LIST_HEAD (&obj->vma_list); obj->ops = ops;
@@ -4121,6 +4119,7 @@ struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,INIT_LIST_HEAD (&vma->vma_link);INIT_LIST_HEAD (&vma->mm_list); +INIT_LIST_HEAD (&vma->exec_list); vma->vm = vm; vma->obj = obj;
@@ -4870,3 +4869,16 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,return NULL ; } + +struct i915_vma * +i915_gem_obj_lookup_or_create_vma (struct drm_i915_gem_object *obj, +struct i915_address_space *vm) +{ +struct i915_vma *vma; + + vma =i915_gem_obj_to_vma (obj, vm); +if (!vma) + vma =i915_gem_vma_create (obj, vm); + +return vma; +}
drivers/gpu/drm/i915/i915_gem_evict.c +14 -17
@@ -37,7 +37,7 @@ mark_free(struct i915_vma *vma, struct list_head *unwind)if (vma->obj->pin_count)return false; -list_add (&vma->obj->exec_list, unwind); +list_add (&vma->exec_list, unwind);return drm_mm_scan_add_block (&vma->node); }
@@ -49,7 +49,6 @@ i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm,drm_i915_private_t *dev_priv = dev->dev_private;struct list_head eviction_list, unwind_list;struct i915_vma *vma; -struct drm_i915_gem_object *obj;int ret =0 ;trace_i915_gem_evict (dev, min_size, alignment, mappable);
@@ -104,14 +103,13 @@ i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm, none:/* Nothing found, clean up and bail out! */ while (!list_empty (&unwind_list)) { - obj =list_first_entry (&unwind_list, - struct drm_i915_gem_object, + vma =list_first_entry (&unwind_list, + struct i915_vma, exec_list); - vma =i915_gem_obj_to_vma (obj, vm); ret =drm_mm_scan_remove_block (&vma->node);BUG_ON (ret); -list_del_init (&obj->exec_list); +list_del_init (&vma->exec_list); }/* We expect the caller to unpin, evict all and try again, or give up.
@@ -125,28 +123,27 @@ found:* temporary list. */ INIT_LIST_HEAD (&eviction_list);while (!list_empty (&unwind_list)) { - obj =list_first_entry (&unwind_list, - struct drm_i915_gem_object, + vma =list_first_entry (&unwind_list, + struct i915_vma, exec_list); - vma =i915_gem_obj_to_vma (obj, vm);if (drm_mm_scan_remove_block (&vma->node)) { -list_move (&obj->exec_list, &eviction_list); -drm_gem_object_reference (&obj->base); +list_move (&vma->exec_list, &eviction_list); +drm_gem_object_reference (&vma->obj->base);continue ; } -list_del_init (&obj->exec_list); +list_del_init (&vma->exec_list); }/* Unbinding will emit any required flushes */ while (!list_empty (&eviction_list)) { - obj =list_first_entry (&eviction_list, - struct drm_i915_gem_object, + vma =list_first_entry (&eviction_list, + struct i915_vma, exec_list);if (ret ==0 ) - ret =i915_vma_unbind (i915_gem_obj_to_vma (obj, vm)); + ret =i915_vma_unbind (vma); -list_del_init (&obj->exec_list); -drm_gem_object_unreference (&obj->base); +list_del_init (&vma->exec_list); +drm_gem_object_unreference (&vma->obj->base); }return ret;
drivers/gpu/drm/i915/i915_gem_execbuffer.c +176 -143
649 lines not on this page · raw message
Signature · 1 line
1.8.3.4
Re: [PATCH 4/4] [v4] drm/i915: Convert execbuf code to use vmas
Quoted 17 lines · Ben Widawsky
From: Ben Widawsky <[email protected]>
In order to transition more of our code over to using a VMA instead of an <OBJ, VM> pair - we must have the vma accessible at execbuf time. Up until now, we've only had a VMA when actually binding an object.
The previous patch helped handle the distinction on bound vs. unbound. This patch will help us catch leaks, and other issues before we actually shuffle a bunch of stuff around.
This attempts to convert all the execbuf code to speak in vmas. Since the execbuf code is very self contained it was a nice isolated conversion.
The meat of the code is about turning
eb_objectsintoeb_vma, and then wiring up the rest of the code to use vmas instead of obj, vm pairs.
Quoted 7 lines
Unfortunately, to do this, we must move the
exec_listlink from the obj structure. This list is reused in the eviction code, so we must also modify the eviction code to make this work.WARNING: This patch makes an already hotly profiled path slower. The cost is unavoidable. In reply to this mail, I will attach the extra data.
[snip]
Here is the output from gem_exec_lut_handle both before and after this patch. The results honestly don't make sense to me, but I'll set Chris parse it before scratching my head harder.
Before patch
============
relocation: buffers= 1: old= 8060 + 165.3*reloc, lut= 7816 + 164.8*reloc (ns)
relocation: buffers= 2: old= 6748 + 166.6*reloc, lut= 6952 + 165.4*reloc (ns)
relocation: buffers= 4: old= 8140 + 165.9*reloc, lut= 8216 + 165.4*reloc (ns)
relocation: buffers= 8: old= 10732 + 166.0*reloc, lut= 10615 + 165.2*reloc (ns)
relocation: buffers= 16: old= 15099 + 167.8*reloc, lut= 15337 + 165.3*reloc (ns)
relocation: buffers= 32: old= 26140 + 166.0*reloc, lut= 25488 + 165.5*reloc (ns)
relocation: buffers= 64: old= 46300 + 170.5*reloc, lut= 44279 + 166.7*reloc (ns)
relocation: buffers= 128: old= 84056 + 176.9*reloc, lut= 85379 + 166.3*reloc (ns)
relocation: buffers= 256: old= 174398 + 167.9*reloc, lut= 167744 + 167.0*reloc (ns)
relocation: buffers= 512: old= 349688 + 175.7*reloc, lut= 348590 + 170.8*reloc (ns)
relocation: buffers=1024: old= 726265 + 191.2*reloc, lut= 719774 + 180.2*reloc (ns)
relocation: buffers=2048: old=1456866 + 224.3*reloc, lut=1442087 + 173.0*reloc (ns)
skip-relocs: buffers= 1: old= 4445 + 16.0*reloc, lut= 4433 + 15.6*reloc (ns)
skip-relocs: buffers= 2: old= 4585 + 16.0*reloc, lut= 4571 + 15.6*reloc (ns)
skip-relocs: buffers= 4: old= 5667 + 16.0*reloc, lut= 5340 + 15.6*reloc (ns)
skip-relocs: buffers= 8: old= 6051 + 16.1*reloc, lut= 6026 + 15.6*reloc (ns)
skip-relocs: buffers= 16: old= 7953 + 16.1*reloc, lut= 7914 + 15.6*reloc (ns)
skip-relocs: buffers= 32: old= 11972 + 16.2*reloc, lut= 11875 + 15.7*reloc (ns)
skip-relocs: buffers= 64: old= 19999 + 16.5*reloc, lut= 19832 + 15.7*reloc (ns)
skip-relocs: buffers= 128: old= 37796 + 16.9*reloc, lut= 36539 + 15.9*reloc (ns)
skip-relocs: buffers= 256: old= 71604 + 18.1*reloc, lut= 71313 + 16.5*reloc (ns)
skip-relocs: buffers= 512: old= 152682 + 24.3*reloc, lut= 141379 + 27.9*reloc (ns)
skip-relocs: buffers=1024: old= 314116 + 41.7*reloc, lut= 303019 + 20.1*reloc (ns)
skip-relocs: buffers=2048: old= 619784 + 54.1*reloc, lut= 603931 + 20.0*reloc (ns)
no-relocs: buffers= 1: old= 4194 + 5.1*reloc, lut= 4206 + 4.8*reloc (ns)
no-relocs: buffers= 2: old= 4404 + 5.1*reloc, lut= 4381 + 4.8*reloc (ns)
no-relocs: buffers= 4: old= 4926 + 5.1*reloc, lut= 4921 + 4.8*reloc (ns)
no-relocs: buffers= 8: old= 5901 + 5.1*reloc, lut= 5822 + 4.9*reloc (ns)
no-relocs: buffers= 16: old= 7840 + 5.1*reloc, lut= 7737 + 4.9*reloc (ns)
no-relocs: buffers= 32: old= 11842 + 5.1*reloc, lut= 11681 + 4.9*reloc (ns)
no-relocs: buffers= 64: old= 19741 + 5.1*reloc, lut= 19542 + 4.8*reloc (ns)
no-relocs: buffers= 128: old= 36479 + 5.2*reloc, lut= 35958 + 4.9*reloc (ns)
no-relocs: buffers= 256: old= 70171 + 5.4*reloc, lut= 69390 + 5.2*reloc (ns)
no-relocs: buffers= 512: old= 147213 + 3.5*reloc, lut= 137953 + 13.0*reloc (ns)
no-relocs: buffers=1024: old= 300165 + 4.8*reloc, lut= 293852 + 4.9*reloc (ns)
no-relocs: buffers=2048: old= 597992 + 8.3*reloc, lut= 590185 + 2.1*reloc (ns)
After patch
===========
relocation: buffers= 1: old= 8075 + 81.4*reloc, lut= 7592 + 80.6*reloc (ns)
relocation: buffers= 2: old= 5744 + 82.3*reloc, lut= 5837 + 81.1*reloc (ns)
relocation: buffers= 4: old= 4875 + 82.7*reloc, lut= 4871 + 81.6*reloc (ns)
relocation: buffers= 8: old= 5729 + 82.7*reloc, lut= 5698 + 81.5*reloc (ns)
relocation: buffers= 16: old= 7952 + 83.0*reloc, lut= 7809 + 81.9*reloc (ns)
relocation: buffers= 32: old= 11884 + 82.9*reloc, lut= 11702 + 81.6*reloc (ns)
relocation: buffers= 64: old= 20388 + 83.4*reloc, lut= 19995 + 82.2*reloc (ns)
relocation: buffers= 128: old= 38057 + 85.0*reloc, lut= 37675 + 83.4*reloc (ns)
relocation: buffers= 256: old= 74912 + 87.0*reloc, lut= 74064 + 85.4*reloc (ns)
relocation: buffers= 512: old= 161136 + 94.8*reloc, lut= 157046 + 87.5*reloc (ns)
relocation: buffers=1024: old= 349443 + 107.0*reloc, lut= 342081 + 91.2*reloc (ns)
relocation: buffers=2048: old= 707951 + 131.8*reloc, lut= 690754 + 96.9*reloc (ns)
skip-relocs: buffers= 1: old= 2966 + 16.6*reloc, lut= 2963 + 15.6*reloc (ns)
skip-relocs: buffers= 2: old= 3083 + 16.5*reloc, lut= 3056 + 15.5*reloc (ns)
skip-relocs: buffers= 4: old= 3279 + 16.6*reloc, lut= 3242 + 15.6*reloc (ns)
skip-relocs: buffers= 8: old= 3692 + 16.7*reloc, lut= 3654 + 15.6*reloc (ns)
skip-relocs: buffers= 16: old= 4522 + 16.7*reloc, lut= 4461 + 15.5*reloc (ns)
skip-relocs: buffers= 32: old= 6254 + 16.7*reloc, lut= 6138 + 15.7*reloc (ns)
skip-relocs: buffers= 64: old= 10098 + 16.8*reloc, lut= 9939 + 15.7*reloc (ns)
skip-relocs: buffers= 128: old= 17983 + 17.6*reloc, lut= 17729 + 16.3*reloc (ns)
skip-relocs: buffers= 256: old= 34388 + 18.8*reloc, lut= 33981 + 17.6*reloc (ns)
skip-relocs: buffers= 512: old= 74211 + 25.2*reloc, lut= 72185 + 18.6*reloc (ns)
skip-relocs: buffers=1024: old= 160514 + 34.1*reloc, lut= 157086 + 20.3*reloc (ns)
skip-relocs: buffers=2048: old= 323954 + 51.5*reloc, lut= 315928 + 22.5*reloc (ns)
no-relocs: buffers= 1: old= 2840 + 5.1*reloc, lut= 2834 + 4.8*reloc (ns)
no-relocs: buffers= 2: old= 2938 + 5.1*reloc, lut= 2917 + 4.8*reloc (ns)
no-relocs: buffers= 4: old= 3220 + 5.1*reloc, lut= 3201 + 4.8*reloc (ns)
no-relocs: buffers= 8: old= 3614 + 5.1*reloc, lut= 3545 + 4.8*reloc (ns)
no-relocs: buffers= 16: old= 4437 + 5.1*reloc, lut= 4368 + 4.8*reloc (ns)
no-relocs: buffers= 32: old= 6105 + 5.1*reloc, lut= 6024 + 4.9*reloc (ns)
no-relocs: buffers= 64: old= 9864 + 5.1*reloc, lut= 9652 + 4.9*reloc (ns)
no-relocs: buffers= 128: old= 17388 + 5.1*reloc, lut= 17126 + 4.9*reloc (ns)
no-relocs: buffers= 256: old= 33087 + 5.4*reloc, lut= 32668 + 5.3*reloc (ns)
no-relocs: buffers= 512: old= 71476 + 5.0*reloc, lut= 69464 + 4.9*reloc (ns)
no-relocs: buffers=1024: old= 154379 + 4.9*reloc, lut= 152796 + 4.3*reloc (ns)
no-relocs: buffers=2048: old= 309435 + 5.0*reloc, lut= 301095 + 4.9*reloc (ns)
Signature · 1 line
Ben Widawsky, Intel Open Source Technology Center
Re: [PATCH 4/4] [v4] drm/i915: Convert execbuf code to use vmas
Quoted 102 lines · Ben Widawsky
Quoted 17 lines · Ben Widawsky
From: Ben Widawsky <[email protected]>
In order to transition more of our code over to using a VMA instead of an <OBJ, VM> pair - we must have the vma accessible at execbuf time. Up until now, we've only had a VMA when actually binding an object.
The previous patch helped handle the distinction on bound vs. unbound. This patch will help us catch leaks, and other issues before we actually shuffle a bunch of stuff around.
This attempts to convert all the execbuf code to speak in vmas. Since the execbuf code is very self contained it was a nice isolated conversion.
The meat of the code is about turning
eb_objectsintoeb_vma, and then wiring up the rest of the code to use vmas instead of obj, vm pairs.Quoted 7 lines
Unfortunately, to do this, we must move the
exec_listlink from the obj structure. This list is reused in the eviction code, so we must also modify the eviction code to make this work.WARNING: This patch makes an already hotly profiled path slower. The cost is unavoidable. In reply to this mail, I will attach the extra data.
[snip]
Here is the output from
gem_exec_lut_handleboth before and after this patch. The results honestly don't make sense to me, but I'll set Chris parse it before scratching my head harder.Before patch
============
relocation: buffers= 1: old= 8060 + 165.3*reloc, lut= 7816 + 164.8*reloc (ns)
relocation: buffers= 2: old= 6748 + 166.6*reloc, lut= 6952 + 165.4*reloc (ns)
relocation: buffers= 4: old= 8140 + 165.9*reloc, lut= 8216 + 165.4*reloc (ns)
relocation: buffers= 8: old= 10732 + 166.0*reloc, lut= 10615 + 165.2*reloc (ns)
relocation: buffers= 16: old= 15099 + 167.8*reloc, lut= 15337 + 165.3*reloc (ns)
relocation: buffers= 32: old= 26140 + 166.0*reloc, lut= 25488 + 165.5*reloc (ns)
relocation: buffers= 64: old= 46300 + 170.5*reloc, lut= 44279 + 166.7*reloc (ns)
relocation: buffers= 128: old= 84056 + 176.9*reloc, lut= 85379 + 166.3*reloc (ns)
relocation: buffers= 256: old= 174398 + 167.9*reloc, lut= 167744 + 167.0*reloc (ns)
relocation: buffers= 512: old= 349688 + 175.7*reloc, lut= 348590 + 170.8*reloc (ns)
relocation: buffers=1024: old= 726265 + 191.2*reloc, lut= 719774 + 180.2*reloc (ns)
relocation: buffers=2048: old=1456866 + 224.3*reloc, lut=1442087 + 173.0*reloc (ns)
skip-relocs: buffers= 1: old= 4445 + 16.0*reloc, lut= 4433 + 15.6*reloc (ns)
skip-relocs: buffers= 2: old= 4585 + 16.0*reloc, lut= 4571 + 15.6*reloc (ns)
skip-relocs: buffers= 4: old= 5667 + 16.0*reloc, lut= 5340 + 15.6*reloc (ns)
skip-relocs: buffers= 8: old= 6051 + 16.1*reloc, lut= 6026 + 15.6*reloc (ns)
skip-relocs: buffers= 16: old= 7953 + 16.1*reloc, lut= 7914 + 15.6*reloc (ns)
skip-relocs: buffers= 32: old= 11972 + 16.2*reloc, lut= 11875 + 15.7*reloc (ns)
skip-relocs: buffers= 64: old= 19999 + 16.5*reloc, lut= 19832 + 15.7*reloc (ns)
skip-relocs: buffers= 128: old= 37796 + 16.9*reloc, lut= 36539 + 15.9*reloc (ns)
skip-relocs: buffers= 256: old= 71604 + 18.1*reloc, lut= 71313 + 16.5*reloc (ns)
skip-relocs: buffers= 512: old= 152682 + 24.3*reloc, lut= 141379 + 27.9*reloc (ns)
skip-relocs: buffers=1024: old= 314116 + 41.7*reloc, lut= 303019 + 20.1*reloc (ns)
skip-relocs: buffers=2048: old= 619784 + 54.1*reloc, lut= 603931 + 20.0*reloc (ns)
no-relocs: buffers= 1: old= 4194 + 5.1*reloc, lut= 4206 + 4.8*reloc (ns)
no-relocs: buffers= 2: old= 4404 + 5.1*reloc, lut= 4381 + 4.8*reloc (ns)
no-relocs: buffers= 4: old= 4926 + 5.1*reloc, lut= 4921 + 4.8*reloc (ns)
no-relocs: buffers= 8: old= 5901 + 5.1*reloc, lut= 5822 + 4.9*reloc (ns)
no-relocs: buffers= 16: old= 7840 + 5.1*reloc, lut= 7737 + 4.9*reloc (ns)
no-relocs: buffers= 32: old= 11842 + 5.1*reloc, lut= 11681 + 4.9*reloc (ns)
no-relocs: buffers= 64: old= 19741 + 5.1*reloc, lut= 19542 + 4.8*reloc (ns)
no-relocs: buffers= 128: old= 36479 + 5.2*reloc, lut= 35958 + 4.9*reloc (ns)
no-relocs: buffers= 256: old= 70171 + 5.4*reloc, lut= 69390 + 5.2*reloc (ns)
no-relocs: buffers= 512: old= 147213 + 3.5*reloc, lut= 137953 + 13.0*reloc (ns)
no-relocs: buffers=1024: old= 300165 + 4.8*reloc, lut= 293852 + 4.9*reloc (ns)
no-relocs: buffers=2048: old= 597992 + 8.3*reloc, lut= 590185 + 2.1*reloc (ns)After patch
===========
relocation: buffers= 1: old= 8075 + 81.4*reloc, lut= 7592 + 80.6*reloc (ns)
relocation: buffers= 2: old= 5744 + 82.3*reloc, lut= 5837 + 81.1*reloc (ns)
relocation: buffers= 4: old= 4875 + 82.7*reloc, lut= 4871 + 81.6*reloc (ns)
relocation: buffers= 8: old= 5729 + 82.7*reloc, lut= 5698 + 81.5*reloc (ns)
relocation: buffers= 16: old= 7952 + 83.0*reloc, lut= 7809 + 81.9*reloc (ns)
relocation: buffers= 32: old= 11884 + 82.9*reloc, lut= 11702 + 81.6*reloc (ns)
relocation: buffers= 64: old= 20388 + 83.4*reloc, lut= 19995 + 82.2*reloc (ns)
relocation: buffers= 128: old= 38057 + 85.0*reloc, lut= 37675 + 83.4*reloc (ns)
relocation: buffers= 256: old= 74912 + 87.0*reloc, lut= 74064 + 85.4*reloc (ns)
relocation: buffers= 512: old= 161136 + 94.8*reloc, lut= 157046 + 87.5*reloc (ns)
relocation: buffers=1024: old= 349443 + 107.0*reloc, lut= 342081 + 91.2*reloc (ns)
relocation: buffers=2048: old= 707951 + 131.8*reloc, lut= 690754 + 96.9*reloc (ns)
skip-relocs: buffers= 1: old= 2966 + 16.6*reloc, lut= 2963 + 15.6*reloc (ns)
skip-relocs: buffers= 2: old= 3083 + 16.5*reloc, lut= 3056 + 15.5*reloc (ns)
skip-relocs: buffers= 4: old= 3279 + 16.6*reloc, lut= 3242 + 15.6*reloc (ns)
skip-relocs: buffers= 8: old= 3692 + 16.7*reloc, lut= 3654 + 15.6*reloc (ns)
skip-relocs: buffers= 16: old= 4522 + 16.7*reloc, lut= 4461 + 15.5*reloc (ns)
skip-relocs: buffers= 32: old= 6254 + 16.7*reloc, lut= 6138 + 15.7*reloc (ns)
skip-relocs: buffers= 64: old= 10098 + 16.8*reloc, lut= 9939 + 15.7*reloc (ns)
skip-relocs: buffers= 128: old= 17983 + 17.6*reloc, lut= 17729 + 16.3*reloc (ns)
skip-relocs: buffers= 256: old= 34388 + 18.8*reloc, lut= 33981 + 17.6*reloc (ns)
skip-relocs: buffers= 512: old= 74211 + 25.2*reloc, lut= 72185 + 18.6*reloc (ns)
skip-relocs: buffers=1024: old= 160514 + 34.1*reloc, lut= 157086 + 20.3*reloc (ns)
skip-relocs: buffers=2048: old= 323954 + 51.5*reloc, lut= 315928 + 22.5*reloc (ns)
no-relocs: buffers= 1: old= 2840 + 5.1*reloc, lut= 2834 + 4.8*reloc (ns)
no-relocs: buffers= 2: old= 2938 + 5.1*reloc, lut= 2917 + 4.8*reloc (ns)
no-relocs: buffers= 4: old= 3220 + 5.1*reloc, lut= 3201 + 4.8*reloc (ns)
no-relocs: buffers= 8: old= 3614 + 5.1*reloc, lut= 3545 + 4.8*reloc (ns)
Quoted 8 lines
no-relocs: buffers= 16: old= 4437 + 5.1*reloc, lut= 4368 + 4.8*reloc (ns)
no-relocs: buffers= 32: old= 6105 + 5.1*reloc, lut= 6024 + 4.9*reloc (ns)
no-relocs: buffers= 64: old= 9864 + 5.1*reloc, lut= 9652 + 4.9*reloc (ns)
no-relocs: buffers= 128: old= 17388 + 5.1*reloc, lut= 17126 + 4.9*reloc (ns)
no-relocs: buffers= 256: old= 33087 + 5.4*reloc, lut= 32668 + 5.3*reloc (ns)
no-relocs: buffers= 512: old= 71476 + 5.0*reloc, lut= 69464 + 4.9*reloc (ns)
no-relocs: buffers=1024: old= 154379 + 4.9*reloc, lut= 152796 + 4.3*reloc (ns)
no-relocs: buffers=2048: old= 309435 + 5.0*reloc, lut= 301095 + 4.9*reloc (ns)
Hmm, either the patch really did make things faster or the results are being subjected to cpufreq.
relocation: do the full relocation processing
skip-relocs: all the relocation addresses are correct, so no rewrite
no-relocs: no buffers moved, no relocation processing
"Buffers" is the number of buffers passed into execbuffer, and so we measure the cost of trying to process a certain complexity of a batch. Then we do a least-squares plot through a number of batches with varying numbers of relocations to estimate the overhead of processing the execbuffer array versus the cost of each addition relocation in the batch. The first number (the x_0 intercept in nanoseconds) should be the measure of how long it takes to grab all the buffers into our local structures - this is what I was expecting to be worst hit by the vma patches.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 4/4] [v4] drm/i915: Convert execbuf code to use vmas
Quoted 81 lines · Chris Wilson
These are my numbers for a beefy haswell box (note the really interesting numbers will be on Baytrail):
unpatched:
relocation: buffers= 1: old= 21945 + 34.4*reloc, lut= 21814 + 34.0*reloc (ns)
relocation: buffers= 2: old= 15947 + 36.4*reloc, lut= 16169 + 35.4*reloc (ns)
relocation: buffers= 4: old= 12711 + 37.6*reloc, lut= 13039 + 36.7*reloc (ns)
relocation: buffers= 8: old= 6154 + 40.9*reloc, lut= 7201 + 38.9*reloc (ns)
relocation: buffers= 16: old= 4846 + 41.6*reloc, lut= 5337 + 40.6*reloc (ns)
relocation: buffers= 32: old= 7097 + 41.9*reloc, lut= 6943 + 41.0*reloc (ns)
relocation: buffers= 64: old= 13318 + 41.9*reloc, lut= 12748 + 41.2*reloc (ns)
relocation: buffers= 128: old= 27282 + 43.0*reloc, lut= 25778 + 41.7*reloc (ns)
relocation: buffers= 256: old= 54535 + 45.2*reloc, lut= 51912 + 43.7*reloc (ns)
relocation: buffers= 512: old= 137447 + 53.2*reloc, lut= 129333 + 45.5*reloc (ns)
relocation: buffers=1024: old= 307347 + 66.5*reloc, lut= 291487 + 48.1*reloc (ns)
relocation: buffers=2048: old= 606300 + 92.1*reloc, lut= 574774 + 51.6*reloc (ns)
skip-relocs: buffers= 1: old= 1583 + 15.6*reloc, lut= 1516 + 14.5*reloc (ns)
skip-relocs: buffers= 2: old= 1621 + 15.6*reloc, lut= 1603 + 14.5*reloc (ns)
skip-relocs: buffers= 4: old= 1791 + 15.6*reloc, lut= 1777 + 14.5*reloc (ns)
skip-relocs: buffers= 8: old= 2009 + 15.6*reloc, lut= 2024 + 14.6*reloc (ns)
skip-relocs: buffers= 16: old= 2637 + 15.7*reloc, lut= 2564 + 14.6*reloc (ns)
skip-relocs: buffers= 32: old= 3835 + 15.8*reloc, lut= 3785 + 14.7*reloc (ns)
skip-relocs: buffers= 64: old= 6996 + 15.8*reloc, lut= 6681 + 14.7*reloc (ns)
skip-relocs: buffers= 128: old= 14333 + 16.4*reloc, lut= 13560 + 15.2*reloc (ns)
skip-relocs: buffers= 256: old= 28092 + 17.7*reloc, lut= 26759 + 16.2*reloc (ns)
skip-relocs: buffers= 512: old= 70885 + 25.2*reloc, lut= 66713 + 17.9*reloc (ns)
skip-relocs: buffers=1024: old= 158520 + 35.2*reloc, lut= 150828 + 20.1*reloc (ns)
skip-relocs: buffers=2048: old= 314208 + 54.3*reloc, lut= 298343 + 22.1*reloc (ns)
no-relocs: buffers= 1: old= 1533 + 5.2*reloc, lut= 1498 + 4.9*reloc (ns)
no-relocs: buffers= 2: old= 1518 + 5.2*reloc, lut= 1505 + 4.9*reloc (ns)
no-relocs: buffers= 4: old= 1647 + 5.2*reloc, lut= 1593 + 4.9*reloc (ns)
no-relocs: buffers= 8: old= 1882 + 5.3*reloc, lut= 1874 + 5.0*reloc (ns)
no-relocs: buffers= 16: old= 2399 + 5.3*reloc, lut= 2341 + 5.0*reloc (ns)
no-relocs: buffers= 32: old= 3638 + 5.3*reloc, lut= 3554 + 5.0*reloc (ns)
no-relocs: buffers= 64: old= 6622 + 5.3*reloc, lut= 6308 + 5.1*reloc (ns)
no-relocs: buffers= 128: old= 13584 + 5.3*reloc, lut= 12872 + 5.1*reloc (ns)
no-relocs: buffers= 256: old= 26519 + 5.8*reloc, lut= 25234 + 5.5*reloc (ns)
no-relocs: buffers= 512: old= 67128 + 5.4*reloc, lut= 63054 + 5.2*reloc (ns)
no-relocs: buffers=1024: old= 146705 + 5.2*reloc, lut= 139020 + 5.1*reloc (ns)
no-relocs: buffers=2048: old= 290319 + 5.4*reloc, lut= 274705 + 5.4*reloc (ns)vma(execbuffer):
relocation: buffers= 1: old= 21922 + 34.6*reloc, lut= 21510 + 34.0*reloc (ns)
relocation: buffers= 2: old= 16851 + 37.4*reloc, lut= 17123 + 35.4*reloc (ns)
relocation: buffers= 4: old= 13234 + 37.8*reloc, lut= 13436 + 36.9*reloc (ns)
relocation: buffers= 8: old= 6549 + 40.8*reloc, lut= 6512 + 39.8*reloc (ns)
relocation: buffers= 16: old= 5012 + 41.8*reloc, lut= 4883 + 41.0*reloc (ns)
relocation: buffers= 32: old= 8591 + 42.2*reloc, lut= 8377 + 41.1*reloc (ns)
relocation: buffers= 64: old= 16051 + 42.8*reloc, lut= 15658 + 41.7*reloc (ns)
relocation: buffers= 128: old= 33397 + 44.5*reloc, lut= 32705 + 43.3*reloc (ns)
relocation: buffers= 256: old= 68012 + 46.8*reloc, lut= 66904 + 45.5*reloc (ns)
relocation: buffers= 512: old= 160162 + 56.4*reloc, lut= 155586 + 49.1*reloc (ns)
relocation: buffers=1024: old= 348728 + 71.8*reloc, lut= 338113 + 55.1*reloc (ns)
relocation: buffers=2048: old= 699331 + 98.7*reloc, lut= 675969 + 62.2*reloc (ns)
skip-relocs: buffers= 1: old= 1642 + 16.5*reloc, lut= 1588 + 15.6*reloc (ns)
skip-relocs: buffers= 2: old= 1676 + 16.4*reloc, lut= 1663 + 15.6*reloc (ns)
skip-relocs: buffers= 4: old= 1926 + 16.4*reloc, lut= 1891 + 15.6*reloc (ns)
skip-relocs: buffers= 8: old= 2218 + 16.6*reloc, lut= 2212 + 15.7*reloc (ns)
skip-relocs: buffers= 16: old= 2933 + 16.6*reloc, lut= 2880 + 15.7*reloc (ns)
skip-relocs: buffers= 32: old= 4594 + 16.6*reloc, lut= 4523 + 15.8*reloc (ns)
skip-relocs: buffers= 64: old= 8414 + 16.8*reloc, lut= 8210 + 15.9*reloc (ns)
skip-relocs: buffers= 128: old= 17429 + 17.9*reloc, lut= 17062 + 16.8*reloc (ns)
skip-relocs: buffers= 256: old= 34794 + 19.8*reloc, lut= 34144 + 18.4*reloc (ns)
skip-relocs: buffers= 512: old= 82287 + 27.6*reloc, lut= 80002 + 20.8*reloc (ns)
skip-relocs: buffers=1024: old= 179851 + 38.0*reloc, lut= 174574 + 23.9*reloc (ns)
skip-relocs: buffers=2048: old= 361511 + 57.2*reloc, lut= 350132 + 26.8*reloc (ns)
no-relocs: buffers= 1: old= 1581 + 5.2*reloc, lut= 1579 + 4.9*reloc (ns)
no-relocs: buffers= 2: old= 1609 + 5.2*reloc, lut= 1572 + 4.9*reloc (ns)
no-relocs: buffers= 4: old= 1701 + 5.3*reloc, lut= 1685 + 4.9*reloc (ns)
no-relocs: buffers= 8: old= 2084 + 5.3*reloc, lut= 2033 + 5.0*reloc (ns)
no-relocs: buffers= 16: old= 2747 + 5.3*reloc, lut= 2686 + 5.0*reloc (ns)
no-relocs: buffers= 32: old= 4379 + 5.3*reloc, lut= 4285 + 5.0*reloc (ns)
no-relocs: buffers= 64: old= 8049 + 5.3*reloc, lut= 7850 + 5.1*reloc (ns)
no-relocs: buffers= 128: old= 16641 + 5.4*reloc, lut= 16301 + 5.2*reloc (ns)
no-relocs: buffers= 256: old= 33111 + 5.7*reloc, lut= 32539 + 5.5*reloc (ns)
no-relocs: buffers= 512: old= 79898 + 5.4*reloc, lut= 77517 + 5.2*reloc (ns)
no-relocs: buffers=1024: old= 172199 + 5.2*reloc, lut= 166907 + 5.1*reloc (ns)
no-relocs: buffers=2048: old= 345542 + 5.2*reloc, lut= 334300 + 5.3*reloc (ns)
Quoted 5 lines
So there is measurable degradation for the extra indirections, both for looking up the execbuffers and for performing the relocations. Though it doesn't merit anything more than a footnote in the changelog. -Chris
I'm sad I can't reproduce it. I think I amended the commit message already, I can do more if you want.
Signature · 1 line
Ben Widawsky, Intel Open Source Technology Center
Split up execbuf vma conversion
In reply to Ben WidawskyHi Ben,
For my own review benefit I've slashed your patch into pieces and split out 3 prep patches. Overall diff is unchanged with the exception of a now stale FIXME comment that I've killed. Still need to do the in-depth review of the last patch, but looks good thus far.
Cheers, Daniel
PATCH 1/4 drm/i915: s/obj->exec_list/obj->obj_exec_link
From: Ben Widawsky <[email protected]>
To convert the execbuf code over to use vmas natively we need to shuffle the exec_list a bit. This patch here just prepares things with the debugfs code, which also uses the old exec_list list_head, newly called obj_exec_link.
--- drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++------ drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem.c | 1 + 3 files changed, 9 insertions(+), 6 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +6 -6
@@ -195,9 +195,9 @@ static int obj_rank_by_stolen(void *priv, struct list_head *A, struct list_head *B) { struct drm_i915_gem_object *a = - container_of(A, struct drm_i915_gem_object, exec_list); + container_of(A, struct drm_i915_gem_object, obj_exec_link);struct drm_i915_gem_object *b = - container_of(B, struct drm_i915_gem_object, exec_list); + container_of(B, struct drm_i915_gem_object, obj_exec_link);return a->stolen->start - b->stolen->start; }
@@ -221,7 +221,7 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)if (obj->stolen ==NULL )continue ; -list_add (&obj->exec_list, &stolen); +list_add (&obj->obj_exec_link, &stolen); total_obj_size += obj->base.size; total_gtt_size +=i915_gem_obj_ggtt_size (obj);
@@ -231,7 +231,7 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)if (obj->stolen ==NULL )continue ; -list_add (&obj->exec_list, &stolen); +list_add (&obj->obj_exec_link, &stolen); total_obj_size += obj->base.size; count++;
@@ -239,11 +239,11 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)list_sort (NULL , &stolen, obj_rank_by_stolen);seq_puts (m,"Stolen:\n" );while (!list_empty (&stolen)) { - obj =list_first_entry (&stolen,typeof (*obj), exec_list); + obj =list_first_entry (&stolen,typeof (*obj), obj_exec_link);seq_puts (m," " );describe_obj (m, obj);seq_putc (m,'\n' ); -list_del_init (&obj->exec_list); +list_del_init (&obj->obj_exec_link); }mutex_unlock (&dev->struct_mutex);
drivers/gpu/drm/i915/i915_drv.h +2 -0
@@ -1312,6 +1312,8 @@ struct drm_i915_gem_object {struct list_head global_list;struct list_head ring_list; +/** Used in execbuf to temporarily hold a ref */ +struct list_head obj_exec_link;/** This object's place in the batchbuffer or on the eviction list */ struct list_head exec_list;
drivers/gpu/drm/i915/i915_gem.c +1 -0
@@ -3993,6 +3993,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,INIT_LIST_HEAD (&obj->global_list);INIT_LIST_HEAD (&obj->ring_list);INIT_LIST_HEAD (&obj->exec_list); +INIT_LIST_HEAD (&obj->obj_exec_link);INIT_LIST_HEAD (&obj->vma_list); obj->ops = ops;
Signature · 1 line
1.8.4.rc1
Re: [PATCH 1/4] drm/i915: s/obj->exec_list/obj->obj_exec_link
Quoted 6 lines · Daniel Vetter
From: Ben Widawsky <[email protected]>
To convert the execbuf code over to use vmas natively we need to shuffle the
exec_lista bit. This patch here just prepares things with the debugfs code, which also uses the oldexec_listlist_head, newly calledobj_exec_link.
Meh, patch subject is missing a "in debugfs" at the end. I'll fix this when merging.
-Daniel
Quoted 86 lines
Signed-off-by Ben Widawsky <[email protected]>[danvet: Split out from Ben's big patch.]Signed-off-by Daniel Vetter <[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++------ drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem.c | 1 + 3 files changed, 9 insertions(+), 6 deletions(-)drivers/gpu/drm/i915/i915_debugfs.c +6
-6@@ -195,9 +195,9 @@ static int obj_rank_by_stolen(void *priv, struct list_head *A, struct list_head *B) { struct drm_i915_gem_object *a = - container_of(A, struct drm_i915_gem_object, exec_list); + container_of(A, struct drm_i915_gem_object, obj_exec_link);struct drm_i915_gem_object *b = - container_of(B, struct drm_i915_gem_object, exec_list); + container_of(B, struct drm_i915_gem_object, obj_exec_link);return a->stolen->start - b->stolen->start; }@@ -221,7 +221,7 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)if (obj->stolen ==NULL )continue ; -list_add (&obj->exec_list, &stolen); +list_add (&obj->obj_exec_link, &stolen); total_obj_size += obj->base.size; total_gtt_size +=i915_gem_obj_ggtt_size (obj);@@ -231,7 +231,7 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)if (obj->stolen ==NULL )continue ; -list_add (&obj->exec_list, &stolen); +list_add (&obj->obj_exec_link, &stolen); total_obj_size += obj->base.size; count++;@@ -239,11 +239,11 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)list_sort (NULL , &stolen, obj_rank_by_stolen);seq_puts (m,"Stolen:\n" );while (!list_empty (&stolen)) { - obj =list_first_entry (&stolen,typeof (*obj), exec_list); + obj =list_first_entry (&stolen,typeof (*obj), obj_exec_link);seq_puts (m," " );describe_obj (m, obj);seq_putc (m,'\n' ); -list_del_init (&obj->exec_list); +list_del_init (&obj->obj_exec_link); }mutex_unlock (&dev->struct_mutex);drivers/gpu/drm/i915/i915_drv.h +2
-0@@ -1312,6 +1312,8 @@ struct drm_i915_gem_object {struct list_head global_list;struct list_head ring_list; +/** Used in execbuf to temporarily hold a ref */ +struct list_head obj_exec_link;/** This object's place in the batchbuffer or on the eviction list */ struct list_head exec_list;drivers/gpu/drm/i915/i915_gem.c +1
-0@@ -3993,6 +3993,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,INIT_LIST_HEAD (&obj->global_list);INIT_LIST_HEAD (&obj->ring_list);INIT_LIST_HEAD (&obj->exec_list); +INIT_LIST_HEAD (&obj->obj_exec_link);INIT_LIST_HEAD (&obj->vma_list); obj->ops = ops;Signature · 1 line
1.8.4.rc1
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 2/4 drm/i915: Switch eviction code to use vmas
In reply to Daniel VetterFrom: Ben Widawsky <[email protected]>
The execbuf wants to do relocations usings vmas, so we need a vma->exec_list. The eviction code also uses the old obj execbuf list for it's own book-keeping, but would really prefer to deal in vmas only. So switch it over to the new list.
Again this is just a prep patch for the big execbuf vma conversion.
--- drivers/gpu/drm/i915/i915_drv.h | 4 ++++ drivers/gpu/drm/i915/i915_gem.c | 1 + drivers/gpu/drm/i915/i915_gem_evict.c | 31 ++++++++++++++----------------- 3 files changed, 19 insertions(+), 17 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +4 -0
@@ -563,6 +563,10 @@ struct i915_vma {struct list_head mm_list;struct list_head vma_link;/* Link in the object's VMA list */ + +/** This vma's place in the batchbuffer or on the eviction list */ +struct list_head exec_list; + };struct i915_ctx_hang_stats {
drivers/gpu/drm/i915/i915_gem.c +1 -0
@@ -4133,6 +4133,7 @@ struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,INIT_LIST_HEAD (&vma->vma_link);INIT_LIST_HEAD (&vma->mm_list); +INIT_LIST_HEAD (&vma->exec_list); vma->vm = vm; vma->obj = obj;
drivers/gpu/drm/i915/i915_gem_evict.c +14 -17
@@ -37,7 +37,7 @@ mark_free(struct i915_vma *vma, struct list_head *unwind)if (vma->obj->pin_count)return false; -list_add (&vma->obj->exec_list, unwind); +list_add (&vma->exec_list, unwind);return drm_mm_scan_add_block (&vma->node); }
@@ -49,7 +49,6 @@ i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm,drm_i915_private_t *dev_priv = dev->dev_private;struct list_head eviction_list, unwind_list;struct i915_vma *vma; -struct drm_i915_gem_object *obj;int ret =0 ;trace_i915_gem_evict (dev, min_size, alignment, mappable);
@@ -104,14 +103,13 @@ i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm, none:/* Nothing found, clean up and bail out! */ while (!list_empty (&unwind_list)) { - obj =list_first_entry (&unwind_list, - struct drm_i915_gem_object, + vma =list_first_entry (&unwind_list, + struct i915_vma, exec_list); - vma =i915_gem_obj_to_vma (obj, vm); ret =drm_mm_scan_remove_block (&vma->node);BUG_ON (ret); -list_del_init (&obj->exec_list); +list_del_init (&vma->exec_list); }/* We expect the caller to unpin, evict all and try again, or give up.
@@ -125,28 +123,27 @@ found:* temporary list. */ INIT_LIST_HEAD (&eviction_list);while (!list_empty (&unwind_list)) { - obj =list_first_entry (&unwind_list, - struct drm_i915_gem_object, + vma =list_first_entry (&unwind_list, + struct i915_vma, exec_list); - vma =i915_gem_obj_to_vma (obj, vm);if (drm_mm_scan_remove_block (&vma->node)) { -list_move (&obj->exec_list, &eviction_list); -drm_gem_object_reference (&obj->base); +list_move (&vma->exec_list, &eviction_list); +drm_gem_object_reference (&vma->obj->base);continue ; } -list_del_init (&obj->exec_list); +list_del_init (&vma->exec_list); }/* Unbinding will emit any required flushes */ while (!list_empty (&eviction_list)) { - obj =list_first_entry (&eviction_list, - struct drm_i915_gem_object, + vma =list_first_entry (&eviction_list, + struct i915_vma, exec_list);if (ret ==0 ) - ret =i915_vma_unbind (i915_gem_obj_to_vma (obj, vm)); + ret =i915_vma_unbind (vma); -list_del_init (&obj->exec_list); -drm_gem_object_unreference (&obj->base); +list_del_init (&vma->exec_list); +drm_gem_object_unreference (&vma->obj->base); }return ret;
Signature · 1 line
1.8.4.rc1
PATCH 3/4 drm/i915: prepare bind_to_vm for preallocated vma
In reply to Daniel VetterFrom: Ben Widawsky <[email protected]>
In the new execbuf code we want to track buffers using the vmas even before they're all properly mapped. Which means that bind_to_vm needs to deal with buffers which have preallocated vmas which aren't yet bound.
This patch implements this prep work and adjusts our WARN/BUG checks.
[danvet: Split out from Ben's big execbuf patch. Also move one BUG back to its original place to deflate the diff a notch.]
--- drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +3 -0
@@ -1914,6 +1914,9 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, struct i915_address_space *vm);struct i915_vma *i915_gem_obj_to_vma (struct drm_i915_gem_object *obj,struct i915_address_space *vm); +struct i915_vma * +i915_gem_obj_lookup_or_create_vma (struct drm_i915_gem_object *obj, +struct i915_address_space *vm);/* Some GGTT VM helpers */ #define obj_to_ggtt (obj) \ (&((struct drm_i915_private *)(obj)->base.dev->dev_private)->gtt.base)
drivers/gpu/drm/i915/i915_gem.c +17 -6
@@ -3124,9 +3124,6 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,struct i915_vma *vma;int ret; -if (WARN_ON (!list_empty (&obj->vma_list))) -return -EBUSY ; - fence_size =i915_gem_get_gtt_size (dev, obj->base.size, obj->tiling_mode);
@@ -3165,16 +3162,17 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,i915_gem_object_pin_pages (obj); -/* FIXME: For now we only ever use 1 VMA per object */ BUG_ON (!i915_is_ggtt (vm)); -WARN_ON (!list_empty (&obj->vma_list)); - vma =i915_gem_vma_create (obj, vm); + vma =i915_gem_obj_lookup_or_create_vma (obj, vm);if (IS_ERR (vma)) { ret =PTR_ERR (vma); goto err_unpin; } +/* For now we only ever use 1 vma per object */ +WARN_ON (!list_is_singular (&obj->vma_list)); + search_free: ret = drm_mm_insert_node_in_range_generic(&vm->mm, &vma->node, size, alignment,
@@ -4883,3 +4881,16 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,return NULL ; } + +struct i915_vma * +i915_gem_obj_lookup_or_create_vma (struct drm_i915_gem_object *obj, +struct i915_address_space *vm) +{ +struct i915_vma *vma; + + vma =i915_gem_obj_to_vma (obj, vm); +if (!vma) + vma =i915_gem_vma_create (obj, vm); + +return vma; +}
Signature · 1 line
1.8.4.rc1
PATCH 4/4 drm/i915: Convert execbuf code to use vmas
In reply to Daniel VetterFrom: Ben Widawsky <[email protected]>
In order to transition more of our code over to using a VMA instead of an <OBJ, VM> pair - we must have the vma accessible at execbuf time. Up until now, we've only had a VMA when actually binding an object.
The previous patch helped handle the distinction on bound vs. unbound. This patch will help us catch leaks, and other issues before we actually shuffle a bunch of stuff around.
This attempts to convert all the execbuf code to speak in vmas. Since the execbuf code is very self contained it was a nice isolated conversion.
The meat of the code is about turning eb_objects into eb_vma, and then wiring up the rest of the code to use vmas instead of obj, vm pairs.
Unfortunately, to do this, we must move the exec_list link from the obj structure. This list is reused in the eviction code, so we must also modify the eviction code to make this work.
WARNING: This patch makes an already hotly profiled path slower. The cost is unavoidable. In reply to this mail, I will attach the extra data.
v2: Release table lock early, and two a 2 phase vma lookup to avoid having to use a GFP_ATOMIC. (Chris)
v3: s/obj_exec_list/obj_exec_link/
Updates to address
commit 6d2b888569d366beb4be72cacfde41adee2c25e1
Author: Chris Wilson <[email protected]> Date: Wed Aug 7 18:30:54 2013 +0100
drm/i915: List objects allocated from stolen memory in debugfs
v4: Use obj = vma->obj for neatness in some places (Chris) need_reloc_mappable() should return false if ppgtt (Chris)
[danvet: Split out prep patches. Also remove a FIXME comment which is now taken care of.]
--- drivers/gpu/drm/i915/i915_drv.h | 16 +- drivers/gpu/drm/i915/i915_gem.c | 1 - drivers/gpu/drm/i915/i915_gem_execbuffer.c | 320 ++++++++++++++++------------- 3 files changed, 183 insertions(+), 154 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +7 -9
@@ -567,6 +567,13 @@ struct i915_vma {/** This vma's place in the batchbuffer or on the eviction list */ struct list_head exec_list; +/** +* Used for performing relocations during execbuffer insertion. +*/ +struct hlist_node exec_node; +unsigned long exec_handle; +struct drm_i915_gem_exec_object2 *exec_entry; + };struct i915_ctx_hang_stats {
@@ -1318,8 +1325,6 @@ struct drm_i915_gem_object {struct list_head ring_list;/** Used in execbuf to temporarily hold a ref */ struct list_head obj_exec_link; -/** This object's place in the batchbuffer or on the eviction list */ -struct list_head exec_list;/** * This is set if the object is on the active lists (has pending
@@ -1405,13 +1410,6 @@ struct drm_i915_gem_object {void *dma_buf_vmapping;int vmapping_count; -/** -* Used for performing relocations during execbuffer insertion. -*/ -struct hlist_node exec_node; -unsigned long exec_handle; -struct drm_i915_gem_exec_object2 *exec_entry; -struct intel_ring_buffer *ring;/** Breadcrumb of last rendering to the buffer. */
drivers/gpu/drm/i915/i915_gem.c +0 -1
@@ -3990,7 +3990,6 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, {INIT_LIST_HEAD (&obj->global_list);INIT_LIST_HEAD (&obj->ring_list); -INIT_LIST_HEAD (&obj->exec_list);INIT_LIST_HEAD (&obj->obj_exec_link);INIT_LIST_HEAD (&obj->vma_list);
drivers/gpu/drm/i915/i915_gem_execbuffer.c +176 -144
650 lines not on this page · raw message
Signature · 1 line
1.8.4.rc1
PATCH 1/1 drm/i915: inline vma_create into lookup_or_create_vma
In the execbuf code we don't clean up any vmas which ended up not getting bound for code simplicity. To make sure that we don't end up creating multiple vma for the same vm kill the somewhat dangerous vma_create function and inline it into lookup_or_create.
This is just a safety measure to prevent surprises in the future.
Also update the somewhat confused comment in the execbuf code and clarify what kind of magic is going on with a new one.
---
That's the only concern I could come up with when reading the execbuf vma conversion patch. So looks good and I'll slurp it all in as soon as some more head scratching is done for the very first patch in this series about the vma_unbind fix to only call vma_destroy if the vma isn't bound.
-Daniel
--- drivers/gpu/drm/i915/i915_drv.h | 2 -- drivers/gpu/drm/i915/i915_gem.c | 41 +++++++++++++----------------- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++--- drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- 4 files changed, 26 insertions(+), 30 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +0 -2
@@ -1748,8 +1748,6 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,struct drm_i915_gem_object *i915_gem_alloc_object (struct drm_device *dev,size_t size);void i915_gem_free_object (struct drm_gem_object *obj); -struct i915_vma *i915_gem_vma_create (struct drm_i915_gem_object *obj, -struct i915_address_space *vm);void i915_gem_vma_destroy (struct i915_vma *vma); int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
drivers/gpu/drm/i915/i915_gem.c +17 -24
53 lines not on this page · raw message
drivers/gpu/drm/i915/i915_gem_execbuffer.c +8 -3
@@ -123,11 +123,16 @@ eb_lookup_vmas(struct eb_vmas *eb,list_for_each_entry (obj, &objects, obj_exec_link) {struct i915_vma *vma; +/* +* NOTE: We can leak any vmas created here when something fails +* later on. But that's no issue since vma_unbind can deal with +* vmas which are not actually bound. And since only +* lookup_or_create exists as an interface to get at the vma +* from the (obj, vm) we don't run the risk of creating +* duplicated vmas for the same vm. +*/ vma =i915_gem_obj_lookup_or_create_vma (obj, vm);if (IS_ERR (vma)) { -/* XXX: We don't need an error path fro vma because if -* the vma was created just for this execbuf, object -* unreference should kill it off.*/ DRM_DEBUG ("Failed to lookup VMA\n" ); ret =PTR_ERR (vma); goto out;
drivers/gpu/drm/i915/i915_gem_stolen.c +1 -1
@@ -375,7 +375,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,if (gtt_offset ==I915_GTT_OFFSET_NONE )return obj; - vma =i915_gem_vma_create (obj, ggtt); + vma =i915_gem_obj_lookup_or_create_vma (obj, ggtt);if (IS_ERR (vma)) { ret =PTR_ERR (vma); goto err_out;
Signature · 1 line
1.8.4.rc1
Re: [PATCH] drm/i915: inline vma_create into lookup_or_create_vma
Quoted 4 lines · Daniel Vetter
In the execbuf code we don't clean up any vmas which ended up not getting bound for code simplicity. To make sure that we don't end up creating multiple vma for the same vm kill the somewhat dangerous
vma_createfunction and inline it intolookup_or_create.
Just make it static. It's a nice little standalone function that helps the reader.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH] drm/i915: inline vma_create into lookup_or_create_vma
Quoted 8 lines · Chris Wilson
Quoted 4 lines · Daniel Vetter
In the execbuf code we don't clean up any vmas which ended up not getting bound for code simplicity. To make sure that we don't end up creating multiple vma for the same vm kill the somewhat dangerous
vma_createfunction and inline it intolookup_or_create.Just make it static. It's a nice little standalone function that helps the reader.
I'm afraid that someone will use it eventually. I'll smash a __ prefix onto it to make it clear that people better don't touch it. -Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 1/1 drm/i915: inline vma_create into lookup_or_create_vma
In reply to Chris WilsonIn the execbuf code we don't clean up any vmas which ended up not getting bound for code simplicity. To make sure that we don't end up creating multiple vma for the same vm kill the somewhat dangerous vma_create function and inline it into lookup_or_create.
This is just a safety measure to prevent surprises in the future.
Also update the somewhat confused comment in the execbuf code and clarify what kind of magic is going on with a new one.
v2: Keep the function separate as requested by Chris. But give it a __ prefix for paranoia and move it tighter together with the other vma stuff.
--- drivers/gpu/drm/i915/i915_drv.h | 2 -- drivers/gpu/drm/i915/i915_gem.c | 50 +++++++++++++++--------------- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++-- drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- 4 files changed, 34 insertions(+), 31 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +0 -2
@@ -1748,8 +1748,6 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,struct drm_i915_gem_object *i915_gem_alloc_object (struct drm_device *dev,size_t size);void i915_gem_free_object (struct drm_gem_object *obj); -struct i915_vma *i915_gem_vma_create (struct drm_i915_gem_object *obj, -struct i915_address_space *vm);void i915_gem_vma_destroy (struct i915_vma *vma); int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
drivers/gpu/drm/i915/i915_gem.c +25 -25
67 lines not on this page · raw message
drivers/gpu/drm/i915/i915_gem_execbuffer.c +8 -3
@@ -123,11 +123,16 @@ eb_lookup_vmas(struct eb_vmas *eb,list_for_each_entry (obj, &objects, obj_exec_link) {struct i915_vma *vma; +/* +* NOTE: We can leak any vmas created here when something fails +* later on. But that's no issue since vma_unbind can deal with +* vmas which are not actually bound. And since only +* lookup_or_create exists as an interface to get at the vma +* from the (obj, vm) we don't run the risk of creating +* duplicated vmas for the same vm. +*/ vma =i915_gem_obj_lookup_or_create_vma (obj, vm);if (IS_ERR (vma)) { -/* XXX: We don't need an error path fro vma because if -* the vma was created just for this execbuf, object -* unreference should kill it off.*/ DRM_DEBUG ("Failed to lookup VMA\n" ); ret =PTR_ERR (vma); goto out;
drivers/gpu/drm/i915/i915_gem_stolen.c +1 -1
@@ -375,7 +375,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,if (gtt_offset ==I915_GTT_OFFSET_NONE )return obj; - vma =i915_gem_vma_create (obj, ggtt); + vma =i915_gem_obj_lookup_or_create_vma (obj, ggtt);if (IS_ERR (vma)) { ret =PTR_ERR (vma); goto err_out;
Signature · 1 line
1.8.4.rc1
Re: [PATCH] drm/i915: inline vma_create into lookup_or_create_vma
In reply to Daniel VetterQuoted 12 lines · Daniel Vetter
In the execbuf code we don't clean up any vmas which ended up not getting bound for code simplicity. To make sure that we don't end up creating multiple vma for the same vm kill the somewhat dangerous
vma_createfunction and inline it intolookup_or_create.This is just a safety measure to prevent surprises in the future.
Also update the somewhat confused comment in the execbuf code and clarify what kind of magic is going on with a new one.
Cc Ben Widawsky <[email protected]>Signed-off-by Daniel Vetter <[email protected]>
Quoted 7 lines
---
That's the only concern I could come up with when reading the execbuf vma conversion patch. So looks good and I'll slurp it all in as soon as some more head scratching is done for the very first patch in this series about the
vma_unbindfix to only callvma_destroyif the vma isn't bound.
One thing I've noticed but forgot to mention here is that the reloc code still uses obj_ggtt_size/offset. I guess that will be fixed later on? -Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH] drm/i915: inline vma_create into lookup_or_create_vma
Quoted 21 lines · Daniel Vetter
Quoted 12 lines · Daniel Vetter
In the execbuf code we don't clean up any vmas which ended up not getting bound for code simplicity. To make sure that we don't end up creating multiple vma for the same vm kill the somewhat dangerous
vma_createfunction and inline it intolookup_or_create.This is just a safety measure to prevent surprises in the future.
Also update the somewhat confused comment in the execbuf code and clarify what kind of magic is going on with a new one.
Cc Ben Widawsky <[email protected]>Signed-off-by Daniel Vetter <[email protected]>Quoted 7 lines
---
That's the only concern I could come up with when reading the execbuf vma conversion patch. So looks good and I'll slurp it all in as soon as some more head scratching is done for the very first patch in this series about the
vma_unbindfix to only callvma_destroyif the vma isn't bound.
Quoted 3 lines
One thing I've noticed but forgot to mention here is that the reloc code still uses obj_ggtt_size/offset. I guess that will be fixed later on? -Daniel
Yes.
Signature · 1 line
Ben Widawsky, Intel Open Source Technology Center
Re: [PATCH 4/4] drm/i915: Convert execbuf code to use vmas
In reply to Daniel Vetterdrivers/gpu/drm/i915/i915_gem_execbuffer.c eb_create(struct drm_i915_gem_execbuffer2 *args) · line 130
Quoted 7 lines · Daniel Vetter
From: Ben Widawsky <[email protected]>
drivers/gpu/drm/i915/i915_gem_execbuffer.c +5
-0+ vma =i915_gem_obj_lookup_or_create_vma (obj, vm); +if (IS_ERR (vma)) { +/* XXX: We don't need an error path fro vma because if +* the vma was created just for this execbuf, object +* unreference should kill it off.*/
The XXX part here is that before the object is finally unreffed we have a dangling vma and so we need to take care else way not to assume that a vma is implicitly bound.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Quoted 11 lines · Ben Widawsky
VMAs can be created and not bound. One may think of it as lazy cleanup, and safely gloss over the conditions which manufacture it. In either case, when the object backing the i915 vma is destroyed, we must cleanup the vma without stumbling into a bunch of pitfalls that assume the vma is bound.
NOTE: I was pretty certain the above condition could only happen when we introduced the use of VMAs being looked up at execbuf, and already existing. Paulo has hit this though, so I must be missing something. As I believe the patch is correct anyway, therefore I won't scratch my head too hard.
If we end up calling evict_everything from i915_gem_object_bind_to_vm then we'll hit this. One more reason for a testcase here ;-) I'll amend the commit message and merge this. I've also applied a tiny bikeshed I've created while reviewing existing vma_create/destroy callsites. -Daniel
Quoted 40 lines
v2: use goto destroy as a compromise (Chris)
Cc Chris Wilson <[email protected]>Cc Paulo Zanoni <[email protected]>Signed-off-by Ben Widawsky <[email protected]>--- drivers/gpu/drm/i915/i915_gem.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/gpu/drm/i915/i915_gem.c +5
-0@@ -2606,6 +2606,9 @@ int i915_vma_unbind(struct i915_vma *vma)if (list_empty (&vma->vma_link))return 0 ; +if (!drm_mm_node_allocated (&vma->node)) + goto destroy; +if (obj->pin_count)return -EBUSY ;@@ -2643,6 +2646,8 @@ int i915_vma_unbind(struct i915_vma *vma) obj->map_and_fenceable = true;drm_mm_remove_node (&vma->node); + +destroy:i915_gem_vma_destroy (vma);/* Since the unbound list is global, only move to that list if Signature · 6 lines
1.8.3.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 13 lines · Daniel Vetter
Quoted 11 lines · Ben Widawsky
VMAs can be created and not bound. One may think of it as lazy cleanup, and safely gloss over the conditions which manufacture it. In either case, when the object backing the i915 vma is destroyed, we must cleanup the vma without stumbling into a bunch of pitfalls that assume the vma is bound.
NOTE: I was pretty certain the above condition could only happen when we introduced the use of VMAs being looked up at execbuf, and already existing. Paulo has hit this though, so I must be missing something. As I believe the patch is correct anyway, therefore I won't scratch my head too hard.
Quoted 4 lines
If we end up calling
evict_everythingfromi915_gem_object_bind_to_vmthen we'll hit this. One more reason for a testcase here ;-) I'll amend the commit message and merge this. I've also applied a tiny bikeshed I've created while reviewing existing vma_create/destroy callsites.
Actually evict_everything isn't in the callpath, and there's no memory allocation where the shrinker might play havoc. Furthermore the pages are pinned so the shrinker shouldn't be able to sneak in at all. This is a bit unsettling, I need to think more about this.
I'll wait with merging this for now.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 19 lines · Daniel Vetter
Quoted 13 lines · Daniel Vetter
Quoted 11 lines · Ben Widawsky
VMAs can be created and not bound. One may think of it as lazy cleanup, and safely gloss over the conditions which manufacture it. In either case, when the object backing the i915 vma is destroyed, we must cleanup the vma without stumbling into a bunch of pitfalls that assume the vma is bound.
NOTE: I was pretty certain the above condition could only happen when we introduced the use of VMAs being looked up at execbuf, and already existing. Paulo has hit this though, so I must be missing something. As I believe the patch is correct anyway, therefore I won't scratch my head too hard.
Quoted 4 lines
If we end up calling
evict_everythingfromi915_gem_object_bind_to_vmthen we'll hit this. One more reason for a testcase here ;-) I'll amend the commit message and merge this. I've also applied a tiny bikeshed I've created while reviewing existing vma_create/destroy callsites.
Quoted 6 lines
Actually
evict_everythingisn't in the callpath, and there's no memory allocation where the shrinker might play havoc. Furthermore the pages are pinned so the shrinker shouldn't be able to sneak in at all. This is a bit unsettling, I need to think more about this.I'll wait with merging this for now.
Ok, I've merged the entire pile. I think now's the time to throw a bit of igt on top to exercise the corner cases here ...
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 25 lines · Daniel Vetter
Quoted 24 lines · Daniel Vetter
Quoted 13 lines · Daniel Vetter
Quoted 11 lines · Ben Widawsky
VMAs can be created and not bound. One may think of it as lazy cleanup, and safely gloss over the conditions which manufacture it. In either case, when the object backing the i915 vma is destroyed, we must cleanup the vma without stumbling into a bunch of pitfalls that assume the vma is bound.
NOTE: I was pretty certain the above condition could only happen when we introduced the use of VMAs being looked up at execbuf, and already existing. Paulo has hit this though, so I must be missing something. As I believe the patch is correct anyway, therefore I won't scratch my head too hard.
Quoted 4 lines
If we end up calling
evict_everythingfromi915_gem_object_bind_to_vmthen we'll hit this. One more reason for a testcase here ;-) I'll amend the commit message and merge this. I've also applied a tiny bikeshed I've created while reviewing existing vma_create/destroy callsites.Actually
evict_everythingisn't in the callpath, and there's no memory allocation where the shrinker might play havoc. Furthermore the pages are pinned so the shrinker shouldn't be able to sneak in at all. This is a bit unsettling, I need to think more about this.
Quoted 5 lines
Quoted 1 line
I'll wait with merging this for now.
Ok, I've merged the entire pile. I think now's the time to throw a bit of igt on top to exercise the corner cases here ...
-Daniel
Was that a specific request for me to do something, I missed the message if so? When should I rework the remaining patches and resend?
Signature · 1 line
Ben Widawsky, Intel Open Source Technology Center
Quoted 12 lines · Ben Widawsky
VMAs can be created and not bound. One may think of it as lazy cleanup, and safely gloss over the conditions which manufacture it. In either case, when the object backing the i915 vma is destroyed, we must cleanup the vma without stumbling into a bunch of pitfalls that assume the vma is bound.
NOTE: I was pretty certain the above condition could only happen when we introduced the use of VMAs being looked up at execbuf, and already existing. Paulo has hit this though, so I must be missing something. As I believe the patch is correct anyway, therefore I won't scratch my head too hard.
Quoted 5 lines
v2: use goto destroy as a compromise (Chris)
Cc Chris Wilson <[email protected]>Cc Paulo Zanoni <[email protected]>Signed-off-by Ben Widawsky <[email protected]>
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre