[RFC 0/4] Shrinker tweaks/fixes?

[RFC 0/4] Shrinker tweaks/fixes?

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A few patches to put my money where my mouth is regarding review comments where
I am suggesting to split up a patch for safety.

This ends one step short compared to the parent
(https://patchwork.freedesktop.org/patch/273593/) by omitting the final
mutex_lock_nested step, in order to see what kind of lockdep traces we get, or
if none for that to be a clear sign we need better tests.

Tvrtko Ursulin (4):
  drm/i915: Reduce recursive mutex locking from the shrinker
  drm/i915: Fix timeout handling in i915_gem_shrinker_vmap
  drm/i915: Return immediately if trylock fails for direct-reclaim
  drm/i915: Remove trylock-looping from the shrinker

 drivers/gpu/drm/i915/i915_drv.h          | 11 ++---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 51 +++++++++++-------------
 2 files changed, 29 insertions(+), 33 deletions(-)

-- 
2.19.1

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

[RFC 1/4] drm/i915: Reduce recursive mutex locking from the shrinker

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In two codepaths internal to the shrinker we know we will end up taking
the resursive mutex path.

It instead feels more elegant to avoid this altogether and not call
mutex_trylock_recursive in those cases.

We achieve this by adding a new I915_SHRINK_LOCKED flag which gets passed
to i915_gem_shrink by the internal callers.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          | 11 ++++++-----
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 18 ++++++++----------
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7fa2a405c5fe..63e0951561fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3179,11 +3179,12 @@ unsigned long i915_gem_shrink(struct drm_i915_private *i915,
 			      unsigned long target,
 			      unsigned long *nr_scanned,
 			      unsigned flags);
-#define I915_SHRINK_PURGEABLE 0x1
-#define I915_SHRINK_UNBOUND 0x2
-#define I915_SHRINK_BOUND 0x4
-#define I915_SHRINK_ACTIVE 0x8
-#define I915_SHRINK_VMAPS 0x10
+#define I915_SHRINK_LOCKED	BIT(0)
+#define I915_SHRINK_PURGEABLE	BIT(1)
+#define I915_SHRINK_UNBOUND	BIT(2)
+#define I915_SHRINK_BOUND	BIT(3)
+#define I915_SHRINK_ACTIVE	BIT(4)
+#define I915_SHRINK_VMAPS	BIT(5)
 unsigned long i915_gem_shrink_all(struct drm_i915_private *i915);
 void i915_gem_shrinker_register(struct drm_i915_private *i915);
 void i915_gem_shrinker_unregister(struct drm_i915_private *i915);
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index ea90d3a0d511..d07ee5921e5e 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -158,9 +158,9 @@ i915_gem_shrink(struct drm_i915_private *i915,
 	}, *phase;
 	unsigned long count = 0;
 	unsigned long scanned = 0;
-	bool unlock;
+	bool unlock = false;
 
-	if (!shrinker_lock(i915, &unlock))
+	if (!(flags & I915_SHRINK_LOCKED) && !shrinker_lock(i915, &unlock))
 		return 0;
 
 	/*
@@ -352,6 +352,8 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 {
 	struct drm_i915_private *i915 =
 		container_of(shrinker, struct drm_i915_private, mm.shrinker);
+	const unsigned flags =
+		I915_SHRINK_LOCKED | I915_SHRINK_BOUND | I915_SHRINK_UNBOUND;
 	unsigned long freed;
 	bool unlock;
 
@@ -363,23 +365,18 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 	freed = i915_gem_shrink(i915,
 				sc->nr_to_scan,
 				&sc->nr_scanned,
-				I915_SHRINK_BOUND |
-				I915_SHRINK_UNBOUND |
-				I915_SHRINK_PURGEABLE);
+				flags | I915_SHRINK_PURGEABLE);
 	if (sc->nr_scanned < sc->nr_to_scan)
 		freed += i915_gem_shrink(i915,
 					 sc->nr_to_scan - sc->nr_scanned,
 					 &sc->nr_scanned,
-					 I915_SHRINK_BOUND |
-					 I915_SHRINK_UNBOUND);
+					 flags);
 	if (sc->nr_scanned < sc->nr_to_scan && current_is_kswapd()) {
 		intel_runtime_pm_get(i915);
 		freed += i915_gem_shrink(i915,
 					 sc->nr_to_scan - sc->nr_scanned,
 					 &sc->nr_scanned,
-					 I915_SHRINK_ACTIVE |
-					 I915_SHRINK_BOUND |
-					 I915_SHRINK_UNBOUND);
+					 flags | I915_SHRINK_ACTIVE);
 		intel_runtime_pm_put(i915);
 	}
 
@@ -478,6 +475,7 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 
 	intel_runtime_pm_get(i915);
 	freed_pages += i915_gem_shrink(i915, -1UL, NULL,
+				       I915_SHRINK_LOCKED |
 				       I915_SHRINK_BOUND |
 				       I915_SHRINK_UNBOUND |
 				       I915_SHRINK_ACTIVE |
-- 
2.19.1

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

[RFC 2/4] drm/i915: Fix timeout handling in i915_gem_shrinker_vmap

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

The code tries to grab struct mutex for 5ms every time the unlocked GPU
idle wait succeeds. But the GPU idle wait itself is practically unbound
which means the 5ms timeout might not be honoured.

Cap the GPU idle wait to 5ms as well to fix this.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d07ee5921e5e..586acf02727e 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -387,13 +387,12 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 
 static bool
 shrinker_lock_uninterruptible(struct drm_i915_private *i915, bool *unlock,
-			      int timeout_ms)
+			      unsigned long timeout)
 {
-	unsigned long timeout = jiffies + msecs_to_jiffies_timeout(timeout_ms);
+	const unsigned long timeout_end = jiffies + timeout;
 
 	do {
-		if (i915_gem_wait_for_idle(i915,
-					   0, MAX_SCHEDULE_TIMEOUT) == 0 &&
+		if (i915_gem_wait_for_idle(i915, 0, timeout) == 0 &&
 		    shrinker_lock(i915, unlock))
 			break;
 
@@ -401,7 +400,7 @@ shrinker_lock_uninterruptible(struct drm_i915_private *i915, bool *unlock,
 		if (fatal_signal_pending(current))
 			return false;
 
-		if (time_after(jiffies, timeout)) {
+		if (time_after(jiffies, timeout_end)) {
 			pr_err("Unable to lock GPU to purge memory.\n");
 			return false;
 		}
@@ -459,11 +458,12 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	struct drm_i915_private *i915 =
 		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
 	struct i915_vma *vma, *next;
+	const unsigned long timeout = msecs_to_jiffies_timeout(5000);
 	unsigned long freed_pages = 0;
 	bool unlock;
 	int ret;
 
-	if (!shrinker_lock_uninterruptible(i915, &unlock, 5000))
+	if (!shrinker_lock_uninterruptible(i915, &unlock, timeout))
 		return NOTIFY_DONE;
 
 	/* Force everything onto the inactive lists */
-- 
2.19.1

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

[RFC 3/4] drm/i915: Return immediately if trylock fails for direct-reclaim

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In an equally named patch Chris Wilson proposes skipping the attempts to
obtain struct_mutex via trylock-looping in order to improve latency for
non-i915 clients. I quote:

"""
Ignore trying to shrink from i915 if we fail to acquire the struct_mutex
in the shrinker while performing direct-reclaim. The trade-off being
(much) lower latency for non-i915 clients at an increased risk of being
unable to obtain a page from direct-reclaim without hitting the
oom-notifier. The proviso being that we still keep trying to hard
obtain the lock for oom so that we can reap under heavy memory pressure.
"""

This version of the patch does strictly what the commit message explains
and leaves other changes out.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 30 ++++++++++++++----------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 586acf02727e..97deec775f40 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -36,7 +36,8 @@
 #include "i915_drv.h"
 #include "i915_trace.h"
 
-static bool shrinker_lock(struct drm_i915_private *i915, bool *unlock)
+static bool
+shrinker_lock(struct drm_i915_private *i915, unsigned int flags, bool *unlock)
 {
 	switch (mutex_trylock_recursive(&i915->drm.struct_mutex)) {
 	case MUTEX_TRYLOCK_RECURSIVE:
@@ -45,15 +46,17 @@ static bool shrinker_lock(struct drm_i915_private *i915, bool *unlock)
 
 	case MUTEX_TRYLOCK_FAILED:
 		*unlock = false;
-		preempt_disable();
-		do {
-			cpu_relax();
-			if (mutex_trylock(&i915->drm.struct_mutex)) {
-				*unlock = true;
-				break;
-			}
-		} while (!need_resched());
-		preempt_enable();
+		if (flags & I915_SHRINK_ACTIVE) {
+			preempt_disable();
+			do {
+				cpu_relax();
+				if (mutex_trylock(&i915->drm.struct_mutex)) {
+					*unlock = true;
+					break;
+				}
+			} while (!need_resched());
+			preempt_enable();
+		}
 		return *unlock;
 
 	case MUTEX_TRYLOCK_SUCCESS:
@@ -160,7 +163,8 @@ i915_gem_shrink(struct drm_i915_private *i915,
 	unsigned long scanned = 0;
 	bool unlock = false;
 
-	if (!(flags & I915_SHRINK_LOCKED) && !shrinker_lock(i915, &unlock))
+	if (!(flags & I915_SHRINK_LOCKED) &&
+	    !shrinker_lock(i915, flags, &unlock))
 		return 0;
 
 	/*
@@ -359,7 +363,7 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 
 	sc->nr_scanned = 0;
 
-	if (!shrinker_lock(i915, &unlock))
+	if (!shrinker_lock(i915, flags, &unlock))
 		return SHRINK_STOP;
 
 	freed = i915_gem_shrink(i915,
@@ -393,7 +397,7 @@ shrinker_lock_uninterruptible(struct drm_i915_private *i915, bool *unlock,
 
 	do {
 		if (i915_gem_wait_for_idle(i915, 0, timeout) == 0 &&
-		    shrinker_lock(i915, unlock))
+		    shrinker_lock(i915, 0, unlock))
 			break;
 
 		schedule_timeout_killable(1);
-- 
2.19.1

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

[RFC 4/4] drm/i915: Remove trylock-looping from the shrinker

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In a patch named "drm/i915: Return immediately if trylock fails for
direct-reclaim" Chris Wilson implicitly suggests it is safe to use a plain
mutex_lock instead of trylock on the I915_SHRINK_ACTIVE code paths.

Explore this to see how the lockdep traces will look.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 97deec775f40..2d4a54851c0a 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -47,15 +47,8 @@ shrinker_lock(struct drm_i915_private *i915, unsigned int flags, bool *unlock)
 	case MUTEX_TRYLOCK_FAILED:
 		*unlock = false;
 		if (flags & I915_SHRINK_ACTIVE) {
-			preempt_disable();
-			do {
-				cpu_relax();
-				if (mutex_trylock(&i915->drm.struct_mutex)) {
-					*unlock = true;
-					break;
-				}
-			} while (!need_resched());
-			preempt_enable();
+			mutex_lock(&i915->drm.struct_mutex);
+			*unlock = true;
 		}
 		return *unlock;
 
-- 
2.19.1

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

Re: [RFC 1/4] drm/i915: Reduce recursive mutex locking from the shrinker

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-03 12:23:26)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> In two codepaths internal to the shrinker we know we will end up taking
> the resursive mutex path.
> 
> It instead feels more elegant to avoid this altogether and not call
> mutex_trylock_recursive in those cases.
> 
> We achieve this by adding a new I915_SHRINK_LOCKED flag which gets passed
> to i915_gem_shrink by the internal callers.

Since we reach here from many paths with struct_mutex either locked or
unlocked, special casing one such seems silly. Especially when removing
struct_mutex from here entirely is within our grasp.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for Shrinker tweaks/fixes?

[Patchwork]

== Series Details ==

Series: Shrinker tweaks/fixes?
URL   : https://patchwork.freedesktop.org/series/54684/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
bd6ce1fccda6 drm/i915: Reduce recursive mutex locking from the shrinker
-:59: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#59: FILE: drivers/gpu/drm/i915/i915_gem_shrinker.c:355:
+	const unsigned flags =

total: 0 errors, 1 warnings, 0 checks, 69 lines checked
9c36907ea664 drm/i915: Fix timeout handling in i915_gem_shrinker_vmap
581b14dc5394 drm/i915: Return immediately if trylock fails for direct-reclaim
e8fe6cd94f45 drm/i915: Remove trylock-looping from the shrinker

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

✗ Fi.CI.SPARSE: warning for Shrinker tweaks/fixes?

[Patchwork]

== Series Details ==

Series: Shrinker tweaks/fixes?
URL   : https://patchwork.freedesktop.org/series/54684/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Reduce recursive mutex locking from the shrinker
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3545:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3546:16: warning: expression using sizeof(void)

Commit: drm/i915: Fix timeout handling in i915_gem_shrinker_vmap
Okay!

Commit: drm/i915: Return immediately if trylock fails for direct-reclaim
Okay!

Commit: drm/i915: Remove trylock-looping from the shrinker
Okay!

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

Re: [RFC 4/4] drm/i915: Remove trylock-looping from the shrinker

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-03 12:23:29)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> In a patch named "drm/i915: Return immediately if trylock fails for
> direct-reclaim" Chris Wilson implicitly suggests it is safe to use a plain
> mutex_lock instead of trylock on the I915_SHRINK_ACTIVE code paths.
> 
> Explore this to see how the lockdep traces will look.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index 97deec775f40..2d4a54851c0a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -47,15 +47,8 @@ shrinker_lock(struct drm_i915_private *i915, unsigned int flags, bool *unlock)
>         case MUTEX_TRYLOCK_FAILED:
>                 *unlock = false;
>                 if (flags & I915_SHRINK_ACTIVE) {
> -                       preempt_disable();
> -                       do {
> -                               cpu_relax();
> -                               if (mutex_trylock(&i915->drm.struct_mutex)) {
> -                                       *unlock = true;
> -                                       break;
> -                               }
> -                       } while (!need_resched());
> -                       preempt_enable();
> +                       mutex_lock(&i915->drm.struct_mutex);

You seemed to have lost some annotation here.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Shrinker tweaks/fixes?

[Patchwork]

== Series Details ==

Series: Shrinker tweaks/fixes?
URL   : https://patchwork.freedesktop.org/series/54684/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5360 -> Patchwork_11180
====================================================

Summary
-------

  **WARNING**

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

  External URL: https://patchwork.freedesktop.org/api/1.0/series/54684/revisions/1/mbox/

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

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

### IGT changes ###

#### Warnings ####

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
    - fi-kbl-7567u:       SKIP -> PASS +33

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        PASS -> DMESG-FAIL [fdo#108735]
    - fi-skl-iommu:       PASS -> INCOMPLETE [fdo#108602] / [fdo#108744]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          PASS -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#107362]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362] +1

  * {igt@runner@aborted}:
    - fi-skl-iommu:       NOTRUN -> FAIL [fdo#104108] / [fdo#108602]

  
#### Possible fixes ####

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-skl-6700k2:      INCOMPLETE [fdo#104108] / [fdo#105524] / [k.org#199541] -> PASS

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         FAIL [fdo#103182] -> PASS

  
  {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#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105524]: https://bugs.freedesktop.org/show_bug.cgi?id=105524
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [k.org#199541]: https://bugzilla.kernel.org/show_bug.cgi?id=199541


Participating hosts (53 -> 44)
------------------------------

  Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-ctg-p8600 fi-bdw-samus 


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

    * Linux: CI_DRM_5360 -> Patchwork_11180

  CI_DRM_5360: 3dc51908b55a5f2eac07f34fa0a18ddf247154f9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4756: 75081c6bfb9998bd7cbf35a7ac0578c683fe55a8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11180: e8fe6cd94f4585ed0e293db3ae193d43e5ca0ecb @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e8fe6cd94f45 drm/i915: Remove trylock-looping from the shrinker
581b14dc5394 drm/i915: Return immediately if trylock fails for direct-reclaim
9c36907ea664 drm/i915: Fix timeout handling in i915_gem_shrinker_vmap
bd6ce1fccda6 drm/i915: Reduce recursive mutex locking from the shrinker

== Logs ==

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

Re: [RFC 1/4] drm/i915: Reduce recursive mutex locking from the shrinker

[Tvrtko Ursulin]

On 03/01/2019 12:29, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-03 12:23:26)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In two codepaths internal to the shrinker we know we will end up taking
>> the resursive mutex path.
>>
>> It instead feels more elegant to avoid this altogether and not call
>> mutex_trylock_recursive in those cases.
>>
>> We achieve this by adding a new I915_SHRINK_LOCKED flag which gets passed
>> to i915_gem_shrink by the internal callers.
> 
> Since we reach here from many paths with struct_mutex either locked or
> unlocked, special casing one such seems silly. Especially when removing
> struct_mutex from here entirely is within our grasp.

Hm, for me it is silly that we rely on mutex_trylock_recursive to handle 
what the code clearly knows. Would you be happy with the 
__i915_gem_shrink which does no locking, and i915_gem_shrink wraps with 
locking, approach instead?

Regards,

Tvrtko

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

Re: [RFC 4/4] drm/i915: Remove trylock-looping from the shrinker

[Tvrtko Ursulin]

On 03/01/2019 12:56, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-03 12:23:29)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In a patch named "drm/i915: Return immediately if trylock fails for
>> direct-reclaim" Chris Wilson implicitly suggests it is safe to use a plain
>> mutex_lock instead of trylock on the I915_SHRINK_ACTIVE code paths.
>>
>> Explore this to see how the lockdep traces will look.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_shrinker.c | 11 ++---------
>>   1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
>> index 97deec775f40..2d4a54851c0a 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
>> @@ -47,15 +47,8 @@ shrinker_lock(struct drm_i915_private *i915, unsigned int flags, bool *unlock)
>>          case MUTEX_TRYLOCK_FAILED:
>>                  *unlock = false;
>>                  if (flags & I915_SHRINK_ACTIVE) {
>> -                       preempt_disable();
>> -                       do {
>> -                               cpu_relax();
>> -                               if (mutex_trylock(&i915->drm.struct_mutex)) {
>> -                                       *unlock = true;
>> -                                       break;
>> -                               }
>> -                       } while (!need_resched());
>> -                       preempt_enable();
>> +                       mutex_lock(&i915->drm.struct_mutex);
> 
> You seemed to have lost some annotation here.

I deliberately omitted mutex_lock_nested from this series to avoid 
hiding any potential lockdep issues. If there won't be any hit then we 
really need to improve on the test coverage.

Regards,

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

✗ Fi.CI.IGT: failure for Shrinker tweaks/fixes?

[Patchwork]

== Series Details ==

Series: Shrinker tweaks/fixes?
URL   : https://patchwork.freedesktop.org/series/54684/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5360_full -> Patchwork_11180_full
====================================================

Summary
-------

  **FAILURE**

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

  

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@gem_persistent_relocs@forked-faulting-reloc-thrashing:
    - shard-kbl:          PASS -> DMESG-WARN +2

  * igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing:
    - shard-apl:          PASS -> DMESG-WARN +2
    - shard-skl:          PASS -> DMESG-WARN +1

  * igt@gem_persistent_relocs@forked-interruptible-thrashing:
    - shard-glk:          PASS -> DMESG-WARN +2
    - shard-iclb:         PASS -> DMESG-WARN +2

  * igt@gem_persistent_relocs@forked-thrashing:
    - shard-snb:          PASS -> DMESG-WARN +1
    - shard-skl:          NOTRUN -> DMESG-WARN
    - shard-hsw:          PASS -> DMESG-WARN +2

  
#### Suppressed ####

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

  * {igt@runner@aborted}:
    - shard-hsw:          NOTRUN -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]

  * igt@i915_suspend@shrink:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#107886] / [fdo#108784]

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_ccs@pipe-a-crc-primary-basic:
    - shard-iclb:         NOTRUN -> FAIL [fdo#107725]

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
    - shard-glk:          PASS -> FAIL [fdo#108145]

  * igt@kms_color@pipe-b-legacy-gamma:
    - shard-apl:          PASS -> FAIL [fdo#104782]

  * igt@kms_cursor_crc@cursor-128x42-onscreen:
    - shard-glk:          PASS -> FAIL [fdo#103232] +2

  * igt@kms_cursor_crc@cursor-256x85-onscreen:
    - shard-apl:          PASS -> FAIL [fdo#103232] +3

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-glk:          PASS -> FAIL [fdo#103167] +3

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-blt:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@psr-suspend:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#106978] / [fdo#107773]

  * igt@kms_plane@pixel-format-pipe-a-planes-source-clamping:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#106885]

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108]

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] +1

  * igt@kms_plane_alpha_blend@pipe-c-alpha-7efc:
    - shard-skl:          NOTRUN -> FAIL [fdo#107815] / [fdo#108145]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166]
    - shard-apl:          PASS -> FAIL [fdo#103166]

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] +2

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@pm_rpm@legacy-planes:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#108654]

  * igt@pm_rpm@modeset-lpsp-stress-no-wait:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

  * igt@pm_rpm@system-suspend-execbuf:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773] / [fdo#107807]

  
#### Possible fixes ####

  * igt@debugfs_test@read_all_entries_display_off:
    - shard-skl:          INCOMPLETE [fdo#104108] -> PASS

  * igt@gem_userptr_blits@readonly-unsync:
    - shard-iclb:         TIMEOUT [fdo#108887] -> PASS

  * igt@kms_concurrent@pipe-b:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS +16

  * igt@kms_cursor_crc@cursor-128x42-sliding:
    - shard-apl:          FAIL [fdo#103232] -> PASS +2
    - shard-glk:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          FAIL [fdo#105363] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-mmap-cpu:
    - shard-iclb:         DMESG-FAIL [fdo#107720] / [fdo#107724] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
    - shard-apl:          FAIL [fdo#103167] -> PASS +1

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-pwrite:
    - shard-glk:          FAIL [fdo#103167] -> PASS +2

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-blt:
    - shard-iclb:         DMESG-WARN [fdo#107724] / [fdo#108336] -> PASS +3

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-mmap-wc:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +3

  * igt@kms_plane@plane-position-covered-pipe-b-planes:
    - shard-glk:          FAIL [fdo#103166] -> PASS +2

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
    - shard-iclb:         FAIL [fdo#103166] -> PASS

  * igt@kms_rotation_crc@multiplane-rotation:
    - shard-iclb:         DMESG-FAIL [fdo#107724] -> PASS +4

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-glk:          DMESG-FAIL [fdo#105763] / [fdo#106538] -> PASS

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         FAIL [fdo#100047] -> PASS

  * igt@kms_universal_plane@universal-plane-pipe-a-functional:
    - shard-iclb:         DMESG-FAIL [fdo#103166] / [fdo#107724] -> PASS

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
    - shard-iclb:         INCOMPLETE [fdo#107713] -> PASS

  * igt@pm_rpm@modeset-lpsp-stress:
    - shard-iclb:         DMESG-WARN [fdo#108654] -> PASS

  
#### Warnings ####

  * igt@i915_suspend@shrink:
    - shard-glk:          DMESG-WARN [fdo#108784] -> INCOMPLETE [fdo#103359] / [fdo#106886] / [k.org#198133]

  * igt@kms_cursor_crc@cursor-64x64-onscreen:
    - shard-iclb:         DMESG-WARN [fdo#107724] / [fdo#108336] -> FAIL [fdo#103232]

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          DMESG-WARN [fdo#105604] -> DMESG-FAIL [fdo#108950]

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

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105604]: https://bugs.freedesktop.org/show_bug.cgi?id=105604
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
  [fdo#106885]: https://bugs.freedesktop.org/show_bug.cgi?id=106885
  [fdo#106886]: https://bugs.freedesktop.org/show_bug.cgi?id=106886
  [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107720]: https://bugs.freedesktop.org/show_bug.cgi?id=107720
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107886]: https://bugs.freedesktop.org/show_bug.cgi?id=107886
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336
  [fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
  [fdo#108784]: https://bugs.freedesktop.org/show_bug.cgi?id=108784
  [fdo#108887]: https://bugs.freedesktop.org/show_bug.cgi?id=108887
  [fdo#108950]: https://bugs.freedesktop.org/show_bug.cgi?id=108950
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts


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

    * Linux: CI_DRM_5360 -> Patchwork_11180

  CI_DRM_5360: 3dc51908b55a5f2eac07f34fa0a18ddf247154f9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4756: 75081c6bfb9998bd7cbf35a7ac0578c683fe55a8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11180: e8fe6cd94f4585ed0e293db3ae193d43e5ca0ecb @ 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_11180/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx