[PATCH 1/2] drm/i915: Comment userptr recursion on struct_mutex

[PATCH 1/2] drm/i915: Comment userptr recursion on struct_mutex

[Daniel Vetter]

Discussed this a bit with Chris, I think a comment here is warranted
that this will be bad once we have more than one i915 instance. And
lockdep won't catch it.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 74da35611d7c..70dc506a5426 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -135,6 +135,12 @@ userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 			switch (mutex_trylock_recursive(unlock)) {
 			default:
 			case MUTEX_TRYLOCK_FAILED:
+				/*
+				 * NOTE: This only works because there's only
+				 * ever one i915-style struct_mutex in the
+				 * entire system. If we could have two i915
+				 * instances, this would deadlock.
+				 */
 				if (mutex_lock_killable_nested(unlock, I915_MM_SHRINKER)) {
 					i915_gem_object_put(obj);
 					return -EINTR;
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Daniel Vetter]

The trouble with having a plain nesting flag for locks which do not
naturally nest (unlike block devices and their partitions, which is
the original motivation for nesting levels) is that lockdep will
never spot a true deadlock if you screw up.

This patch is an attempt at trying better, by highlighting a bit more
the actual nature of the nesting that's going on. Essentially we have
two kinds of objects:

- objects without pages allocated, which cannot be on any lru and are
  hence inaccessible to the shrinker.

- objects which have pages allocated, which are on an lru, and which
  the shrinker can decide to throw out.

For the former type of object, memory allcoations while holding
obj->mm.lock are permissible. For the latter they are not. And
get/put_pages transitions between the two types of objects.

This is still not entirely fool-proof since the rules might chance.
But as long as we run such a code ever at runtime lockdep should be
able to observe the inconsistency and complain (like with any other
lockdep class that we've split up in multiple classes). But there are
a few clear benefits:

- We can drop the nesting flag parameter from
  __i915_gem_object_put_pages, because that function by definition is
  never going allocate memory, and calling it on an object which
  doesn't have its pages allocated would be a bug.

- We strictly catch more bugs, since there's not only one place in the
  entire tree which is annotated with the special class. All the
  other places that had explicit lockdep nesting annotations we're now
  going to leave up to lockdep again.

- Specifically this catches stuff like calling get_pages from
  put_pages (which isn't really a good idea, if we can call put_pages
  so could the shrinker). I've seen patches do exactly that.

Of course I fully expect CI will show me for the fool I am with this
one here :-)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c       |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.h       | 16 +++++++++++++---
 drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 10 +++++++++-
 drivers/gpu/drm/i915/gem/i915_gem_pages.c        |  7 +++----
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c     |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c      |  2 +-
 drivers/gpu/drm/i915/gem/selftests/huge_pages.c  | 12 ++++++------
 7 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 3929c3a6b281..a1a835539e09 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -191,7 +191,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
 		GEM_BUG_ON(!list_empty(&obj->lut_list));
 
 		atomic_set(&obj->mm.pages_pin_count, 0);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		GEM_BUG_ON(i915_gem_object_has_pages(obj));
 		bitmap_free(obj->bit_17);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 3714cf234d64..3bde9a601eca 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -281,11 +281,21 @@ i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 
 enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock/struct_mutex */
 	I915_MM_NORMAL = 0,
-	I915_MM_SHRINKER /* called "recursively" from direct-reclaim-esque */
+	/*
+	 * Only used by struct_mutex, when called "recursively" from
+	 * direct-reclaim-esque. Safe because there is only every one
+	 * struct_mutex in the entire system. */
+	I915_MM_SHRINKER,
+	/*
+	 * Used for obj->mm.lock when allocating pages. Safe because the object
+	 * isn't yet on any LRU, and therefore the shrinker can't deadlock on
+	 * it. As soon as the object has pages, obj->mm.lock nests within
+	 * fs_reclaim.
+	 */
+	I915_MM_GET_PAGES
 };
 
-int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				enum i915_mm_subclass subclass);
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
 void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
 void i915_gem_object_writeback(struct drm_i915_gem_object *obj);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index d474c6ac4100..1ea3c3c96a5a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -157,7 +157,15 @@ struct drm_i915_gem_object {
 	unsigned int pin_global;
 
 	struct {
-		struct mutex lock; /* protects the pages and their use */
+		/*
+		 * Protects the pages and their use.
+		 *
+		 * IMPORTANT: It is not allowed to allocate memory while holding
+		 * this lock, because the shrinker might recurse on it, except
+		 * when there are no pages allocated yet and the object isn't
+		 * visible on any LRU.
+		 */
+		struct mutex lock;
 		atomic_t pages_pin_count;
 
 		struct sg_table *pages;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index 18f0ce0135c1..3b7ec6e6ea8b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -101,7 +101,7 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 {
 	int err;
 
-	err = mutex_lock_interruptible(&obj->mm.lock);
+	err = mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES);
 	if (err)
 		return err;
 
@@ -179,8 +179,7 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
 	return pages;
 }
 
-int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				enum i915_mm_subclass subclass)
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 {
 	struct sg_table *pages;
 	int err;
@@ -191,7 +190,7 @@ int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
 	GEM_BUG_ON(atomic_read(&obj->bind_count));
 
 	/* May be called by shrinker from within get_pages() (on another bo) */
-	mutex_lock_nested(&obj->mm.lock, subclass);
+	mutex_lock(&obj->mm.lock);
 	if (unlikely(atomic_read(&obj->mm.pages_pin_count))) {
 		err = -EBUSY;
 		goto unlock;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
index edd21d14e64f..773f8927929d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
@@ -98,7 +98,7 @@ static bool unsafe_drop_pages(struct drm_i915_gem_object *obj,
 		flags = I915_GEM_OBJECT_UNBIND_ACTIVE;
 
 	if (i915_gem_object_unbind(obj, flags) == 0)
-		__i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
+		__i915_gem_object_put_pages(obj);
 
 	return !i915_gem_object_has_pages(obj);
 }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 70dc506a5426..f7475f2c84d1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -158,7 +158,7 @@ userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		ret = i915_gem_object_unbind(obj,
 					     I915_GEM_OBJECT_UNBIND_ACTIVE);
 		if (ret == 0)
-			ret = __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
+			ret = __i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 		if (ret)
 			goto unlock;
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index 6cbd4a668c9a..df586035c33e 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -562,7 +562,7 @@ static int igt_mock_ppgtt_misaligned_dma(void *arg)
 		i915_vma_close(vma);
 
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 
@@ -590,7 +590,7 @@ static void close_object_list(struct list_head *objects,
 
 		list_del(&obj->st_link);
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 }
@@ -860,7 +860,7 @@ static int igt_mock_ppgtt_64K(void *arg)
 			i915_vma_close(vma);
 
 			i915_gem_object_unpin_pages(obj);
-			__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+			__i915_gem_object_put_pages(obj);
 			i915_gem_object_put(obj);
 		}
 	}
@@ -1268,7 +1268,7 @@ static int igt_ppgtt_exhaust_huge(void *arg)
 			}
 
 			i915_gem_object_unpin_pages(obj);
-			__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+			__i915_gem_object_put_pages(obj);
 			i915_gem_object_put(obj);
 		}
 	}
@@ -1330,7 +1330,7 @@ static int igt_ppgtt_internal_huge(void *arg)
 		}
 
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 
@@ -1399,7 +1399,7 @@ static int igt_ppgtt_gemfs_huge(void *arg)
 		}
 
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Chris Wilson]

Quoting Daniel Vetter (2019-08-14 13:49:33)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> index d474c6ac4100..1ea3c3c96a5a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -157,7 +157,15 @@ struct drm_i915_gem_object {
>         unsigned int pin_global;
>  
>         struct {
> -               struct mutex lock; /* protects the pages and their use */
> +               /*
> +                * Protects the pages and their use.
> +                *
> +                * IMPORTANT: It is not allowed to allocate memory while holding
> +                * this lock, because the shrinker might recurse on it, except
> +                * when there are no pages allocated yet and the object isn't
> +                * visible on any LRU.

It's not meant to be public free-for-lock, just to guard the transition
between 0<->1. Inside that transition we do page allocations.

Everyone else takes a pin.

> +                */
> +               struct mutex lock;
>                 atomic_t pages_pin_count;
>  
>                 struct sg_table *pages;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index 18f0ce0135c1..3b7ec6e6ea8b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -101,7 +101,7 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)

Fwiw, we have use cases (and people asking where are those patches) for
nested allocations.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex
URL   : https://patchwork.freedesktop.org/series/65177/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
ea422f4a5a9e drm/i915: Comment userptr recursion on struct_mutex
-:30: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'

total: 0 errors, 1 warnings, 0 checks, 12 lines checked
2c4c1d48651d RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head
-:78: WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#78: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:287:
+	 * struct_mutex in the entire system. */

-:231: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'

total: 0 errors, 2 warnings, 0 checks, 137 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Daniel Vetter]

On Wed, Aug 14, 2019 at 3:06 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Daniel Vetter (2019-08-14 13:49:33)
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > index d474c6ac4100..1ea3c3c96a5a 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > @@ -157,7 +157,15 @@ struct drm_i915_gem_object {
> >         unsigned int pin_global;
> >
> >         struct {
> > -               struct mutex lock; /* protects the pages and their use */
> > +               /*
> > +                * Protects the pages and their use.
> > +                *
> > +                * IMPORTANT: It is not allowed to allocate memory while holding
> > +                * this lock, because the shrinker might recurse on it, except
> > +                * when there are no pages allocated yet and the object isn't
> > +                * visible on any LRU.
>
> It's not meant to be public free-for-lock, just to guard the transition
> between 0<->1. Inside that transition we do page allocations.
>
> Everyone else takes a pin.

Well yeah the comment is missing a lot of things.
>
> > +                */
> > +               struct mutex lock;
> >                 atomic_t pages_pin_count;
> >
> >                 struct sg_table *pages;
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > index 18f0ce0135c1..3b7ec6e6ea8b 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > @@ -101,7 +101,7 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
>
> Fwiw, we have use cases (and people asking where are those patches) for
> nested allocations.

Yeah and it's those patches I'm freaking out about. Imo the current
annotations for obj->mm.lock just annotate the call chain nesting.
Which trivially shuts up lockdep, but also guarantees you're not going
to find real deadlocks before they happen. There needs to be a very
clear proof attached to why the nesting annotation is correct. Random
selection of examples:
- block partitions nesting in their overall device, where you never
take them the other way round
- the struct_mutex nesting, which works because there's only ever one
struct_mutex in the entire system. As soon as there are two we need a
new reason (and that's the reason for patch 1 here).
- obj->mm.lock, which nest both ways with fs_reclaim, but there's a
clear difference for the pages_pin_count goes 0->1 (allocation
allowed) and 1->0 (can be called from shrinker context or anything
else that needs freeing Which this series tries to make a bit clearer

Just noticing that the shrinker generally can nest within normal code
and annotating that nesting like that doesn't really proof anything.
And allows some fun stuff, like someon allocating memory from a
put_pages call, which I expect will lead to some fun later on. Just
kinda usual paranoia.

And I'm not sure at all whether at least the internal patches floating
around with a lot more nesting actually work. There's not really solid
explanation attached to them, and I haven't figured one out (unlike
for the two cases in upstream we have already, with struct_mutex and
obj->mm.lock).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex
URL   : https://patchwork.freedesktop.org/series/65177/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6707 -> Patchwork_14014
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_14014 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_14014, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_14014:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_gttfill@basic:
    - fi-kbl-x1275:       [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-kbl-x1275/igt@gem_exec_gttfill@basic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-kbl-x1275/igt@gem_exec_gttfill@basic.html
    - fi-icl-u3:          [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-icl-u3/igt@gem_exec_gttfill@basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-icl-u3/igt@gem_exec_gttfill@basic.html
    - fi-cml-u:           [PASS][5] -> [DMESG-WARN][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-cml-u/igt@gem_exec_gttfill@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-cml-u/igt@gem_exec_gttfill@basic.html
    - fi-kbl-8809g:       [PASS][7] -> [DMESG-WARN][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-kbl-8809g/igt@gem_exec_gttfill@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-kbl-8809g/igt@gem_exec_gttfill@basic.html
    - fi-kbl-guc:         [PASS][9] -> [DMESG-WARN][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-kbl-guc/igt@gem_exec_gttfill@basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-kbl-guc/igt@gem_exec_gttfill@basic.html
    - fi-cml-u2:          [PASS][11] -> [DMESG-WARN][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-cml-u2/igt@gem_exec_gttfill@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-cml-u2/igt@gem_exec_gttfill@basic.html
    - fi-skl-6600u:       [PASS][13] -> [DMESG-WARN][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-skl-6600u/igt@gem_exec_gttfill@basic.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-skl-6600u/igt@gem_exec_gttfill@basic.html
    - fi-ivb-3770:        [PASS][15] -> [DMESG-WARN][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-ivb-3770/igt@gem_exec_gttfill@basic.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-ivb-3770/igt@gem_exec_gttfill@basic.html
    - fi-byt-j1900:       [PASS][17] -> [DMESG-WARN][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-byt-j1900/igt@gem_exec_gttfill@basic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-byt-j1900/igt@gem_exec_gttfill@basic.html
    - fi-skl-lmem:        [PASS][19] -> [DMESG-WARN][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-skl-lmem/igt@gem_exec_gttfill@basic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-skl-lmem/igt@gem_exec_gttfill@basic.html
    - fi-apl-guc:         [PASS][21] -> [DMESG-WARN][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-apl-guc/igt@gem_exec_gttfill@basic.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-apl-guc/igt@gem_exec_gttfill@basic.html
    - fi-byt-n2820:       [PASS][23] -> [DMESG-WARN][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-byt-n2820/igt@gem_exec_gttfill@basic.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-byt-n2820/igt@gem_exec_gttfill@basic.html
    - fi-skl-6770hq:      [PASS][25] -> [DMESG-WARN][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-skl-6770hq/igt@gem_exec_gttfill@basic.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-skl-6770hq/igt@gem_exec_gttfill@basic.html
    - fi-skl-6260u:       [PASS][27] -> [DMESG-WARN][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-skl-6260u/igt@gem_exec_gttfill@basic.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-skl-6260u/igt@gem_exec_gttfill@basic.html
    - fi-cfl-8109u:       [PASS][29] -> [DMESG-WARN][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-cfl-8109u/igt@gem_exec_gttfill@basic.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-cfl-8109u/igt@gem_exec_gttfill@basic.html
    - fi-kbl-r:           [PASS][31] -> [DMESG-WARN][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-kbl-r/igt@gem_exec_gttfill@basic.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-kbl-r/igt@gem_exec_gttfill@basic.html
    - fi-bdw-5557u:       NOTRUN -> [DMESG-WARN][33]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-bdw-5557u/igt@gem_exec_gttfill@basic.html
    - fi-kbl-7567u:       [PASS][34] -> [DMESG-WARN][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-kbl-7567u/igt@gem_exec_gttfill@basic.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-kbl-7567u/igt@gem_exec_gttfill@basic.html
    - fi-skl-guc:         [PASS][36] -> [DMESG-WARN][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-skl-guc/igt@gem_exec_gttfill@basic.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-skl-guc/igt@gem_exec_gttfill@basic.html
    - fi-pnv-d510:        [PASS][38] -> [DMESG-WARN][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-pnv-d510/igt@gem_exec_gttfill@basic.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-pnv-d510/igt@gem_exec_gttfill@basic.html
    - fi-cfl-8700k:       [PASS][40] -> [DMESG-WARN][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-cfl-8700k/igt@gem_exec_gttfill@basic.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-cfl-8700k/igt@gem_exec_gttfill@basic.html
    - fi-whl-u:           [PASS][42] -> [DMESG-WARN][43]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-whl-u/igt@gem_exec_gttfill@basic.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-whl-u/igt@gem_exec_gttfill@basic.html
    - fi-icl-u2:          [PASS][44] -> [DMESG-WARN][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-icl-u2/igt@gem_exec_gttfill@basic.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-icl-u2/igt@gem_exec_gttfill@basic.html
    - fi-snb-2520m:       [PASS][46] -> [DMESG-WARN][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-snb-2520m/igt@gem_exec_gttfill@basic.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-snb-2520m/igt@gem_exec_gttfill@basic.html
    - fi-elk-e7500:       [PASS][48] -> [DMESG-WARN][49]
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-elk-e7500/igt@gem_exec_gttfill@basic.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-elk-e7500/igt@gem_exec_gttfill@basic.html
    - fi-skl-iommu:       [PASS][50] -> [DMESG-WARN][51]
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-skl-iommu/igt@gem_exec_gttfill@basic.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-skl-iommu/igt@gem_exec_gttfill@basic.html
    - fi-cfl-guc:         [PASS][52] -> [DMESG-WARN][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-cfl-guc/igt@gem_exec_gttfill@basic.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-cfl-guc/igt@gem_exec_gttfill@basic.html
    - fi-ilk-650:         [PASS][54] -> [DMESG-WARN][55]
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-ilk-650/igt@gem_exec_gttfill@basic.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-ilk-650/igt@gem_exec_gttfill@basic.html

  * igt@gem_mmap@basic-small-bo:
    - fi-bwr-2160:        [PASS][56] -> [DMESG-WARN][57]
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-bwr-2160/igt@gem_mmap@basic-small-bo.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-bwr-2160/igt@gem_mmap@basic-small-bo.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@gem_exec_gttfill@basic:
    - {fi-icl-dsi}:       [PASS][58] -> [DMESG-WARN][59]
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-icl-dsi/igt@gem_exec_gttfill@basic.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-icl-dsi/igt@gem_exec_gttfill@basic.html
    - {fi-icl-u4}:        [PASS][60] -> [DMESG-WARN][61]
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-icl-u4/igt@gem_exec_gttfill@basic.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-icl-u4/igt@gem_exec_gttfill@basic.html
    - {fi-icl-guc}:       NOTRUN -> [DMESG-WARN][62]
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-icl-guc/igt@gem_exec_gttfill@basic.html

  
Known issues
------------

  Here are the changes found in Patchwork_14014 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_gttfill@basic:
    - fi-hsw-4770:        [PASS][63] -> [DMESG-WARN][64] ([fdo#110789])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-hsw-4770/igt@gem_exec_gttfill@basic.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-hsw-4770/igt@gem_exec_gttfill@basic.html
    - fi-snb-2600:        [PASS][65] -> [DMESG-WARN][66] ([fdo#110789])
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-snb-2600/igt@gem_exec_gttfill@basic.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-snb-2600/igt@gem_exec_gttfill@basic.html
    - fi-hsw-4770r:       [PASS][67] -> [DMESG-WARN][68] ([fdo#110789])
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-hsw-4770r/igt@gem_exec_gttfill@basic.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-hsw-4770r/igt@gem_exec_gttfill@basic.html

  * igt@gem_exec_store@basic-all:
    - fi-icl-u3:          [PASS][69] -> [DMESG-WARN][70] ([fdo#107724]) +1 similar issue
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-icl-u3/igt@gem_exec_store@basic-all.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-icl-u3/igt@gem_exec_store@basic-all.html

  * igt@i915_selftest@live_execlists:
    - fi-bwr-2160:        [PASS][71] -> [DMESG-WARN][72] ([fdo#111115])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-bwr-2160/igt@i915_selftest@live_execlists.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-bwr-2160/igt@i915_selftest@live_execlists.html

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        [PASS][73] -> [DMESG-FAIL][74] ([fdo#111115])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-bwr-2160/igt@i915_selftest@live_hangcheck.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-bwr-2160/igt@i915_selftest@live_hangcheck.html

  
#### Possible fixes ####

  * igt@gem_cpu_reloc@basic:
    - {fi-icl-dsi}:       [DMESG-WARN][75] ([fdo#106107]) -> [PASS][76]
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-icl-dsi/igt@gem_cpu_reloc@basic.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-icl-dsi/igt@gem_cpu_reloc@basic.html

  * igt@gem_ctx_switch@rcs0:
    - {fi-icl-guc}:       [INCOMPLETE][77] ([fdo#107713]) -> [PASS][78]
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-icl-guc/igt@gem_ctx_switch@rcs0.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-icl-guc/igt@gem_ctx_switch@rcs0.html

  * igt@i915_selftest@live_execlists:
    - fi-skl-gvtdvm:      [DMESG-FAIL][79] ([fdo#111108]) -> [PASS][80]
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html

  * igt@prime_busy@basic-wait-before-default:
    - fi-icl-u3:          [DMESG-WARN][81] ([fdo#107724]) -> [PASS][82] +1 similar issue
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-icl-u3/igt@prime_busy@basic-wait-before-default.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-icl-u3/igt@prime_busy@basic-wait-before-default.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         [DMESG-WARN][83] ([fdo#106387]) -> [PASS][84] +1 similar issue
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6707/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#106107]: https://bugs.freedesktop.org/show_bug.cgi?id=106107
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#110789]: https://bugs.freedesktop.org/show_bug.cgi?id=110789
  [fdo#111108]: https://bugs.freedesktop.org/show_bug.cgi?id=111108
  [fdo#111115]: https://bugs.freedesktop.org/show_bug.cgi?id=111115


Participating hosts (53 -> 46)
------------------------------

  Additional (2): fi-bxt-j4205 fi-bdw-5557u 
  Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6707 -> Patchwork_14014

  CI-20190529: 20190529
  CI_DRM_6707: 82b4c694de0a905e1fd86b56009fa1f21ae99324 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5134: 81df2f22385bc275975cf199d962eed9bc10f916 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14014: 2c4c1d48651dfa7ff884e5c2eeb56cde137148ac @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2c4c1d48651d RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head
ea422f4a5a9e drm/i915: Comment userptr recursion on struct_mutex

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14014/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Daniel Vetter]

The trouble with having a plain nesting flag for locks which do not
naturally nest (unlike block devices and their partitions, which is
the original motivation for nesting levels) is that lockdep will
never spot a true deadlock if you screw up.

This patch is an attempt at trying better, by highlighting a bit more
the actual nature of the nesting that's going on. Essentially we have
two kinds of objects:

- objects without pages allocated, which cannot be on any lru and are
  hence inaccessible to the shrinker.

- objects which have pages allocated, which are on an lru, and which
  the shrinker can decide to throw out.

For the former type of object, memory allcoations while holding
obj->mm.lock are permissible. For the latter they are not. And
get/put_pages transitions between the two types of objects.

This is still not entirely fool-proof since the rules might chance.
But as long as we run such a code ever at runtime lockdep should be
able to observe the inconsistency and complain (like with any other
lockdep class that we've split up in multiple classes). But there are
a few clear benefits:

- We can drop the nesting flag parameter from
  __i915_gem_object_put_pages, because that function by definition is
  never going allocate memory, and calling it on an object which
  doesn't have its pages allocated would be a bug.

- We strictly catch more bugs, since there's not only one place in the
  entire tree which is annotated with the special class. All the
  other places that had explicit lockdep nesting annotations we're now
  going to leave up to lockdep again.

- Specifically this catches stuff like calling get_pages from
  put_pages (which isn't really a good idea, if we can call put_pages
  so could the shrinker). I've seen patches do exactly that.

Of course I fully expect CI will show me for the fool I am with this
one here :-)

v2: There can only be one (lockdep only has a cache for the first
subclass, not for deeper ones, and we don't want to make these locks
even slower). Still separate enums for better documentation.

Real fix: don forget about phys objs and pin_map(), and fix the
shrinker to have the right annotations ... silly me.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c       |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.h       | 16 +++++++++++++---
 drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 10 +++++++++-
 drivers/gpu/drm/i915/gem/i915_gem_pages.c        |  9 ++++-----
 drivers/gpu/drm/i915/gem/i915_gem_phys.c         |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c     |  5 ++---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c      |  2 +-
 drivers/gpu/drm/i915/gem/selftests/huge_pages.c  | 12 ++++++------
 8 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 3929c3a6b281..a1a835539e09 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -191,7 +191,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
 		GEM_BUG_ON(!list_empty(&obj->lut_list));
 
 		atomic_set(&obj->mm.pages_pin_count, 0);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		GEM_BUG_ON(i915_gem_object_has_pages(obj));
 		bitmap_free(obj->bit_17);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 3714cf234d64..5ce511ca7fa8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -281,11 +281,21 @@ i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 
 enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock/struct_mutex */
 	I915_MM_NORMAL = 0,
-	I915_MM_SHRINKER /* called "recursively" from direct-reclaim-esque */
+	/*
+	 * Only used by struct_mutex, when called "recursively" from
+	 * direct-reclaim-esque. Safe because there is only every one
+	 * struct_mutex in the entire system. */
+	I915_MM_SHRINKER = 1,
+	/*
+	 * Used for obj->mm.lock when allocating pages. Safe because the object
+	 * isn't yet on any LRU, and therefore the shrinker can't deadlock on
+	 * it. As soon as the object has pages, obj->mm.lock nests within
+	 * fs_reclaim.
+	 */
+	I915_MM_GET_PAGES = 1,
 };
 
-int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				enum i915_mm_subclass subclass);
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
 void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
 void i915_gem_object_writeback(struct drm_i915_gem_object *obj);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index d474c6ac4100..1ea3c3c96a5a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -157,7 +157,15 @@ struct drm_i915_gem_object {
 	unsigned int pin_global;
 
 	struct {
-		struct mutex lock; /* protects the pages and their use */
+		/*
+		 * Protects the pages and their use.
+		 *
+		 * IMPORTANT: It is not allowed to allocate memory while holding
+		 * this lock, because the shrinker might recurse on it, except
+		 * when there are no pages allocated yet and the object isn't
+		 * visible on any LRU.
+		 */
+		struct mutex lock;
 		atomic_t pages_pin_count;
 
 		struct sg_table *pages;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index 18f0ce0135c1..202526e8910f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -101,7 +101,7 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 {
 	int err;
 
-	err = mutex_lock_interruptible(&obj->mm.lock);
+	err = mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES);
 	if (err)
 		return err;
 
@@ -179,8 +179,7 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
 	return pages;
 }
 
-int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				enum i915_mm_subclass subclass)
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 {
 	struct sg_table *pages;
 	int err;
@@ -191,7 +190,7 @@ int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
 	GEM_BUG_ON(atomic_read(&obj->bind_count));
 
 	/* May be called by shrinker from within get_pages() (on another bo) */
-	mutex_lock_nested(&obj->mm.lock, subclass);
+	mutex_lock(&obj->mm.lock);
 	if (unlikely(atomic_read(&obj->mm.pages_pin_count))) {
 		err = -EBUSY;
 		goto unlock;
@@ -285,7 +284,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
 	if (unlikely(!i915_gem_object_has_struct_page(obj)))
 		return ERR_PTR(-ENXIO);
 
-	err = mutex_lock_interruptible(&obj->mm.lock);
+	err = mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES);
 	if (err)
 		return ERR_PTR(err);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index 102fd7a23d3d..209925be8a76 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -156,7 +156,7 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, int align)
 	if (err)
 		return err;
 
-	mutex_lock(&obj->mm.lock);
+	mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES);
 
 	if (obj->mm.madv != I915_MADV_WILLNEED) {
 		err = -EFAULT;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
index edd21d14e64f..0b0d6e27b996 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
@@ -98,7 +98,7 @@ static bool unsafe_drop_pages(struct drm_i915_gem_object *obj,
 		flags = I915_GEM_OBJECT_UNBIND_ACTIVE;
 
 	if (i915_gem_object_unbind(obj, flags) == 0)
-		__i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
+		__i915_gem_object_put_pages(obj);
 
 	return !i915_gem_object_has_pages(obj);
 }
@@ -254,8 +254,7 @@ i915_gem_shrink(struct drm_i915_private *i915,
 
 			if (unsafe_drop_pages(obj, shrink)) {
 				/* May arrive from get_pages on another bo */
-				mutex_lock_nested(&obj->mm.lock,
-						  I915_MM_SHRINKER);
+				mutex_lock(&obj->mm.lock);
 				if (!i915_gem_object_has_pages(obj)) {
 					try_to_writeback(obj, shrink);
 					count += obj->base.size >> PAGE_SHIFT;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 70dc506a5426..f7475f2c84d1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -158,7 +158,7 @@ userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		ret = i915_gem_object_unbind(obj,
 					     I915_GEM_OBJECT_UNBIND_ACTIVE);
 		if (ret == 0)
-			ret = __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
+			ret = __i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 		if (ret)
 			goto unlock;
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index 6cbd4a668c9a..df586035c33e 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -562,7 +562,7 @@ static int igt_mock_ppgtt_misaligned_dma(void *arg)
 		i915_vma_close(vma);
 
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 
@@ -590,7 +590,7 @@ static void close_object_list(struct list_head *objects,
 
 		list_del(&obj->st_link);
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 }
@@ -860,7 +860,7 @@ static int igt_mock_ppgtt_64K(void *arg)
 			i915_vma_close(vma);
 
 			i915_gem_object_unpin_pages(obj);
-			__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+			__i915_gem_object_put_pages(obj);
 			i915_gem_object_put(obj);
 		}
 	}
@@ -1268,7 +1268,7 @@ static int igt_ppgtt_exhaust_huge(void *arg)
 			}
 
 			i915_gem_object_unpin_pages(obj);
-			__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+			__i915_gem_object_put_pages(obj);
 			i915_gem_object_put(obj);
 		}
 	}
@@ -1330,7 +1330,7 @@ static int igt_ppgtt_internal_huge(void *arg)
 		}
 
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 
@@ -1399,7 +1399,7 @@ static int igt_ppgtt_gemfs_huge(void *arg)
 		}
 
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Chris Wilson]

Quoting Daniel Vetter (2019-08-14 19:49:41)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> index d474c6ac4100..1ea3c3c96a5a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -157,7 +157,15 @@ struct drm_i915_gem_object {
>         unsigned int pin_global;
>  
>         struct {
> -               struct mutex lock; /* protects the pages and their use */
> +               /*
> +                * Protects the pages and their use.

"Their use" is still a misleading suggest of mine. This should be
"protects the pinning of pages". The couple of other things it is used
for are tied to the concept of the pages being pinned; further use should
be discouraged; direct use prohibited.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Comment userptr recursion on struct_mutex

[Daniel Vetter]

On Wed, Aug 14, 2019 at 02:49:32PM +0200, Daniel Vetter wrote:
> Discussed this a bit with Chris, I think a comment here is warranted
> that this will be bad once we have more than one i915 instance. And
> lockdep won't catch it.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 74da35611d7c..70dc506a5426 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -135,6 +135,12 @@ userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  			switch (mutex_trylock_recursive(unlock)) {
>  			default:
>  			case MUTEX_TRYLOCK_FAILED:
> +				/*
> +				 * NOTE: This only works because there's only
> +				 * ever one i915-style struct_mutex in the
> +				 * entire system. If we could have two i915
> +				 * instances, this would deadlock.
> +				 */

While fixing up annotations for the 2nd patch I though more about this,
and I'm not sold that "there's only one" makes sense. Scenario:

thread A:
get_pages
-> mutex_lock(obj->mm.lock)
-> fs_reclaim
-> mmu_notifier picks range of memory we're interested in
-> mutex_lock_killable(struct_mutex)

Why can this not deadlock against any other thread which does:

mutex_lock(struct_mutex)
-> get_pages
-> mutex_lock(obj->mm.lock)

They would both need to pick the same object, that's right now at a 0->1
transition for pages_pin_count. Plus a long list of other unhappy
circumstances ...

Note that this is different from the same annotation in shrinker_lock:
That one is only used if current_is_kswapd is, which guarantees we're not
holding a few unfortunate locks.
-Daniel

>  				if (mutex_lock_killable_nested(unlock, I915_MM_SHRINKER)) {
>  					i915_gem_object_put(obj);
>  					return -EINTR;
> -- 
> 2.22.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Daniel Vetter]

The trouble with having a plain nesting flag for locks which do not
naturally nest (unlike block devices and their partitions, which is
the original motivation for nesting levels) is that lockdep will
never spot a true deadlock if you screw up.

This patch is an attempt at trying better, by highlighting a bit more
the actual nature of the nesting that's going on. Essentially we have
two kinds of objects:

- objects without pages allocated, which cannot be on any lru and are
  hence inaccessible to the shrinker.

- objects which have pages allocated, which are on an lru, and which
  the shrinker can decide to throw out.

For the former type of object, memory allcoations while holding
obj->mm.lock are permissible. For the latter they are not. And
get/put_pages transitions between the two types of objects.

This is still not entirely fool-proof since the rules might chance.
But as long as we run such a code ever at runtime lockdep should be
able to observe the inconsistency and complain (like with any other
lockdep class that we've split up in multiple classes). But there are
a few clear benefits:

- We can drop the nesting flag parameter from
  __i915_gem_object_put_pages, because that function by definition is
  never going allocate memory, and calling it on an object which
  doesn't have its pages allocated would be a bug.

- We strictly catch more bugs, since there's not only one place in the
  entire tree which is annotated with the special class. All the
  other places that had explicit lockdep nesting annotations we're now
  going to leave up to lockdep again.

- Specifically this catches stuff like calling get_pages from
  put_pages (which isn't really a good idea, if we can call put_pages
  so could the shrinker). I've seen patches do exactly that.

Of course I fully expect CI will show me for the fool I am with this
one here :-)

v2: There can only be one (lockdep only has a cache for the first
subclass, not for deeper ones, and we don't want to make these locks
even slower). Still separate enums for better documentation.

Real fix: don forget about phys objs and pin_map(), and fix the
shrinker to have the right annotations ... silly me.

v3: Forgot usertptr too ...

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c       |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.h       | 16 +++++++++++++---
 drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 10 +++++++++-
 drivers/gpu/drm/i915/gem/i915_gem_pages.c        |  9 ++++-----
 drivers/gpu/drm/i915/gem/i915_gem_phys.c         |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c     |  5 ++---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c      |  4 ++--
 drivers/gpu/drm/i915/gem/selftests/huge_pages.c  | 12 ++++++------
 8 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 3929c3a6b281..a1a835539e09 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -191,7 +191,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
 		GEM_BUG_ON(!list_empty(&obj->lut_list));
 
 		atomic_set(&obj->mm.pages_pin_count, 0);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		GEM_BUG_ON(i915_gem_object_has_pages(obj));
 		bitmap_free(obj->bit_17);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 3714cf234d64..5ce511ca7fa8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -281,11 +281,21 @@ i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 
 enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock/struct_mutex */
 	I915_MM_NORMAL = 0,
-	I915_MM_SHRINKER /* called "recursively" from direct-reclaim-esque */
+	/*
+	 * Only used by struct_mutex, when called "recursively" from
+	 * direct-reclaim-esque. Safe because there is only every one
+	 * struct_mutex in the entire system. */
+	I915_MM_SHRINKER = 1,
+	/*
+	 * Used for obj->mm.lock when allocating pages. Safe because the object
+	 * isn't yet on any LRU, and therefore the shrinker can't deadlock on
+	 * it. As soon as the object has pages, obj->mm.lock nests within
+	 * fs_reclaim.
+	 */
+	I915_MM_GET_PAGES = 1,
 };
 
-int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				enum i915_mm_subclass subclass);
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
 void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
 void i915_gem_object_writeback(struct drm_i915_gem_object *obj);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index d474c6ac4100..1ea3c3c96a5a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -157,7 +157,15 @@ struct drm_i915_gem_object {
 	unsigned int pin_global;
 
 	struct {
-		struct mutex lock; /* protects the pages and their use */
+		/*
+		 * Protects the pages and their use.
+		 *
+		 * IMPORTANT: It is not allowed to allocate memory while holding
+		 * this lock, because the shrinker might recurse on it, except
+		 * when there are no pages allocated yet and the object isn't
+		 * visible on any LRU.
+		 */
+		struct mutex lock;
 		atomic_t pages_pin_count;
 
 		struct sg_table *pages;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index 18f0ce0135c1..202526e8910f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -101,7 +101,7 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 {
 	int err;
 
-	err = mutex_lock_interruptible(&obj->mm.lock);
+	err = mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES);
 	if (err)
 		return err;
 
@@ -179,8 +179,7 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
 	return pages;
 }
 
-int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
-				enum i915_mm_subclass subclass)
+int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 {
 	struct sg_table *pages;
 	int err;
@@ -191,7 +190,7 @@ int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
 	GEM_BUG_ON(atomic_read(&obj->bind_count));
 
 	/* May be called by shrinker from within get_pages() (on another bo) */
-	mutex_lock_nested(&obj->mm.lock, subclass);
+	mutex_lock(&obj->mm.lock);
 	if (unlikely(atomic_read(&obj->mm.pages_pin_count))) {
 		err = -EBUSY;
 		goto unlock;
@@ -285,7 +284,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
 	if (unlikely(!i915_gem_object_has_struct_page(obj)))
 		return ERR_PTR(-ENXIO);
 
-	err = mutex_lock_interruptible(&obj->mm.lock);
+	err = mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES);
 	if (err)
 		return ERR_PTR(err);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index 102fd7a23d3d..209925be8a76 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -156,7 +156,7 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, int align)
 	if (err)
 		return err;
 
-	mutex_lock(&obj->mm.lock);
+	mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES);
 
 	if (obj->mm.madv != I915_MADV_WILLNEED) {
 		err = -EFAULT;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
index edd21d14e64f..0b0d6e27b996 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
@@ -98,7 +98,7 @@ static bool unsafe_drop_pages(struct drm_i915_gem_object *obj,
 		flags = I915_GEM_OBJECT_UNBIND_ACTIVE;
 
 	if (i915_gem_object_unbind(obj, flags) == 0)
-		__i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
+		__i915_gem_object_put_pages(obj);
 
 	return !i915_gem_object_has_pages(obj);
 }
@@ -254,8 +254,7 @@ i915_gem_shrink(struct drm_i915_private *i915,
 
 			if (unsafe_drop_pages(obj, shrink)) {
 				/* May arrive from get_pages on another bo */
-				mutex_lock_nested(&obj->mm.lock,
-						  I915_MM_SHRINKER);
+				mutex_lock(&obj->mm.lock);
 				if (!i915_gem_object_has_pages(obj)) {
 					try_to_writeback(obj, shrink);
 					count += obj->base.size >> PAGE_SHIFT;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 70dc506a5426..f3b3bc7c32cb 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -158,7 +158,7 @@ userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 		ret = i915_gem_object_unbind(obj,
 					     I915_GEM_OBJECT_UNBIND_ACTIVE);
 		if (ret == 0)
-			ret = __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
+			ret = __i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 		if (ret)
 			goto unlock;
@@ -514,7 +514,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 		}
 	}
 
-	mutex_lock(&obj->mm.lock);
+	mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES);
 	if (obj->userptr.work == &work->work) {
 		struct sg_table *pages = ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index 6cbd4a668c9a..df586035c33e 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -562,7 +562,7 @@ static int igt_mock_ppgtt_misaligned_dma(void *arg)
 		i915_vma_close(vma);
 
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 
@@ -590,7 +590,7 @@ static void close_object_list(struct list_head *objects,
 
 		list_del(&obj->st_link);
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 }
@@ -860,7 +860,7 @@ static int igt_mock_ppgtt_64K(void *arg)
 			i915_vma_close(vma);
 
 			i915_gem_object_unpin_pages(obj);
-			__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+			__i915_gem_object_put_pages(obj);
 			i915_gem_object_put(obj);
 		}
 	}
@@ -1268,7 +1268,7 @@ static int igt_ppgtt_exhaust_huge(void *arg)
 			}
 
 			i915_gem_object_unpin_pages(obj);
-			__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+			__i915_gem_object_put_pages(obj);
 			i915_gem_object_put(obj);
 		}
 	}
@@ -1330,7 +1330,7 @@ static int igt_ppgtt_internal_huge(void *arg)
 		}
 
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 
@@ -1399,7 +1399,7 @@ static int igt_ppgtt_gemfs_huge(void *arg)
 		}
 
 		i915_gem_object_unpin_pages(obj);
-		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
+		__i915_gem_object_put_pages(obj);
 		i915_gem_object_put(obj);
 	}
 
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex (rev3)
URL   : https://patchwork.freedesktop.org/series/65177/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
68f7d12ba111 drm/i915: Comment userptr recursion on struct_mutex
-:30: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'

total: 0 errors, 1 warnings, 0 checks, 12 lines checked
85d9fbe5cdd8 RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head
-:88: WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#88: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:287:
+	 * struct_mutex in the entire system. */

-:282: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'

total: 0 errors, 2 warnings, 0 checks, 170 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex (rev3)
URL   : https://patchwork.freedesktop.org/series/65177/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6709 -> Patchwork_14017
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/

Known issues
------------

  Here are the changes found in Patchwork_14017 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      [PASS][1] -> [FAIL][2] ([fdo#108511])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       [PASS][3] -> [SKIP][4] ([fdo#109271] / [fdo#109278]) +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       [INCOMPLETE][5] ([fdo#107718]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@i915_selftest@live_sanitycheck:
    - fi-icl-u3:          [DMESG-WARN][7] ([fdo#107724]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/fi-icl-u3/igt@i915_selftest@live_sanitycheck.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/fi-icl-u3/igt@i915_selftest@live_sanitycheck.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7567u:       [FAIL][9] ([fdo#109485]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/fi-kbl-7567u/igt@kms_chamelium@hdmi-hpd-fast.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/fi-kbl-7567u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-guc}:       [FAIL][11] ([fdo#103167]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/fi-icl-guc/igt@kms_frontbuffer_tracking@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/fi-icl-guc/igt@kms_frontbuffer_tracking@basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (53 -> 45)
------------------------------

  Additional (1): fi-kbl-guc 
  Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus fi-cml-u 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6709 -> Patchwork_14017

  CI-20190529: 20190529
  CI_DRM_6709: 4c9976607118e10dfc9f9feb3b9be2b3579631c9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5134: 81df2f22385bc275975cf199d962eed9bc10f916 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14017: 85d9fbe5cdd84f806610507651c7f27e133afc65 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

85d9fbe5cdd8 RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head
68f7d12ba111 drm/i915: Comment userptr recursion on struct_mutex

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Chris Wilson]

Quoting Daniel Vetter (2019-08-15 08:23:01)
> On Wed, Aug 14, 2019 at 07:57:57PM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2019-08-14 19:49:41)
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > > index d474c6ac4100..1ea3c3c96a5a 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > > @@ -157,7 +157,15 @@ struct drm_i915_gem_object {
> > >         unsigned int pin_global;
> > >  
> > >         struct {
> > > -               struct mutex lock; /* protects the pages and their use */
> > > +               /*
> > > +                * Protects the pages and their use.
> > 
> > "Their use" is still a misleading suggest of mine. This should be
> > "protects the pinning of pages". The couple of other things it is used
> > for are tied to the concept of the pages being pinned; further use should
> > be discouraged; direct use prohibited.
> 
> So something like "Protects the pinning of pages. Do not use directly, but
> instead go through the get/put_pages and pin/unpin interfaces." There's a
> few too many of those to list them all imo ...

I would say only pin/unpin. get/put is the for implementation.

Fwiw, elsewhere we adopted the name pin_mutex / pin_count. But there's
also variations on that naming scheme. The pattern of "that who is being
pinned may unpin others" is repeating.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Comment userptr recursion on struct_mutex (rev3)
URL   : https://patchwork.freedesktop.org/series/65177/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6709_full -> Patchwork_14017_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_14017_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@drm_import_export@flink:
    - shard-apl:          [PASS][1] -> [INCOMPLETE][2] ([fdo#103927])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-apl6/igt@drm_import_export@flink.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-apl2/igt@drm_import_export@flink.html

  * igt@gem_exec_schedule@preempt-contexts-bsd2:
    - shard-iclb:         [PASS][3] -> [SKIP][4] ([fdo#109276]) +15 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb2/igt@gem_exec_schedule@preempt-contexts-bsd2.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb3/igt@gem_exec_schedule@preempt-contexts-bsd2.html

  * igt@gem_exec_schedule@preempt-other-chain-bsd:
    - shard-iclb:         [PASS][5] -> [SKIP][6] ([fdo#111325]) +4 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb5/igt@gem_exec_schedule@preempt-other-chain-bsd.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb2/igt@gem_exec_schedule@preempt-other-chain-bsd.html

  * igt@gem_pwrite@huge-cpu-fbr:
    - shard-hsw:          [PASS][7] -> [INCOMPLETE][8] ([fdo#103540])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-hsw8/igt@gem_pwrite@huge-cpu-fbr.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-hsw8/igt@gem_pwrite@huge-cpu-fbr.html

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          [PASS][9] -> [DMESG-WARN][10] ([fdo#108566]) +2 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-apl8/igt@gem_softpin@noreloc-s3.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-apl4/igt@gem_softpin@noreloc-s3.html

  * igt@gem_sync@basic-many-each:
    - shard-kbl:          [PASS][11] -> [INCOMPLETE][12] ([fdo#103665])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-kbl3/igt@gem_sync@basic-many-each.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-kbl1/igt@gem_sync@basic-many-each.html

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-iclb:         [PASS][13] -> [INCOMPLETE][14] ([fdo#107713])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb4/igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb7/igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-skl:          [PASS][15] -> [INCOMPLETE][16] ([fdo#110741])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-skl5/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-skl1/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_cursor_legacy@cursor-vs-flip-atomic:
    - shard-hsw:          [PASS][17] -> [FAIL][18] ([fdo#103355])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-hsw5/igt@kms_cursor_legacy@cursor-vs-flip-atomic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-hsw6/igt@kms_cursor_legacy@cursor-vs-flip-atomic.html

  * igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
    - shard-iclb:         [PASS][19] -> [FAIL][20] ([fdo#103167]) +6 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [PASS][21] -> [FAIL][22] ([fdo#108145])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-skl4/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-skl3/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_psr@psr2_cursor_plane_move:
    - shard-iclb:         [PASS][23] -> [SKIP][24] ([fdo#109441]) +3 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb2/igt@kms_psr@psr2_cursor_plane_move.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb3/igt@kms_psr@psr2_cursor_plane_move.html

  * igt@kms_setmode@basic:
    - shard-apl:          [PASS][25] -> [FAIL][26] ([fdo#99912])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-apl4/igt@kms_setmode@basic.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-apl1/igt@kms_setmode@basic.html

  * igt@perf@polling:
    - shard-skl:          [PASS][27] -> [FAIL][28] ([fdo#110728])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-skl6/igt@perf@polling.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-skl9/igt@perf@polling.html

  
#### Possible fixes ####

  * igt@gem_ctx_shared@exec-single-timeline-bsd:
    - shard-iclb:         [SKIP][29] ([fdo#110841]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb4/igt@gem_ctx_shared@exec-single-timeline-bsd.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb8/igt@gem_ctx_shared@exec-single-timeline-bsd.html

  * igt@gem_exec_schedule@reorder-wide-bsd:
    - shard-iclb:         [SKIP][31] ([fdo#111325]) -> [PASS][32] +5 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb1/igt@gem_exec_schedule@reorder-wide-bsd.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb5/igt@gem_exec_schedule@reorder-wide-bsd.html

  * igt@kms_cursor_legacy@cursor-vs-flip-varying-size:
    - shard-apl:          [INCOMPLETE][33] ([fdo#103927]) -> [PASS][34] +1 similar issue
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-apl4/igt@kms_cursor_legacy@cursor-vs-flip-varying-size.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-apl7/igt@kms_cursor_legacy@cursor-vs-flip-varying-size.html

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-pwrite:
    - shard-iclb:         [FAIL][35] ([fdo#103167]) -> [PASS][36] +3 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-pwrite.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-pwrite.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
    - shard-apl:          [DMESG-WARN][37] ([fdo#108566]) -> [PASS][38] +2 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-apl3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-apl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          [FAIL][39] ([fdo#108145]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-skl2/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-skl9/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][41] ([fdo#108145] / [fdo#110403]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-skl8/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-iclb:         [FAIL][43] ([fdo#103166]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-y.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-y.html

  * igt@kms_psr@psr2_sprite_render:
    - shard-iclb:         [SKIP][45] ([fdo#109441]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb5/igt@kms_psr@psr2_sprite_render.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb2/igt@kms_psr@psr2_sprite_render.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][47] ([fdo#99912]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-kbl1/igt@kms_setmode@basic.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-kbl7/igt@kms_setmode@basic.html

  * igt@perf@blocking:
    - shard-skl:          [FAIL][49] ([fdo#110728]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-skl9/igt@perf@blocking.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-skl9/igt@perf@blocking.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [SKIP][51] ([fdo#109276]) -> [PASS][52] +11 similar issues
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb7/igt@prime_busy@hang-bsd2.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb4/igt@prime_busy@hang-bsd2.html

  
#### Warnings ####

  * igt@gem_mocs_settings@mocs-isolation-bsd2:
    - shard-iclb:         [SKIP][53] ([fdo#109276]) -> [FAIL][54] ([fdo#111330]) +1 similar issue
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb7/igt@gem_mocs_settings@mocs-isolation-bsd2.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb4/igt@gem_mocs_settings@mocs-isolation-bsd2.html

  * igt@gem_mocs_settings@mocs-settings-bsd2:
    - shard-iclb:         [FAIL][55] ([fdo#111330]) -> [SKIP][56] ([fdo#109276])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb1/igt@gem_mocs_settings@mocs-settings-bsd2.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb5/igt@gem_mocs_settings@mocs-settings-bsd2.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [DMESG-WARN][57] ([fdo#107724]) -> [SKIP][58] ([fdo#109349])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-iclb3/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-fullscreen:
    - shard-apl:          [INCOMPLETE][59] ([fdo#103927]) -> [SKIP][60] ([fdo#109271])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6709/shard-apl2/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-fullscreen.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/shard-apl3/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-fullscreen.html

  
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
  [fdo#110741]: https://bugs.freedesktop.org/show_bug.cgi?id=110741
  [fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841
  [fdo#111325]: https://bugs.freedesktop.org/show_bug.cgi?id=111325
  [fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6709 -> Patchwork_14017

  CI-20190529: 20190529
  CI_DRM_6709: 4c9976607118e10dfc9f9feb3b9be2b3579631c9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5134: 81df2f22385bc275975cf199d962eed9bc10f916 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14017: 85d9fbe5cdd84f806610507651c7f27e133afc65 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14017/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Tang, CQ]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Wednesday, August 14, 2019 12:25 PM
> To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>; Vetter, Daniel
> <daniel.vetter@intel.com>
> Subject: [Intel-gfx] [PATCH] RFC: drm/i915: Switch obj->mm.lock lockdep
> annotations on its head
> 
> The trouble with having a plain nesting flag for locks which do not naturally
> nest (unlike block devices and their partitions, which is the original motivation
> for nesting levels) is that lockdep will never spot a true deadlock if you screw
> up.
> 
> This patch is an attempt at trying better, by highlighting a bit more the actual
> nature of the nesting that's going on. Essentially we have two kinds of
> objects:
> 
> - objects without pages allocated, which cannot be on any lru and are
>   hence inaccessible to the shrinker.
> 
> - objects which have pages allocated, which are on an lru, and which
>   the shrinker can decide to throw out.
> 
> For the former type of object, memory allcoations while holding
> obj->mm.lock are permissible. For the latter they are not. And
> get/put_pages transitions between the two types of objects.
> 
> This is still not entirely fool-proof since the rules might chance.
> But as long as we run such a code ever at runtime lockdep should be able to
> observe the inconsistency and complain (like with any other lockdep class
> that we've split up in multiple classes). But there are a few clear benefits:
> 
> - We can drop the nesting flag parameter from
>   __i915_gem_object_put_pages, because that function by definition is
>   never going allocate memory, and calling it on an object which
>   doesn't have its pages allocated would be a bug.
> 
> - We strictly catch more bugs, since there's not only one place in the
>   entire tree which is annotated with the special class. All the
>   other places that had explicit lockdep nesting annotations we're now
>   going to leave up to lockdep again.
> 
> - Specifically this catches stuff like calling get_pages from
>   put_pages (which isn't really a good idea, if we can call put_pages
>   so could the shrinker). I've seen patches do exactly that.
> 
> Of course I fully expect CI will show me for the fool I am with this one here :-)
> 
> v2: There can only be one (lockdep only has a cache for the first subclass, not
> for deeper ones, and we don't want to make these locks even slower). Still
> separate enums for better documentation.
> 
> Real fix: don forget about phys objs and pin_map(), and fix the shrinker to
> have the right annotations ... silly me.
> 
> v3: Forgot usertptr too ...

I eventually looked this patch. My question is on the shrinking calling stack:

Pin_pages(A)-->get_page(A)-->lock(objA->mm.lock, I915_MM_GET_PAGES)-->i915_gem_shrink()-->
Lock(struct_mutex)-->put_pages(B)-->lock(objB->mm.lock)....

objA is locked with: mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES);

objB is locked with: mutex_lock(&obj->mm.lock);
My understanding is that objB locking is equivalently to:
mutex_lock_nested(&obj->mm.lock, I915_MM_NORMAL);

so you lock subclass=2 first on A, then lock subclass=0 next B, the reverse order.
Doesn't this cause a lockdep warning?   

--CQ


> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_object.c       |  2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_object.h       | 16 +++++++++++++---
>  drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 10 +++++++++-
>  drivers/gpu/drm/i915/gem/i915_gem_pages.c        |  9 ++++-----
>  drivers/gpu/drm/i915/gem/i915_gem_phys.c         |  2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_shrinker.c     |  5 ++---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c      |  4 ++--
>  drivers/gpu/drm/i915/gem/selftests/huge_pages.c  | 12 ++++++------
>  8 files changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 3929c3a6b281..a1a835539e09 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -191,7 +191,7 @@ static void __i915_gem_free_objects(struct
> drm_i915_private *i915,
>  		GEM_BUG_ON(!list_empty(&obj->lut_list));
> 
>  		atomic_set(&obj->mm.pages_pin_count, 0);
> -		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> +		__i915_gem_object_put_pages(obj);
>  		GEM_BUG_ON(i915_gem_object_has_pages(obj));
>  		bitmap_free(obj->bit_17);
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index 3714cf234d64..5ce511ca7fa8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -281,11 +281,21 @@ i915_gem_object_unpin_pages(struct
> drm_i915_gem_object *obj)
> 
>  enum i915_mm_subclass { /* lockdep subclass for obj-
> >mm.lock/struct_mutex */
>  	I915_MM_NORMAL = 0,
> -	I915_MM_SHRINKER /* called "recursively" from direct-reclaim-
> esque */
> +	/*
> +	 * Only used by struct_mutex, when called "recursively" from
> +	 * direct-reclaim-esque. Safe because there is only every one
> +	 * struct_mutex in the entire system. */
> +	I915_MM_SHRINKER = 1,
> +	/*
> +	 * Used for obj->mm.lock when allocating pages. Safe because the
> object
> +	 * isn't yet on any LRU, and therefore the shrinker can't deadlock on
> +	 * it. As soon as the object has pages, obj->mm.lock nests within
> +	 * fs_reclaim.
> +	 */
> +	I915_MM_GET_PAGES = 1,
>  };
> 
> -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> -				enum i915_mm_subclass subclass);
> +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>  void i915_gem_object_truncate(struct drm_i915_gem_object *obj);  void
> i915_gem_object_writeback(struct drm_i915_gem_object *obj);
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> index d474c6ac4100..1ea3c3c96a5a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -157,7 +157,15 @@ struct drm_i915_gem_object {
>  	unsigned int pin_global;
> 
>  	struct {
> -		struct mutex lock; /* protects the pages and their use */
> +		/*
> +		 * Protects the pages and their use.
> +		 *
> +		 * IMPORTANT: It is not allowed to allocate memory while
> holding
> +		 * this lock, because the shrinker might recurse on it, except
> +		 * when there are no pages allocated yet and the object isn't
> +		 * visible on any LRU.
> +		 */
> +		struct mutex lock;
>  		atomic_t pages_pin_count;
> 
>  		struct sg_table *pages;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index 18f0ce0135c1..202526e8910f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -101,7 +101,7 @@ int __i915_gem_object_get_pages(struct
> drm_i915_gem_object *obj)  {
>  	int err;
> 
> -	err = mutex_lock_interruptible(&obj->mm.lock);
> +	err = mutex_lock_interruptible_nested(&obj->mm.lock,
> +I915_MM_GET_PAGES);
>  	if (err)
>  		return err;
> 
> @@ -179,8 +179,7 @@ __i915_gem_object_unset_pages(struct
> drm_i915_gem_object *obj)
>  	return pages;
>  }
> 
> -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> -				enum i915_mm_subclass subclass)
> +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>  {
>  	struct sg_table *pages;
>  	int err;
> @@ -191,7 +190,7 @@ int __i915_gem_object_put_pages(struct
> drm_i915_gem_object *obj,
>  	GEM_BUG_ON(atomic_read(&obj->bind_count));
> 
>  	/* May be called by shrinker from within get_pages() (on another bo)
> */
> -	mutex_lock_nested(&obj->mm.lock, subclass);
> +	mutex_lock(&obj->mm.lock);
>  	if (unlikely(atomic_read(&obj->mm.pages_pin_count))) {
>  		err = -EBUSY;
>  		goto unlock;
> @@ -285,7 +284,7 @@ void *i915_gem_object_pin_map(struct
> drm_i915_gem_object *obj,
>  	if (unlikely(!i915_gem_object_has_struct_page(obj)))
>  		return ERR_PTR(-ENXIO);
> 
> -	err = mutex_lock_interruptible(&obj->mm.lock);
> +	err = mutex_lock_interruptible_nested(&obj->mm.lock,
> +I915_MM_GET_PAGES);
>  	if (err)
>  		return ERR_PTR(err);
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> index 102fd7a23d3d..209925be8a76 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> @@ -156,7 +156,7 @@ int i915_gem_object_attach_phys(struct
> drm_i915_gem_object *obj, int align)
>  	if (err)
>  		return err;
> 
> -	mutex_lock(&obj->mm.lock);
> +	mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES);
> 
>  	if (obj->mm.madv != I915_MADV_WILLNEED) {
>  		err = -EFAULT;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> index edd21d14e64f..0b0d6e27b996 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> @@ -98,7 +98,7 @@ static bool unsafe_drop_pages(struct
> drm_i915_gem_object *obj,
>  		flags = I915_GEM_OBJECT_UNBIND_ACTIVE;
> 
>  	if (i915_gem_object_unbind(obj, flags) == 0)
> -		__i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
> +		__i915_gem_object_put_pages(obj);
> 
>  	return !i915_gem_object_has_pages(obj);  } @@ -254,8 +254,7 @@
> i915_gem_shrink(struct drm_i915_private *i915,
> 
>  			if (unsafe_drop_pages(obj, shrink)) {
>  				/* May arrive from get_pages on another bo
> */
> -				mutex_lock_nested(&obj->mm.lock,
> -						  I915_MM_SHRINKER);
> +				mutex_lock(&obj->mm.lock);
>  				if (!i915_gem_object_has_pages(obj)) {
>  					try_to_writeback(obj, shrink);
>  					count += obj->base.size >>
> PAGE_SHIFT; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 70dc506a5426..f3b3bc7c32cb 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -158,7 +158,7 @@ userptr_mn_invalidate_range_start(struct
> mmu_notifier *_mn,
>  		ret = i915_gem_object_unbind(obj,
> 
> I915_GEM_OBJECT_UNBIND_ACTIVE);
>  		if (ret == 0)
> -			ret = __i915_gem_object_put_pages(obj,
> I915_MM_SHRINKER);
> +			ret = __i915_gem_object_put_pages(obj);
>  		i915_gem_object_put(obj);
>  		if (ret)
>  			goto unlock;
> @@ -514,7 +514,7 @@ __i915_gem_userptr_get_pages_worker(struct
> work_struct *_work)
>  		}
>  	}
> 
> -	mutex_lock(&obj->mm.lock);
> +	mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES);
>  	if (obj->userptr.work == &work->work) {
>  		struct sg_table *pages = ERR_PTR(ret);
> 
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> index 6cbd4a668c9a..df586035c33e 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> @@ -562,7 +562,7 @@ static int igt_mock_ppgtt_misaligned_dma(void *arg)
>  		i915_vma_close(vma);
> 
>  		i915_gem_object_unpin_pages(obj);
> -		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> +		__i915_gem_object_put_pages(obj);
>  		i915_gem_object_put(obj);
>  	}
> 
> @@ -590,7 +590,7 @@ static void close_object_list(struct list_head *objects,
> 
>  		list_del(&obj->st_link);
>  		i915_gem_object_unpin_pages(obj);
> -		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> +		__i915_gem_object_put_pages(obj);
>  		i915_gem_object_put(obj);
>  	}
>  }
> @@ -860,7 +860,7 @@ static int igt_mock_ppgtt_64K(void *arg)
>  			i915_vma_close(vma);
> 
>  			i915_gem_object_unpin_pages(obj);
> -			__i915_gem_object_put_pages(obj,
> I915_MM_NORMAL);
> +			__i915_gem_object_put_pages(obj);
>  			i915_gem_object_put(obj);
>  		}
>  	}
> @@ -1268,7 +1268,7 @@ static int igt_ppgtt_exhaust_huge(void *arg)
>  			}
> 
>  			i915_gem_object_unpin_pages(obj);
> -			__i915_gem_object_put_pages(obj,
> I915_MM_NORMAL);
> +			__i915_gem_object_put_pages(obj);
>  			i915_gem_object_put(obj);
>  		}
>  	}
> @@ -1330,7 +1330,7 @@ static int igt_ppgtt_internal_huge(void *arg)
>  		}
> 
>  		i915_gem_object_unpin_pages(obj);
> -		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> +		__i915_gem_object_put_pages(obj);
>  		i915_gem_object_put(obj);
>  	}
> 
> @@ -1399,7 +1399,7 @@ static int igt_ppgtt_gemfs_huge(void *arg)
>  		}
> 
>  		i915_gem_object_unpin_pages(obj);
> -		__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> +		__i915_gem_object_put_pages(obj);
>  		i915_gem_object_put(obj);
>  	}
> 
> --
> 2.22.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] RFC: drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Daniel Vetter]

On Thu, Aug 15, 2019 at 9:35 PM Tang, CQ <cq.tang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of Daniel Vetter
> > Sent: Wednesday, August 14, 2019 12:25 PM
> > To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>; Vetter, Daniel
> > <daniel.vetter@intel.com>
> > Subject: [Intel-gfx] [PATCH] RFC: drm/i915: Switch obj->mm.lock lockdep
> > annotations on its head
> >
> > The trouble with having a plain nesting flag for locks which do not naturally
> > nest (unlike block devices and their partitions, which is the original motivation
> > for nesting levels) is that lockdep will never spot a true deadlock if you screw
> > up.
> >
> > This patch is an attempt at trying better, by highlighting a bit more the actual
> > nature of the nesting that's going on. Essentially we have two kinds of
> > objects:
> >
> > - objects without pages allocated, which cannot be on any lru and are
> >   hence inaccessible to the shrinker.
> >
> > - objects which have pages allocated, which are on an lru, and which
> >   the shrinker can decide to throw out.
> >
> > For the former type of object, memory allcoations while holding
> > obj->mm.lock are permissible. For the latter they are not. And
> > get/put_pages transitions between the two types of objects.
> >
> > This is still not entirely fool-proof since the rules might chance.
> > But as long as we run such a code ever at runtime lockdep should be able to
> > observe the inconsistency and complain (like with any other lockdep class
> > that we've split up in multiple classes). But there are a few clear benefits:
> >
> > - We can drop the nesting flag parameter from
> >   __i915_gem_object_put_pages, because that function by definition is
> >   never going allocate memory, and calling it on an object which
> >   doesn't have its pages allocated would be a bug.
> >
> > - We strictly catch more bugs, since there's not only one place in the
> >   entire tree which is annotated with the special class. All the
> >   other places that had explicit lockdep nesting annotations we're now
> >   going to leave up to lockdep again.
> >
> > - Specifically this catches stuff like calling get_pages from
> >   put_pages (which isn't really a good idea, if we can call put_pages
> >   so could the shrinker). I've seen patches do exactly that.
> >
> > Of course I fully expect CI will show me for the fool I am with this one here :-)
> >
> > v2: There can only be one (lockdep only has a cache for the first subclass, not
> > for deeper ones, and we don't want to make these locks even slower). Still
> > separate enums for better documentation.
> >
> > Real fix: don forget about phys objs and pin_map(), and fix the shrinker to
> > have the right annotations ... silly me.
> >
> > v3: Forgot usertptr too ...
>
> I eventually looked this patch. My question is on the shrinking calling stack:
>
> Pin_pages(A)-->get_page(A)-->lock(objA->mm.lock, I915_MM_GET_PAGES)-->i915_gem_shrink()-->
> Lock(struct_mutex)-->put_pages(B)-->lock(objB->mm.lock)....
>
> objA is locked with: mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES);
>
> objB is locked with: mutex_lock(&obj->mm.lock);
> My understanding is that objB locking is equivalently to:
> mutex_lock_nested(&obj->mm.lock, I915_MM_NORMAL);
>
> so you lock subclass=2 first on A, then lock subclass=0 next B, the reverse order.
> Doesn't this cause a lockdep warning?

So mutex_lock_nested really is misnamed, it should be called
mutex_lock_subclass. There's no guarantees/promises about a certain
nesting, just that lockdep should use the 1st/2nd/... subclass of the
lockdep class the lock uses (there's a limit of 8 total, including the
main/default subclass). From lockdep's pov a subclass works exactly
like allocating a new lockdep key. But with the difference that you
can change it at runtime, i.e. when an object makes a state
transition, you can change the subclass you're using. Like this patch
tries to do here.

But ofc like everytime you're creating a new lockdep class, you need
to spend a few words on why it's not hiding real issues from lockdep.
-Daniel

> --CQ
>
>
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_object.c       |  2 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_object.h       | 16 +++++++++++++---
> >  drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 10 +++++++++-
> >  drivers/gpu/drm/i915/gem/i915_gem_pages.c        |  9 ++++-----
> >  drivers/gpu/drm/i915/gem/i915_gem_phys.c         |  2 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_shrinker.c     |  5 ++---
> >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c      |  4 ++--
> >  drivers/gpu/drm/i915/gem/selftests/huge_pages.c  | 12 ++++++------
> >  8 files changed, 38 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > index 3929c3a6b281..a1a835539e09 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -191,7 +191,7 @@ static void __i915_gem_free_objects(struct
> > drm_i915_private *i915,
> >               GEM_BUG_ON(!list_empty(&obj->lut_list));
> >
> >               atomic_set(&obj->mm.pages_pin_count, 0);
> > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > +             __i915_gem_object_put_pages(obj);
> >               GEM_BUG_ON(i915_gem_object_has_pages(obj));
> >               bitmap_free(obj->bit_17);
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > index 3714cf234d64..5ce511ca7fa8 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > @@ -281,11 +281,21 @@ i915_gem_object_unpin_pages(struct
> > drm_i915_gem_object *obj)
> >
> >  enum i915_mm_subclass { /* lockdep subclass for obj-
> > >mm.lock/struct_mutex */
> >       I915_MM_NORMAL = 0,
> > -     I915_MM_SHRINKER /* called "recursively" from direct-reclaim-
> > esque */
> > +     /*
> > +      * Only used by struct_mutex, when called "recursively" from
> > +      * direct-reclaim-esque. Safe because there is only every one
> > +      * struct_mutex in the entire system. */
> > +     I915_MM_SHRINKER = 1,
> > +     /*
> > +      * Used for obj->mm.lock when allocating pages. Safe because the
> > object
> > +      * isn't yet on any LRU, and therefore the shrinker can't deadlock on
> > +      * it. As soon as the object has pages, obj->mm.lock nests within
> > +      * fs_reclaim.
> > +      */
> > +     I915_MM_GET_PAGES = 1,
> >  };
> >
> > -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > -                             enum i915_mm_subclass subclass);
> > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
> >  void i915_gem_object_truncate(struct drm_i915_gem_object *obj);  void
> > i915_gem_object_writeback(struct drm_i915_gem_object *obj);
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > index d474c6ac4100..1ea3c3c96a5a 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > @@ -157,7 +157,15 @@ struct drm_i915_gem_object {
> >       unsigned int pin_global;
> >
> >       struct {
> > -             struct mutex lock; /* protects the pages and their use */
> > +             /*
> > +              * Protects the pages and their use.
> > +              *
> > +              * IMPORTANT: It is not allowed to allocate memory while
> > holding
> > +              * this lock, because the shrinker might recurse on it, except
> > +              * when there are no pages allocated yet and the object isn't
> > +              * visible on any LRU.
> > +              */
> > +             struct mutex lock;
> >               atomic_t pages_pin_count;
> >
> >               struct sg_table *pages;
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > index 18f0ce0135c1..202526e8910f 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > @@ -101,7 +101,7 @@ int __i915_gem_object_get_pages(struct
> > drm_i915_gem_object *obj)  {
> >       int err;
> >
> > -     err = mutex_lock_interruptible(&obj->mm.lock);
> > +     err = mutex_lock_interruptible_nested(&obj->mm.lock,
> > +I915_MM_GET_PAGES);
> >       if (err)
> >               return err;
> >
> > @@ -179,8 +179,7 @@ __i915_gem_object_unset_pages(struct
> > drm_i915_gem_object *obj)
> >       return pages;
> >  }
> >
> > -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> > -                             enum i915_mm_subclass subclass)
> > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
> >  {
> >       struct sg_table *pages;
> >       int err;
> > @@ -191,7 +190,7 @@ int __i915_gem_object_put_pages(struct
> > drm_i915_gem_object *obj,
> >       GEM_BUG_ON(atomic_read(&obj->bind_count));
> >
> >       /* May be called by shrinker from within get_pages() (on another bo)
> > */
> > -     mutex_lock_nested(&obj->mm.lock, subclass);
> > +     mutex_lock(&obj->mm.lock);
> >       if (unlikely(atomic_read(&obj->mm.pages_pin_count))) {
> >               err = -EBUSY;
> >               goto unlock;
> > @@ -285,7 +284,7 @@ void *i915_gem_object_pin_map(struct
> > drm_i915_gem_object *obj,
> >       if (unlikely(!i915_gem_object_has_struct_page(obj)))
> >               return ERR_PTR(-ENXIO);
> >
> > -     err = mutex_lock_interruptible(&obj->mm.lock);
> > +     err = mutex_lock_interruptible_nested(&obj->mm.lock,
> > +I915_MM_GET_PAGES);
> >       if (err)
> >               return ERR_PTR(err);
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> > index 102fd7a23d3d..209925be8a76 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> > @@ -156,7 +156,7 @@ int i915_gem_object_attach_phys(struct
> > drm_i915_gem_object *obj, int align)
> >       if (err)
> >               return err;
> >
> > -     mutex_lock(&obj->mm.lock);
> > +     mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES);
> >
> >       if (obj->mm.madv != I915_MADV_WILLNEED) {
> >               err = -EFAULT;
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> > index edd21d14e64f..0b0d6e27b996 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> > @@ -98,7 +98,7 @@ static bool unsafe_drop_pages(struct
> > drm_i915_gem_object *obj,
> >               flags = I915_GEM_OBJECT_UNBIND_ACTIVE;
> >
> >       if (i915_gem_object_unbind(obj, flags) == 0)
> > -             __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
> > +             __i915_gem_object_put_pages(obj);
> >
> >       return !i915_gem_object_has_pages(obj);  } @@ -254,8 +254,7 @@
> > i915_gem_shrink(struct drm_i915_private *i915,
> >
> >                       if (unsafe_drop_pages(obj, shrink)) {
> >                               /* May arrive from get_pages on another bo
> > */
> > -                             mutex_lock_nested(&obj->mm.lock,
> > -                                               I915_MM_SHRINKER);
> > +                             mutex_lock(&obj->mm.lock);
> >                               if (!i915_gem_object_has_pages(obj)) {
> >                                       try_to_writeback(obj, shrink);
> >                                       count += obj->base.size >>
> > PAGE_SHIFT; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > index 70dc506a5426..f3b3bc7c32cb 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > @@ -158,7 +158,7 @@ userptr_mn_invalidate_range_start(struct
> > mmu_notifier *_mn,
> >               ret = i915_gem_object_unbind(obj,
> >
> > I915_GEM_OBJECT_UNBIND_ACTIVE);
> >               if (ret == 0)
> > -                     ret = __i915_gem_object_put_pages(obj,
> > I915_MM_SHRINKER);
> > +                     ret = __i915_gem_object_put_pages(obj);
> >               i915_gem_object_put(obj);
> >               if (ret)
> >                       goto unlock;
> > @@ -514,7 +514,7 @@ __i915_gem_userptr_get_pages_worker(struct
> > work_struct *_work)
> >               }
> >       }
> >
> > -     mutex_lock(&obj->mm.lock);
> > +     mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES);
> >       if (obj->userptr.work == &work->work) {
> >               struct sg_table *pages = ERR_PTR(ret);
> >
> > diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> > b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> > index 6cbd4a668c9a..df586035c33e 100644
> > --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> > +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> > @@ -562,7 +562,7 @@ static int igt_mock_ppgtt_misaligned_dma(void *arg)
> >               i915_vma_close(vma);
> >
> >               i915_gem_object_unpin_pages(obj);
> > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > +             __i915_gem_object_put_pages(obj);
> >               i915_gem_object_put(obj);
> >       }
> >
> > @@ -590,7 +590,7 @@ static void close_object_list(struct list_head *objects,
> >
> >               list_del(&obj->st_link);
> >               i915_gem_object_unpin_pages(obj);
> > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > +             __i915_gem_object_put_pages(obj);
> >               i915_gem_object_put(obj);
> >       }
> >  }
> > @@ -860,7 +860,7 @@ static int igt_mock_ppgtt_64K(void *arg)
> >                       i915_vma_close(vma);
> >
> >                       i915_gem_object_unpin_pages(obj);
> > -                     __i915_gem_object_put_pages(obj,
> > I915_MM_NORMAL);
> > +                     __i915_gem_object_put_pages(obj);
> >                       i915_gem_object_put(obj);
> >               }
> >       }
> > @@ -1268,7 +1268,7 @@ static int igt_ppgtt_exhaust_huge(void *arg)
> >                       }
> >
> >                       i915_gem_object_unpin_pages(obj);
> > -                     __i915_gem_object_put_pages(obj,
> > I915_MM_NORMAL);
> > +                     __i915_gem_object_put_pages(obj);
> >                       i915_gem_object_put(obj);
> >               }
> >       }
> > @@ -1330,7 +1330,7 @@ static int igt_ppgtt_internal_huge(void *arg)
> >               }
> >
> >               i915_gem_object_unpin_pages(obj);
> > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > +             __i915_gem_object_put_pages(obj);
> >               i915_gem_object_put(obj);
> >       }
> >
> > @@ -1399,7 +1399,7 @@ static int igt_ppgtt_gemfs_huge(void *arg)
> >               }
> >
> >               i915_gem_object_unpin_pages(obj);
> > -             __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> > +             __i915_gem_object_put_pages(obj);
> >               i915_gem_object_put(obj);
> >       }
> >
> > --
> > 2.22.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx