On Tue, Jul 02, 2013 at 09:26:45AM +0200, Daniel Vetter wrote: > On Mon, Jul 01, 2013 at 03:56:50PM -0700, Ben Widawsky wrote: > > On Sun, Jun 30, 2013 at 05:38:16PM +0200, Daniel Vetter wrote: > > > On Thu, Jun 27, 2013 at 04:30:27PM -0700, Ben Widawsky wrote: > > > > for file in `ls drivers/gpu/drm/i915/*.c` ; do sed -i > > > > "s/dev_priv->mm.inactive_list/i915_gtt_mm-\>inactive_list/" $file; done > > > > for file in `ls drivers/gpu/drm/i915/*.c` ; do sed -i > > > > "s/dev_priv->mm.active_list/i915_gtt_mm-\>active_list/" $file; done > > > > > > > > I've also opted to move the comments out of line a bit so one can get a > > > > better picture of what the various lists do. > > > > > > Bikeshed: That makes you now inconsistent with all the other in-detail > > > structure memeber comments we have. And I don't see how it looks better, > > > so I'd vote to keep things as-is with per-member comments. > > > > > Initially I moved all the comments (in the original mm destruction I > > did). > > I mean to keep the per-struct-member comments right next to each > individual declaration.
I meant, in the initial version I had a big blob where I wrote about all the tracking, and what each list did. It actually was pretty cool, but at that time I was trying to track [un]bound with the vm. > > > > > v2: Leave the bound list as a global one. (Chris, indirectly) > > > > > > > > CC: Chris Wilson <[email protected]> > > > > Signed-off-by: Ben Widawsky <[email protected]> > > > > > > The real comment though is on the commit message, it fails to explain why > > > we want to move the active/inactive lists from mm/obj to the address > > > space/vma pair. I think I understand, but this should be explained more > > > in-depth. > > > > > > I think in the first commit which starts moving those lists and execution > > > tracking state you should also mention why some of the state > > > (bound/unbound lists e.g.) are not moved. > > > > > > Cheers, Daniel > > > > Can I use, "because Chris told me to"? :p > > I think some high-level explanation should be doable ;-) E.g. when moving > the lists around explain that the active/inactive stuff is used by > eviction when we run out of address space, so needs to be per-vma and > per-address space. Bound/unbound otoh is used by the shrinker which only > cares about the amount of memory used and not one bit about in which > address space this memory is all used in. Of course to actual kick out an > object we need to unbind it from every address space, but for that we have > the per-object list of vmas. > -Daniel I was being facetious, but thanks for writing the commit message for me :D > > > > > > > > > > --- > > > > drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++---- > > > > drivers/gpu/drm/i915/i915_drv.h | 49 > > > > ++++++++++++++-------------------- > > > > drivers/gpu/drm/i915/i915_gem.c | 24 +++++++---------- > > > > drivers/gpu/drm/i915/i915_gem_debug.c | 2 +- > > > > drivers/gpu/drm/i915/i915_gem_evict.c | 10 +++---- > > > > drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- > > > > drivers/gpu/drm/i915/i915_irq.c | 6 ++--- > > > > 7 files changed, 46 insertions(+), 58 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > > index f3c76ab..a0babc7 100644 > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > > @@ -158,11 +158,11 @@ static int i915_gem_object_list_info(struct > > > > seq_file *m, void *data) > > > > switch (list) { > > > > case ACTIVE_LIST: > > > > seq_printf(m, "Active:\n"); > > > > - head = &dev_priv->mm.active_list; > > > > + head = &i915_gtt_vm->active_list; > > > > break; > > > > case INACTIVE_LIST: > > > > seq_printf(m, "Inactive:\n"); > > > > - head = &dev_priv->mm.inactive_list; > > > > + head = &i915_gtt_vm->inactive_list; > > > > break; > > > > default: > > > > mutex_unlock(&dev->struct_mutex); > > > > @@ -247,12 +247,12 @@ static int i915_gem_object_info(struct seq_file > > > > *m, void* data) > > > > count, mappable_count, size, mappable_size); > > > > > > > > size = count = mappable_size = mappable_count = 0; > > > > - count_objects(&dev_priv->mm.active_list, mm_list); > > > > + count_objects(&i915_gtt_vm->active_list, mm_list); > > > > seq_printf(m, " %u [%u] active objects, %zu [%zu] bytes\n", > > > > count, mappable_count, size, mappable_size); > > > > > > > > size = count = mappable_size = mappable_count = 0; > > > > - count_objects(&dev_priv->mm.inactive_list, mm_list); > > > > + count_objects(&i915_gtt_vm->inactive_list, mm_list); > > > > seq_printf(m, " %u [%u] inactive objects, %zu [%zu] bytes\n", > > > > count, mappable_count, size, mappable_size); > > > > > > > > @@ -1977,7 +1977,8 @@ i915_drop_caches_set(void *data, u64 val) > > > > i915_gem_retire_requests(dev); > > > > > > > > if (val & DROP_BOUND) { > > > > - list_for_each_entry_safe(obj, next, > > > > &dev_priv->mm.inactive_list, mm_list) > > > > + list_for_each_entry_safe(obj, next, > > > > &i915_gtt_vm->inactive_list, > > > > + mm_list) > > > > if (obj->pin_count == 0) { > > > > ret = i915_gem_object_unbind(obj); > > > > if (ret) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > index e65cf57..0553410 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -448,6 +448,22 @@ struct i915_address_space { > > > > unsigned long start; /* Start offset always 0 for > > > > dri2 */ > > > > size_t total; /* size addr space maps (ex. 2GB for > > > > ggtt) */ > > > > > > > > +/* We use many types of lists for object tracking: > > > > + * active_list: List of objects currently involved in rendering. > > > > + * Includes buffers having the contents of their GPU caches > > > > flushed, not > > > > + * necessarily primitives. last_rendering_seqno represents when the > > > > + * rendering involved will be completed. A reference is held on > > > > the buffer > > > > + * while on this list. > > > > + * inactive_list: LRU list of objects which are not in the ringbuffer > > > > + * objects are ready to unbind but are still mapped. > > > > + * last_rendering_seqno is 0 while an object is in this list. > > > > + * A reference is not held on the buffer while on this list, > > > > + * as merely being GTT-bound shouldn't prevent its being > > > > + * freed, and we'll pull it off the list in the free path. > > > > + */ > > > > + struct list_head active_list; > > > > + struct list_head inactive_list; > > > > + > > > > struct { > > > > dma_addr_t addr; > > > > struct page *page; > > > > @@ -835,42 +851,17 @@ struct intel_l3_parity { > > > > }; > > > > > > > > struct i915_gem_mm { > > > > - /** List of all objects in gtt_space. Used to restore gtt > > > > - * mappings on resume */ > > > > - struct list_head bound_list; > > > > /** > > > > - * List of objects which are not bound to the GTT (thus > > > > - * are idle and not used by the GPU) but still have > > > > - * (presumably uncached) pages still attached. > > > > + * Lists of objects which are [not] bound to a VM. Unbound > > > > objects are > > > > + * idle are idle but still have (presumably uncached) pages > > > > still > > > > + * attached. > > > > */ > > > > + struct list_head bound_list; > > > > struct list_head unbound_list; > > > > > > > > struct shrinker inactive_shrinker; > > > > bool shrinker_no_lock_stealing; > > > > > > > > - /** > > > > - * List of objects currently involved in rendering. > > > > - * > > > > - * Includes buffers having the contents of their GPU caches > > > > - * flushed, not necessarily primitives. last_rendering_seqno > > > > - * represents when the rendering involved will be completed. > > > > - * > > > > - * A reference is held on the buffer while on this list. > > > > - */ > > > > - struct list_head active_list; > > > > - > > > > - /** > > > > - * LRU list of objects which are not in the ringbuffer and > > > > - * are ready to unbind, but are still in the GTT. > > > > - * > > > > - * last_rendering_seqno is 0 while an object is in this list. > > > > - * > > > > - * A reference is not held on the buffer while on this list, > > > > - * as merely being GTT-bound shouldn't prevent its being > > > > - * freed, and we'll pull it off the list in the free path. > > > > - */ > > > > - struct list_head inactive_list; > > > > - > > > > /** LRU list of objects with fence regs on them. */ > > > > struct list_head fence_list; > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > > b/drivers/gpu/drm/i915/i915_gem.c > > > > index 608b6b5..7da06df 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > @@ -1706,7 +1706,7 @@ __i915_gem_shrink(struct drm_i915_private > > > > *dev_priv, long target, > > > > } > > > > > > > > list_for_each_entry_safe(obj, next, > > > > - &dev_priv->mm.inactive_list, > > > > + &i915_gtt_vm->inactive_list, > > > > mm_list) { > > > > if ((i915_gem_object_is_purgeable(obj) || > > > > !purgeable_only) && > > > > i915_gem_object_unbind(obj) == 0 && > > > > @@ -1881,7 +1881,7 @@ i915_gem_object_move_to_active(struct > > > > drm_i915_gem_object *obj, > > > > } > > > > > > > > /* Move from whatever list we were on to the tail of execution. > > > > */ > > > > - list_move_tail(&obj->mm_list, &dev_priv->mm.active_list); > > > > + list_move_tail(&obj->mm_list, &i915_gtt_vm->active_list); > > > > list_move_tail(&obj->ring_list, &ring->active_list); > > > > > > > > obj->last_read_seqno = seqno; > > > > @@ -1909,7 +1909,7 @@ i915_gem_object_move_to_inactive(struct > > > > drm_i915_gem_object *obj) > > > > BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS); > > > > BUG_ON(!obj->active); > > > > > > > > - list_move_tail(&obj->mm_list, &dev_priv->mm.inactive_list); > > > > + list_move_tail(&obj->mm_list, &i915_gtt_vm->inactive_list); > > > > > > > > list_del_init(&obj->ring_list); > > > > obj->ring = NULL; > > > > @@ -2279,12 +2279,8 @@ bool i915_gem_reset(struct drm_device *dev) > > > > /* Move everything out of the GPU domains to ensure we do any > > > > * necessary invalidation upon reuse. > > > > */ > > > > - list_for_each_entry(obj, > > > > - &dev_priv->mm.inactive_list, > > > > - mm_list) > > > > - { > > > > + list_for_each_entry(obj, &i915_gtt_vm->inactive_list, mm_list) > > > > obj->base.read_domains &= ~I915_GEM_GPU_DOMAINS; > > > > - } > > > > > > > > /* The fence registers are invalidated so clear them out */ > > > > i915_gem_restore_fences(dev); > > > > @@ -3162,7 +3158,7 @@ search_free: > > > > } > > > > > > > > list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); > > > > - list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list); > > > > + list_add_tail(&obj->mm_list, &i915_gtt_vm->inactive_list); > > > > > > > > obj->gtt_space = node; > > > > obj->gtt_offset = node->start; > > > > @@ -3313,7 +3309,7 @@ i915_gem_object_set_to_gtt_domain(struct > > > > drm_i915_gem_object *obj, bool write) > > > > > > > > /* And bump the LRU for this access */ > > > > if (i915_gem_object_is_inactive(obj)) > > > > - list_move_tail(&obj->mm_list, > > > > &dev_priv->mm.inactive_list); > > > > + list_move_tail(&obj->mm_list, > > > > &i915_gtt_vm->inactive_list); > > > > > > > > return 0; > > > > } > > > > @@ -4291,7 +4287,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, > > > > void *data, > > > > return ret; > > > > } > > > > > > > > - BUG_ON(!list_empty(&dev_priv->mm.active_list)); > > > > + BUG_ON(!list_empty(&i915_gtt_vm->active_list)); > > > > mutex_unlock(&dev->struct_mutex); > > > > > > > > ret = drm_irq_install(dev); > > > > @@ -4352,8 +4348,8 @@ i915_gem_load(struct drm_device *dev) > > > > SLAB_HWCACHE_ALIGN, > > > > NULL); > > > > > > > > - INIT_LIST_HEAD(&dev_priv->mm.active_list); > > > > - INIT_LIST_HEAD(&dev_priv->mm.inactive_list); > > > > + INIT_LIST_HEAD(&i915_gtt_vm->active_list); > > > > + INIT_LIST_HEAD(&i915_gtt_vm->inactive_list); > > > > INIT_LIST_HEAD(&dev_priv->mm.unbound_list); > > > > INIT_LIST_HEAD(&dev_priv->mm.bound_list); > > > > INIT_LIST_HEAD(&dev_priv->mm.fence_list); > > > > @@ -4652,7 +4648,7 @@ i915_gem_inactive_shrink(struct shrinker > > > > *shrinker, struct shrink_control *sc) > > > > list_for_each_entry(obj, &dev_priv->mm.unbound_list, > > > > global_list) > > > > if (obj->pages_pin_count == 0) > > > > cnt += obj->base.size >> PAGE_SHIFT; > > > > - list_for_each_entry(obj, &dev_priv->mm.inactive_list, > > > > global_list) > > > > + list_for_each_entry(obj, &i915_gtt_vm->inactive_list, > > > > global_list) > > > > if (obj->pin_count == 0 && obj->pages_pin_count == 0) > > > > cnt += obj->base.size >> PAGE_SHIFT; > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_debug.c > > > > b/drivers/gpu/drm/i915/i915_gem_debug.c > > > > index 582e6a5..bf945a3 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_debug.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_debug.c > > > > @@ -97,7 +97,7 @@ i915_verify_lists(struct drm_device *dev) > > > > } > > > > } > > > > > > > > - list_for_each_entry(obj, &dev_priv->mm.inactive_list, list) { > > > > + list_for_each_entry(obj, &i915_gtt_vm->inactive_list, list) { > > > > if (obj->base.dev != dev || > > > > !atomic_read(&obj->base.refcount.refcount)) { > > > > DRM_ERROR("freed inactive %p\n", obj); > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c > > > > b/drivers/gpu/drm/i915/i915_gem_evict.c > > > > index 6e620f86..92856a2 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > > > > @@ -86,7 +86,7 @@ i915_gem_evict_something(struct drm_device *dev, int > > > > min_size, > > > > cache_level); > > > > > > > > /* First see if there is a large enough contiguous idle > > > > region... */ > > > > - list_for_each_entry(obj, &dev_priv->mm.inactive_list, mm_list) { > > > > + list_for_each_entry(obj, &i915_gtt_vm->inactive_list, mm_list) { > > > > if (mark_free(obj, &unwind_list)) > > > > goto found; > > > > } > > > > @@ -95,7 +95,7 @@ i915_gem_evict_something(struct drm_device *dev, int > > > > min_size, > > > > goto none; > > > > > > > > /* Now merge in the soon-to-be-expired objects... */ > > > > - list_for_each_entry(obj, &dev_priv->mm.active_list, mm_list) { > > > > + list_for_each_entry(obj, &i915_gtt_vm->active_list, mm_list) { > > > > if (mark_free(obj, &unwind_list)) > > > > goto found; > > > > } > > > > @@ -158,8 +158,8 @@ i915_gem_evict_everything(struct drm_device *dev) > > > > bool lists_empty; > > > > int ret; > > > > > > > > - lists_empty = (list_empty(&dev_priv->mm.inactive_list) && > > > > - list_empty(&dev_priv->mm.active_list)); > > > > + lists_empty = (list_empty(&i915_gtt_vm->inactive_list) && > > > > + list_empty(&i915_gtt_vm->active_list)); > > > > if (lists_empty) > > > > return -ENOSPC; > > > > > > > > @@ -177,7 +177,7 @@ i915_gem_evict_everything(struct drm_device *dev) > > > > > > > > /* Having flushed everything, unbind() should never raise an > > > > error */ > > > > list_for_each_entry_safe(obj, next, > > > > - &dev_priv->mm.inactive_list, mm_list) > > > > + &i915_gtt_vm->inactive_list, mm_list) > > > > if (obj->pin_count == 0) > > > > WARN_ON(i915_gem_object_unbind(obj)); > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c > > > > b/drivers/gpu/drm/i915/i915_gem_stolen.c > > > > index 49e8be7..3f6564d 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > > > > @@ -384,7 +384,7 @@ > > > > i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, > > > > obj->has_global_gtt_mapping = 1; > > > > > > > > list_add_tail(&obj->global_list, &dev_priv->mm.bound_list); > > > > - list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list); > > > > + list_add_tail(&obj->mm_list, &i915_gtt_vm->inactive_list); > > > > > > > > return obj; > > > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > > > b/drivers/gpu/drm/i915/i915_irq.c > > > > index 1e25920..5dc055a 100644 > > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > > @@ -1722,7 +1722,7 @@ i915_error_first_batchbuffer(struct > > > > drm_i915_private *dev_priv, > > > > } > > > > > > > > seqno = ring->get_seqno(ring, false); > > > > - list_for_each_entry(obj, &dev_priv->mm.active_list, mm_list) { > > > > + list_for_each_entry(obj, &i915_gtt_vm->active_list, mm_list) { > > > > if (obj->ring != ring) > > > > continue; > > > > > > > > @@ -1857,7 +1857,7 @@ static void i915_gem_capture_buffers(struct > > > > drm_i915_private *dev_priv, > > > > int i; > > > > > > > > i = 0; > > > > - list_for_each_entry(obj, &dev_priv->mm.active_list, mm_list) > > > > + list_for_each_entry(obj, &i915_gtt_vm->active_list, mm_list) > > > > i++; > > > > error->active_bo_count = i; > > > > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) > > > > @@ -1877,7 +1877,7 @@ static void i915_gem_capture_buffers(struct > > > > drm_i915_private *dev_priv, > > > > error->active_bo_count = > > > > capture_active_bo(error->active_bo, > > > > error->active_bo_count, > > > > - &dev_priv->mm.active_list); > > > > + &i915_gtt_vm->active_list); > > > > > > > > if (error->pinned_bo) > > > > error->pinned_bo_count = > > > > -- > > > > 1.8.3.1 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > [email protected] > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > > > -- > > Ben Widawsky, Intel Open Source Technology Center > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
