[igt-dev] [PATCH i-g-t v2 1/1] i915_pm_rpm: gem-execbuf-stress-extra-wait faster

[igt-dev] [PATCH i-g-t v2 1/1] i915_pm_rpm: gem-execbuf-stress-extra-wait faster

[Caz Yokoyama]

With the corresponding patch in drm-tip,
confirm there is no additional work needed when
GPU is in suspended state.

Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com>
---
 lib/igt_debugfs.h        | 7 +++++++
 tests/i915/i915_pm_rpm.c | 7 +++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
index b9cf0f81..9fa38b42 100644
--- a/lib/igt_debugfs.h
+++ b/lib/igt_debugfs.h
@@ -216,6 +216,13 @@ void igt_require_hpd_storm_ctl(int fd);
 		  DROP_FREED | \
 		  DROP_IDLE)
 
+/**
+ * DROP_SUSPEND:
+ *
+ * force to suspended state.
+ */
+#define DROP_SUSPEND 0x200
+
 void igt_reset_fifo_underrun_reporting(int drm_fd);
 
 bool igt_drop_caches_has(int fd, uint64_t val);
diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
index be296f52..79528d18 100644
--- a/tests/i915/i915_pm_rpm.c
+++ b/tests/i915/i915_pm_rpm.c
@@ -1338,12 +1338,15 @@ static void gem_execbuf_stress_subtest(int rounds, int wait_flags)
 	for (i = 0; i < rounds; i++) {
 		gem_execbuf(drm_fd, &execbuf);
 
+		igt_drop_caches_set(drm_fd, DROP_IDLE); /* clean up idle work */
 		if (wait_flags & WAIT_STATUS)
 			igt_assert(wait_for_suspended());
 		if (wait_flags & WAIT_PC8_RES)
 			igt_assert(pc8_plus_residency_changed(30));
-		if (wait_flags & WAIT_EXTRA)
-			sleep(5);
+		if (wait_flags & WAIT_EXTRA) {
+			/* force to suspend mode */
+			igt_drop_caches_set(drm_fd, DROP_SUSPEND);
+		}
 	}
 
 	gem_close(drm_fd, handle);
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v2 2/2] add DROP_SUSPEND.

[Caz Yokoyama]

From: "Yokoyama, Caz" <caz.yokoyama@intel.com>

Confirm there is no additional work needed when
GPU is in suspended state.

Change-Id: I0775d784f1485015304261309652de084280bf82
Signed-off-by: Yokoyama, Caz <caz.yokoyama@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0a6348ad7c98..ca0ba1f0d89e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -28,6 +28,7 @@
 
 #include <linux/sort.h>
 #include <linux/sched/mm.h>
+#include <linux/pm_runtime.h>
 #include <drm/drm_debugfs.h>
 #include <drm/drm_fourcc.h>
 #include "intel_drv.h"
@@ -3873,6 +3874,7 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_wedged_fops,
 #define DROP_IDLE	BIT(6)
 #define DROP_RESET_ACTIVE	BIT(7)
 #define DROP_RESET_SEQNO	BIT(8)
+#define DROP_SUSPEND	BIT(9)
 #define DROP_ALL (DROP_UNBOUND	| \
 		  DROP_BOUND	| \
 		  DROP_RETIRE	| \
@@ -3899,7 +3901,9 @@ i915_drop_caches_set(void *data, u64 val)
 
 	DRM_DEBUG("Dropping caches: 0x%08llx [0x%08llx]\n",
 		  val, val & DROP_ALL);
-	wakeref = intel_runtime_pm_get(i915);
+	if (!(val & DROP_SUSPEND)) {
+		wakeref = intel_runtime_pm_get(i915);
+	}
 
 	if (val & DROP_RESET_ACTIVE &&
 	    wait_for(intel_engines_are_idle(i915), I915_IDLE_ENGINES_TIMEOUT))
@@ -3949,8 +3953,23 @@ i915_drop_caches_set(void *data, u64 val)
 	if (val & DROP_FREED)
 		i915_gem_drain_freed_objects(i915);
 
+	if (val & DROP_SUSPEND) {
+		struct device *dev = i915->drm.dev;
+
+		dev_info(dev, "%s() %d: runtime_status: %d\n", __func__, __LINE__,
+			 dev->power.runtime_status);
+		pm_runtime_disable(dev);
+		if (!pm_runtime_status_suspended(dev)) {
+			dev_warn(dev, "%s() %d: runtime_status: %d\n", __func__, __LINE__,
+				 dev->power.runtime_status);
+		}
+		pm_runtime_enable(dev);
+	}
+
 out:
-	intel_runtime_pm_put(i915, wakeref);
+	if (!(val & DROP_SUSPEND)) {
+		intel_runtime_pm_put(i915, wakeref);
+	}
 
 	return ret;
 }
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2 1/1] i915_pm_rpm: gem-execbuf-stress-extra-wait faster

[Caz Yokoyama]

The corresponding patch is subjected as "[PATCH v2 2/2] add
DROP_SUSPEND".

The patch in i915_drop_caches_set() is the first part
of pm_runtime_force_suspend(). After GPU becomes suspended state,
pm_runtime_status_suspended(dev) is always true and get out. In other
words, the patch and pm_runtime_force_suspend() are equivalent on
suspended state.

As a result, I prefer to remove gem-execbuf-stress-extra-wait subtest
because it does same as gem-execbuf-stress except for meaningless (10 x
5 sec) delay.
Chris, how do you think?
-caz

On Tue, 2019-03-12 at 03:31 +0000, Caz Yokoyama wrote:
> With the corresponding patch in drm-tip,
> confirm there is no additional work needed when
> GPU is in suspended state.
> 
> Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com>
> ---
>  lib/igt_debugfs.h        | 7 +++++++
>  tests/i915/i915_pm_rpm.c | 7 +++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
> index b9cf0f81..9fa38b42 100644
> --- a/lib/igt_debugfs.h
> +++ b/lib/igt_debugfs.h
> @@ -216,6 +216,13 @@ void igt_require_hpd_storm_ctl(int fd);
>  		  DROP_FREED | \
>  		  DROP_IDLE)
>  
> +/**
> + * DROP_SUSPEND:
> + *
> + * force to suspended state.
> + */
> +#define DROP_SUSPEND 0x200
> +
>  void igt_reset_fifo_underrun_reporting(int drm_fd);
>  
>  bool igt_drop_caches_has(int fd, uint64_t val);
> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> index be296f52..79528d18 100644
> --- a/tests/i915/i915_pm_rpm.c
> +++ b/tests/i915/i915_pm_rpm.c
> @@ -1338,12 +1338,15 @@ static void gem_execbuf_stress_subtest(int
> rounds, int wait_flags)
>  	for (i = 0; i < rounds; i++) {
>  		gem_execbuf(drm_fd, &execbuf);
>  
> +		igt_drop_caches_set(drm_fd, DROP_IDLE); /* clean up
> idle work */
>  		if (wait_flags & WAIT_STATUS)
>  			igt_assert(wait_for_suspended());
>  		if (wait_flags & WAIT_PC8_RES)
>  			igt_assert(pc8_plus_residency_changed(30));
> -		if (wait_flags & WAIT_EXTRA)
> -			sleep(5);
> +		if (wait_flags & WAIT_EXTRA) {
> +			/* force to suspend mode */
> +			igt_drop_caches_set(drm_fd, DROP_SUSPEND);
> +		}
>  	}
>  
>  	gem_close(drm_fd, handle);

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2 1/1] i915_pm_rpm: gem-execbuf-stress-extra-wait faster

[Chris Wilson]

