Re: [PATCH 10/12] drm/i915: Introduce dedicated object VMA iterator

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 10/12] drm/i915: Introduce dedicated object VMA iterator
Date: Tue, 2 Feb 2016 12:58:05 +0000	[thread overview]
Message-ID: <20160202125805.GK15851@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <56B09CAB.8050802@linux.intel.com>

On Tue, Feb 02, 2016 at 12:10:19PM +0000, Tvrtko Ursulin wrote:
> On 02/02/16 11:36, Chris Wilson wrote:
> >#define list_for_each_entry_check(pos, list, member, lock) \
> >for (typeof(*lock) == typeof(struct mutex) ? assert_lockdep_held(lock) : assert_spin_locked(lock); \
> >      pos = list_first_entry(head, typeof(*pos), member); \
> >      &pos->member != (head); \
> >      pos = list_next_entry(pos, member))
> >
> >#define i915_gem_object_for_each_vma(vma, obj) \
> >	list_for_each_entry_check(vma, &(obj)->vma_list, vma_link, &(obj)->base.dev->struct_mutex)
> >	
> >could be lifted easily, and makes i915_gem_object_for_each_vma() easier
> >to understand (i.e. serves better as documentation).
> 
> Don't know, needs buy-in from the relevant people, and depends on
> how useful to outside of i915 it would be. And if you can make
> lockdep_assert_held work in this context.

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4004a7cf8db4..931684a74533 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -140,7 +140,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
                   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
        if (obj->base.name)
                seq_printf(m, " (name: %d)", obj->base.name);
-       list_for_each_entry(vma, &obj->vma_list, obj_link) {
+       i915_gem_object_for_each_vma(vma, obj) {
                if (vma->pin_count > 0)
                        pin_count++;
        }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 31487aa11977..574e45ab43cb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3565,4 +3565,36 @@ int remap_io_mapping(struct vm_area_struct *vma,
                     unsigned long addr, unsigned long pfn, unsigned long size,
                     struct io_mapping *iomap);
 
+static __always_inline void __list_check_mutex(struct mutex *lock)
+{
+       lockdep_assert_held(lock);
+}
+
+static __always_inline void __list_check_spin(struct spinlock *lock)
+{
+        assert_spin_locked(lock);
+}
+
+static __always_inline void __list_check_bug(void *_lock)
+{
+       BUILD_BUG_ON("unknown lock type");
+}
+
+#define __list_check(lock) \
+       ({ __builtin_types_compatible_p(typeof(*lock), struct mutex) ? \
+        __list_check_mutex((struct mutex *)lock) : \
+        __builtin_types_compatible_p(typeof(*lock), struct spinlock) ? \
+        __list_check_spin((struct spinlock *)lock) : \
+        __list_check_bug(lock); \
+        0; })
+
+#define list_for_each_entry_check(pos, head, member, lock) \
+       for (__list_check(lock), \
+            pos = list_first_entry(head, typeof(*pos), member); \
+            &pos->member != (head); \
+            pos = list_next_entry(pos, member))
+
+#define i915_gem_object_for_each_vma(vma, obj) \
+       list_for_each_entry_check(vma, &(obj)->vma_list, obj_link, &(obj)->base.dev->struct_mutex)
+
 #endif

> But I am also not sure all i915 debugging should be tied to lockdep
> debugging so to me it is not so clear-cut.

> >>+	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
> >>+	     &vma->vma_link != (&(obj)->vma_list); \
> >>+	     vma = list_next_entry(vma, vma_link))
> >>+#else
> >>+  #define i915_gem_obj_for_each_vma(vma, obj) \
> >>+		list_for_each_entry((vma), &(obj)->vma_list, vma_link)
> >>+#endif
> >>+
> >>  /* Flags used by pin/bind&friends. */
> >>  #define PIN_MAPPABLE	(1<<0)
> >>  #define PIN_NONBLOCK	(1<<1)
> >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>index c558887b2084..ce9d0544b42c 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>@@ -2454,7 +2454,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
> >>  	list_move_tail(&obj->global_list,
> >>  		       &to_i915(obj->base.dev)->mm.bound_list);
> >>
> >>-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >>+	i915_gem_obj_for_each_vma(vma, obj) {
> >
> >This and the majority of the conversions simply should not exist and
> >only do so because of what I consider to be bad API. After they are
> 
> Bad API or what you really meant was GEM internals should not do it?
> What is the harm anyway? More use, if it is painless, means less
> likelyhood for copy&paste errors in the future.

I mean we have many functions that walk the vma_list that have no
purpose.
 
> >removed, there are only a few list iterators left. That said, there is
> >value in documenting the current locking.
> 
> The last bit meaning exactly?

This patch (its precusor) did not find a bug, it generated a
false-positive warning. It does have some usefulness for providing
locking annotation. And in the future when the locking changes, it will
be useful in verifying the callsites remain correct.
-Chris

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