Quoting Caz Yokoyama (2019-03-12 03:31:31)
> With the corresponding patch in drm-tip,
> confirm there is no additional work needed when
> GPU is in suspended state.
> 
> Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com>
> ---
>  lib/igt_debugfs.h        | 7 +++++++
>  tests/i915/i915_pm_rpm.c | 7 +++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
> index b9cf0f81..9fa38b42 100644
> --- a/lib/igt_debugfs.h
> +++ b/lib/igt_debugfs.h
> @@ -216,6 +216,13 @@ void igt_require_hpd_storm_ctl(int fd);
>                   DROP_FREED | \
>                   DROP_IDLE)
>  
> +/**
> + * DROP_SUSPEND:
> + *
> + * force to suspended state.
> + */
> +#define DROP_SUSPEND 0x200
> +
>  void igt_reset_fifo_underrun_reporting(int drm_fd);
>  
>  bool igt_drop_caches_has(int fd, uint64_t val);
> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> index be296f52..79528d18 100644
> --- a/tests/i915/i915_pm_rpm.c
> +++ b/tests/i915/i915_pm_rpm.c
> @@ -1338,12 +1338,15 @@ static void gem_execbuf_stress_subtest(int rounds, int wait_flags)
>         for (i = 0; i < rounds; i++) {
>                 gem_execbuf(drm_fd, &execbuf);
>  
> +               igt_drop_caches_set(drm_fd, DROP_IDLE); /* clean up idle work */
>                 if (wait_flags & WAIT_STATUS)
>                         igt_assert(wait_for_suspended());
>                 if (wait_flags & WAIT_PC8_RES)
>                         igt_assert(pc8_plus_residency_changed(30));
> -               if (wait_flags & WAIT_EXTRA)
> -                       sleep(5);
> +               if (wait_flags & WAIT_EXTRA) {
> +                       /* force to suspend mode */
> +                       igt_drop_caches_set(drm_fd, DROP_SUSPEND);
> +               }

Hmm, this would ostensibly be a quicker WAIT_STATUS. So the meaning of
WAIT_EXTRA (whatever that meaning was) is now tautological.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 2/2] add DROP_SUSPEND.

[Chris Wilson]

Quoting Caz Yokoyama (2019-03-12 03:31:32)
> +       if (val & DROP_SUSPEND) {
> +               struct device *dev = i915->drm.dev;
> +
> +               dev_info(dev, "%s() %d: runtime_status: %d\n", __func__, __LINE__,
> +                        dev->power.runtime_status);


> +               pm_runtime_disable(dev);
> +               if (!pm_runtime_status_suspended(dev)) {
> +                       dev_warn(dev, "%s() %d: runtime_status: %d\n", __func__, __LINE__,
> +                                dev->power.runtime_status);
> +               }
> +               pm_runtime_enable(dev);

ret = pm_runtime_force_suspend(i915->drm.dev);
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 2/2] add DROP_SUSPEND.

[Chris Wilson]

Quoting Caz Yokoyama (2019-03-12 15:52:09)
> On Tue, 2019-03-12 at 15:30 +0000, Chris Wilson wrote:
> > Quoting Caz Yokoyama (2019-03-12 03:31:32)
> > > +       if (val & DROP_SUSPEND) {
> > > +               struct device *dev = i915->drm.dev;
> > > +
> > > +               dev_info(dev, "%s() %d: runtime_status: %d\n",
> > > __func__, __LINE__,
> > > +                        dev->power.runtime_status);
> > 
> > 
> > > +               pm_runtime_disable(dev);
> > > +               if (!pm_runtime_status_suspended(dev)) {
> > > +                       dev_warn(dev, "%s() %d: runtime_status:
> > > %d\n", __func__, __LINE__,
> > > +                                dev->power.runtime_status);
> > > +               }
> > > +               pm_runtime_enable(dev);
> > 
> > ret = pm_runtime_force_suspend(i915->drm.dev);
> > 
> Yes, this works. However, pm_runtime_force_suspend() always exits on 
>         if (pm_runtime_status_suspended(dev))
>                 return 0;
> Therefore, this patch is meaningless after suspended state.

Which would be fine for DROP_SUSPEND. Ah, so your intent is to do a
DROP_WAKE as well :)
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 2/2] add DROP_SUSPEND.

[Caz Yokoyama]

On Tue, 2019-03-12 at 15:30 +0000, Chris Wilson wrote:
> Quoting Caz Yokoyama (2019-03-12 03:31:32)
> > +       if (val & DROP_SUSPEND) {
> > +               struct device *dev = i915->drm.dev;
> > +
> > +               dev_info(dev, "%s() %d: runtime_status: %d\n",
> > __func__, __LINE__,
> > +                        dev->power.runtime_status);
> 
> 
> > +               pm_runtime_disable(dev);
> > +               if (!pm_runtime_status_suspended(dev)) {
> > +                       dev_warn(dev, "%s() %d: runtime_status:
> > %d\n", __func__, __LINE__,
> > +                                dev->power.runtime_status);
> > +               }
> > +               pm_runtime_enable(dev);
> 
> ret = pm_runtime_force_suspend(i915->drm.dev);
> 
Yes, this works. However, pm_runtime_force_suspend() always exits on 
	if (pm_runtime_status_suspended(dev))
		return 0;
Therefore, this patch is meaningless after suspended state.
-caz

> -Chris

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 2/2] add DROP_SUSPEND.

[Chris Wilson]

Quoting Caz Yokoyama (2019-03-12 16:11:14)
> On Tue, 2019-03-12 at 15:51 +0000, Chris Wilson wrote:
> > Quoting Caz Yokoyama (2019-03-12 15:52:09)
> > > On Tue, 2019-03-12 at 15:30 +0000, Chris Wilson wrote:
> > > > Quoting Caz Yokoyama (2019-03-12 03:31:32)
> > > > > +       if (val & DROP_SUSPEND) {
> > > > > +               struct device *dev = i915->drm.dev;
> > > > > +
> > > > > +               dev_info(dev, "%s() %d: runtime_status: %d\n",
> > > > > __func__, __LINE__,
> > > > > +                        dev->power.runtime_status);
> > > > 
> > > > 
> > > > > +               pm_runtime_disable(dev);
> > > > > +               if (!pm_runtime_status_suspended(dev)) {
> > > > > +                       dev_warn(dev, "%s() %d: runtime_status:
> > > > > %d\n", __func__, __LINE__,
> > > > > +                                dev->power.runtime_status);
> > > > > +               }
> > > > > +               pm_runtime_enable(dev);
> > > > 
> > > > ret = pm_runtime_force_suspend(i915->drm.dev);
> > > > 
> > > 
> > > Yes, this works. However, pm_runtime_force_suspend() always exits
> > > on 
> > >         if (pm_runtime_status_suspended(dev))
> > >                 return 0;
> > > Therefore, this patch is meaningless after suspended state.
> > 
> > Which would be fine for DROP_SUSPEND. Ah, so your intent is to do a
> > DROP_WAKE as well :)
> 
> My intention is to remove gem-execbuf-stress-extra-wait subtest
> because it does same as gem-execbuf-stress except for meaningless (10 x
> 5 sec) delay.

Then why complain that pm_runtime_force_suspend() is a no-op starting
from a suspended state?

WAIT_EXTRA is after WAIT_STATUS, so it will always be in a suspended
state. If your intent then is to bolster the test by forcing a fresh
suspend cycle, make that explicit and have both DROP_WAKE |
DROP_SUSPEND. As it stands we can use DROP_SUSPEND as a faster
WAIT_STATUS.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 2/2] add DROP_SUSPEND.

[Caz Yokoyama]

On Tue, 2019-03-12 at 15:51 +0000, Chris Wilson wrote:
> Quoting Caz Yokoyama (2019-03-12 15:52:09)
> > On Tue, 2019-03-12 at 15:30 +0000, Chris Wilson wrote:
> > > Quoting Caz Yokoyama (2019-03-12 03:31:32)
> > > > +       if (val & DROP_SUSPEND) {
> > > > +               struct device *dev = i915->drm.dev;
> > > > +
> > > > +               dev_info(dev, "%s() %d: runtime_status: %d\n",
> > > > __func__, __LINE__,
> > > > +                        dev->power.runtime_status);
> > > 
> > > 
> > > > +               pm_runtime_disable(dev);
> > > > +               if (!pm_runtime_status_suspended(dev)) {
> > > > +                       dev_warn(dev, "%s() %d: runtime_status:
> > > > %d\n", __func__, __LINE__,
> > > > +                                dev->power.runtime_status);
> > > > +               }
> > > > +               pm_runtime_enable(dev);
> > > 
> > > ret = pm_runtime_force_suspend(i915->drm.dev);
> > > 
> > 
> > Yes, this works. However, pm_runtime_force_suspend() always exits
> > on 
> >         if (pm_runtime_status_suspended(dev))
> >                 return 0;
> > Therefore, this patch is meaningless after suspended state.
> 
> Which would be fine for DROP_SUSPEND. Ah, so your intent is to do a
> DROP_WAKE as well :)

My intention is to remove gem-execbuf-stress-extra-wait subtest
because it does same as gem-execbuf-stress except for meaningless (10 x
5 sec) delay.

DROP_WAKE is a joke, correct? The runtime of gem-execbuf-stress is 1.5
sec. or less. It is quick enough.
-caz

> -Chris

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 2/2] add DROP_SUSPEND.

[Caz Yokoyama]

On Tue, 2019-03-12 at 16:09 +0000, Chris Wilson wrote:
> Quoting Caz Yokoyama (2019-03-12 16:11:14)
> > On Tue, 2019-03-12 at 15:51 +0000, Chris Wilson wrote:
> > > Quoting Caz Yokoyama (2019-03-12 15:52:09)
> > > > On Tue, 2019-03-12 at 15:30 +0000, Chris Wilson wrote:
> > > > > Quoting Caz Yokoyama (2019-03-12 03:31:32)
> > > > > > +       if (val & DROP_SUSPEND) {
> > > > > > +               struct device *dev = i915->drm.dev;
> > > > > > +
> > > > > > +               dev_info(dev, "%s() %d: runtime_status:
> > > > > > %d\n",
> > > > > > __func__, __LINE__,
> > > > > > +                        dev->power.runtime_status);
> > > > > 
> > > > > 
> > > > > > +               pm_runtime_disable(dev);
> > > > > > +               if (!pm_runtime_status_suspended(dev)) {
> > > > > > +                       dev_warn(dev, "%s() %d:
> > > > > > runtime_status:
> > > > > > %d\n", __func__, __LINE__,
> > > > > > +                                dev-
> > > > > > >power.runtime_status);
> > > > > > +               }
> > > > > > +               pm_runtime_enable(dev);
> > > > > 
> > > > > ret = pm_runtime_force_suspend(i915->drm.dev);
> > > > > 
> > > > 
> > > > Yes, this works. However, pm_runtime_force_suspend() always
> > > > exits
> > > > on 
> > > >         if (pm_runtime_status_suspended(dev))
> > > >                 return 0;
> > > > Therefore, this patch is meaningless after suspended state.
> > > 
> > > Which would be fine for DROP_SUSPEND. Ah, so your intent is to do
> > > a
> > > DROP_WAKE as well :)
> > 
> > My intention is to remove gem-execbuf-stress-extra-wait subtest
> > because it does same as gem-execbuf-stress except for meaningless
> > (10 x
> > 5 sec) delay.
> 
> Then why complain that pm_runtime_force_suspend() is a no-op starting
> from a suspended state?
No, I am not complaining. The patch above is to confirm the patch is
meaningless on suspended state.

> 
> WAIT_EXTRA is after WAIT_STATUS, so it will always be in a suspended
> state. If your intent then is to bolster the test by forcing a fresh
> suspend cycle, make that explicit and have both DROP_WAKE |
> DROP_SUSPEND. As it stands we can use DROP_SUSPEND as a faster
> WAIT_STATUS.
Let me update the patch.
-caz

> -Chris

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 2/2] add DROP_SUSPEND.

[Caz Yokoyama]

On Tue, 2019-03-12 at 08:52 -0700, Caz Yokoyama wrote:
> On Tue, 2019-03-12 at 15:30 +0000, Chris Wilson wrote:
> > Quoting Caz Yokoyama (2019-03-12 03:31:32)
> > > +       if (val & DROP_SUSPEND) {
> > > +               struct device *dev = i915->drm.dev;
> > > +
> > > +               dev_info(dev, "%s() %d: runtime_status: %d\n",
> > > __func__, __LINE__,
> > > +                        dev->power.runtime_status);
> > 
> > 
> > > +               pm_runtime_disable(dev);
> > > +               if (!pm_runtime_status_suspended(dev)) {
> > > +                       dev_warn(dev, "%s() %d: runtime_status:
> > > %d\n", __func__, __LINE__,
> > > +                                dev->power.runtime_status);
> > > +               }
> > > +               pm_runtime_enable(dev);
> > 
> > ret = pm_runtime_force_suspend(i915->drm.dev);
> > 
> 
> Yes, this works. However, pm_runtime_force_suspend() always exits on 
> 	if (pm_runtime_status_suspended(dev))
> 		return 0;
Correction. This does not work because of kernel crash. When exiting
from pm_runtime_force_suspend() by pm_runtime_status_suspended(dev) ==
true, pm_runtime_enable(dev) is not called. It looks a bug for me.
-caz

> Therefore, this patch is meaningless after suspended state.
> -caz
> 
> > -Chris
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 2/2] add DROP_SUSPEND.

[Chris Wilson]

Quoting Caz Yokoyama (2019-03-12 22:36:49)
> On Tue, 2019-03-12 at 08:52 -0700, Caz Yokoyama wrote:
> > On Tue, 2019-03-12 at 15:30 +0000, Chris Wilson wrote:
> > > Quoting Caz Yokoyama (2019-03-12 03:31:32)
> > > > +       if (val & DROP_SUSPEND) {
> > > > +               struct device *dev = i915->drm.dev;
> > > > +
> > > > +               dev_info(dev, "%s() %d: runtime_status: %d\n",
> > > > __func__, __LINE__,
> > > > +                        dev->power.runtime_status);
> > > 
> > > 
> > > > +               pm_runtime_disable(dev);
> > > > +               if (!pm_runtime_status_suspended(dev)) {
> > > > +                       dev_warn(dev, "%s() %d: runtime_status:
> > > > %d\n", __func__, __LINE__,
> > > > +                                dev->power.runtime_status);
> > > > +               }
> > > > +               pm_runtime_enable(dev);
> > > 
> > > ret = pm_runtime_force_suspend(i915->drm.dev);
> > > 
> > 
> > Yes, this works. However, pm_runtime_force_suspend() always exits on 
> >       if (pm_runtime_status_suspended(dev))
> >               return 0;
> Correction. This does not work because of kernel crash. When exiting
> from pm_runtime_force_suspend() by pm_runtime_status_suspended(dev) ==
> true, pm_runtime_enable(dev) is not called. It looks a bug for me.

Bah, just that I'm suggesting to use them outside of their expected
context. They should only be used during system-wide suspend/resume
apparently, which is annoying.

However, without such a call, we do not guarantee we suspend (skipping a
call to intel_runtime_suspend).

Maybe...
	pm_runtime_disable();
	pm_runtime_suspend();
	pm_runtime_enable();

If a plain pm_runtime_suspend() isn't enough by itself.

Fun experiments,
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev