[PATCH 1/3] PM / Runtime: Introduce pm_runtime_get_noidle

[PATCH 1/3] PM / Runtime: Introduce pm_runtime_get_noidle

[Joonas Lahtinen]

Introduce pm_runtime_get_noidle to for situations where it is not
desireable to touch an idling device. One use scenario is periodic
hangchecks performed by the drm/i915 driver which can be omitted
on a device in a runtime idle state.

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: linux-pm@vger.kernel.org
---
 Documentation/power/runtime_pm.txt | 3 +++
 include/linux/pm_runtime.h         | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
index 0784bc3..4545399 100644
--- a/Documentation/power/runtime_pm.txt
+++ b/Documentation/power/runtime_pm.txt
@@ -363,6 +363,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
   void pm_runtime_get_noresume(struct device *dev);
     - increment the device's usage counter
 
+  int pm_runtime_get_noidle(struct device *dev);
+    - increment the device's usage counter if it was not 0 and return it
+
   int pm_runtime_get(struct device *dev);
     - increment the device's usage counter, run pm_request_resume(dev) and
       return its result
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 3bdbb41..ae34f5e 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -66,6 +66,11 @@ static inline void pm_runtime_get_noresume(struct device *dev)
 	atomic_inc(&dev->power.usage_count);
 }
 
+static inline int pm_runtime_get_noidle(struct device *dev)
+{
+	return atomic_add_unless(&dev->power.usage_count, 1, 0);
+}
+
 static inline void pm_runtime_put_noidle(struct device *dev)
 {
 	atomic_add_unless(&dev->power.usage_count, -1, 0);
@@ -153,6 +158,7 @@ static inline void pm_runtime_forbid(struct device *dev) {}
 
 static inline bool pm_children_suspended(struct device *dev) { return false; }
 static inline void pm_runtime_get_noresume(struct device *dev) {}
+static inline bool pm_runtime_get_noidle(struct device *dev) { return true; }
 static inline void pm_runtime_put_noidle(struct device *dev) {}
 static inline bool device_run_wake(struct device *dev) { return false; }
 static inline void device_set_run_wake(struct device *dev, bool enable) {}
-- 
2.4.3

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

[PATCH 2/3] drm/i915: Take runtime pm wakelock during hangcheck

[Joonas Lahtinen]

From: Chris Wilson <chris@chris-wilson.co.uk>

During hangcheck we access the hardware registers, for which we must
hold a runtime pm reference. Hangcheck also should only be running
whilst the GPU is active, and we hold a runtime pm whilst the GPU is
busy. Therefore, if the runtime pm is suspended (no wakelocks held
anywhere) we know the GPU is already idle and we can skip the hangcheck
(and all further hangchecks until the next request is submitted to the
GPU, waking it up).

Currently, hangcheck relies upon being flushed during
intel_runtime_suspend() but is being done so too late causing invalid
hardware access whilst the device is being suspend. By taking an
explicit wakelock (albeit only if already awake) inside hangcheck we can
remove the synchronous cancellation from the suspend function.

v2:
- Actually make the code work (Joonas)
- Use previously introduced pm_runtime_get_noidle instead of directly
  touching PM object internals (Joonas)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93121
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c         |  9 +++++++++
 drivers/gpu/drm/i915/intel_drv.h        |  1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e88d692..6c20d5b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2989,6 +2989,13 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 	if (!i915.enable_hangcheck)
 		return;
 
+	/* If the runtime pm is off, then the GPU is asleep and we are
+	 * completely idle, so we can belatedly cancel hangcheck. Hangcheck
+	 * will be restarted on the next request.
+	 */
+	if (!intel_runtime_get_noidle(dev_priv))
+		return;
+
 	for_each_ring(ring, dev_priv, i) {
 		u64 acthd;
 		u32 seqno;
@@ -3080,6 +3087,8 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 		}
 	}
 
+	intel_runtime_pm_put(dev_priv);
+
 	if (rings_hung)
 		return i915_handle_error(dev, true, "Ring hung");
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8963a8a..022e612 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1429,6 +1429,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
+bool intel_runtime_pm_get_noidle(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 
 void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 2c2151f..950e960 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2263,6 +2263,29 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
 }
 
 /**
+ * intel_runtime_pm_get_noidle - grab a runtime pm reference if not idle
+ * @dev_priv: i915 device instance
+ *
+ * This function grabs a device-level runtime pm reference if it the device
+ * is not idle.
+ *
+ * Any successful call must have a symmetric call to intel_runtime_pm_put()
+ * to release the reference.
+ *
+ * See intel_runtime_pm_get() for more.
+ */
+bool intel_runtime_pm_get_noidle(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct device *device = &dev->pdev->dev;
+
+	if (!HAS_RUNTIME_PM(dev))
+		return true;
+
+	return !!pm_runtime_get_noidle(device);
+}
+
+/**
  * intel_runtime_pm_put - release a runtime pm reference
  * @dev_priv: i915 device instance
  *
-- 
2.4.3

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

[PATCH 3/3] drm/i915: Do not cancel hangcheck prior to runtime suspend

[Joonas Lahtinen]

Hangcheck will not touch an idle device anymore, so it does not need
to be cancelled.

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 81862d5..ec5ccaa 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1462,8 +1462,6 @@ static int intel_runtime_suspend(struct device *device)
 	i915_gem_release_all_mmaps(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 
-	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
-
 	intel_guc_suspend(dev);
 
 	intel_suspend_gt_powersave(dev);
-- 
2.4.3

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

[PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Joonas Lahtinen]

Introduce pm_runtime_get_noidle to for situations where it is not
desireable to touch an idling device. One use scenario is periodic
hangchecks performed by the drm/i915 driver which can be omitted
on a device in a runtime idle state.

v2:
- Fix inconsistent return value when !CONFIG_PM.
- Update documentation for bool return value

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: linux-pm@vger.kernel.org
---
 Documentation/power/runtime_pm.txt | 4 ++++
 include/linux/pm_runtime.h         | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
index 0784bc3..a0cbfe1 100644
--- a/Documentation/power/runtime_pm.txt
+++ b/Documentation/power/runtime_pm.txt
@@ -363,6 +363,10 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
   void pm_runtime_get_noresume(struct device *dev);
     - increment the device's usage counter
 
+  bool pm_runtime_get_noidle(struct device *dev);
+    - increment the device's usage counter if it is not 0 and return
+      whether usage counter was incremented
+
   int pm_runtime_get(struct device *dev);
     - increment the device's usage counter, run pm_request_resume(dev) and
       return its result
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 3bdbb41..8f429bc 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -66,6 +66,11 @@ static inline void pm_runtime_get_noresume(struct device *dev)
 	atomic_inc(&dev->power.usage_count);
 }
 
+static inline bool pm_runtime_get_noidle(struct device *dev)
+{
+	return atomic_add_unless(&dev->power.usage_count, 1, 0) > 0;
+}
+
 static inline void pm_runtime_put_noidle(struct device *dev)
 {
 	atomic_add_unless(&dev->power.usage_count, -1, 0);
@@ -153,6 +158,7 @@ static inline void pm_runtime_forbid(struct device *dev) {}
 
 static inline bool pm_children_suspended(struct device *dev) { return false; }
 static inline void pm_runtime_get_noresume(struct device *dev) {}
+static inline bool pm_runtime_get_noidle(struct device *dev) { return true; }
 static inline void pm_runtime_put_noidle(struct device *dev) {}
 static inline bool device_run_wake(struct device *dev) { return false; }
 static inline void device_set_run_wake(struct device *dev, bool enable) {}
-- 
2.4.3

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

Re: [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen wrote:
> Introduce pm_runtime_get_noidle to for situations where it is not
> desireable to touch an idling device. One use scenario is periodic
> hangchecks performed by the drm/i915 driver which can be omitted
> on a device in a runtime idle state.
> 
> v2:
> - Fix inconsistent return value when !CONFIG_PM.
> - Update documentation for bool return value
> 
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: linux-pm@vger.kernel.org

Well, I don't quite see how this can be used in a non-racy way
without doing an additional pm_runtime_resume() or something like
that in the same code path.

Thanks,
Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Imre Deak]

On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki wrote:
> On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen wrote:
> > Introduce pm_runtime_get_noidle to for situations where it is not
> > desireable to touch an idling device. One use scenario is periodic
> > hangchecks performed by the drm/i915 driver which can be omitted
> > on a device in a runtime idle state.
> > 
> > v2:
> > - Fix inconsistent return value when !CONFIG_PM.
> > - Update documentation for bool return value
> > 
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: linux-pm@vger.kernel.org
> 
> Well, I don't quite see how this can be used in a non-racy way
> without doing an additional pm_runtime_resume() or something like
> that in the same code path.

We don't want to resume, that would be the whole point. We'd like to
ensure that we hold a reference _and_ the device is already active. So
AFAICS we'd need to check runtime_status == RPM_ACTIVE in addition
after taking the reference.

--Imre

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Imre Deak]

On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki wrote:
> > On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
> > > On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki wrote:
> > > > On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen
> > > > wrote:
> > > > > Introduce pm_runtime_get_noidle to for situations where it is
> > > > > not
> > > > > desireable to touch an idling device. One use scenario is
> > > > > periodic
> > > > > hangchecks performed by the drm/i915 driver which can be
> > > > > omitted
> > > > > on a device in a runtime idle state.
> > > > > 
> > > > > v2:
> > > > > - Fix inconsistent return value when !CONFIG_PM.
> > > > > - Update documentation for bool return value
> > > > > 
> > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.c
> > > > > om>
> > > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > > > Cc: linux-pm@vger.kernel.org
> > > > 
> > > > Well, I don't quite see how this can be used in a non-racy way
> > > > without doing an additional pm_runtime_resume() or something
> > > > like
> > > > that in the same code path.
> > > 
> > > We don't want to resume, that would be the whole point. We'd like
> > > to
> > > ensure that we hold a reference _and_ the device is already
> > > active. So
> > > AFAICS we'd need to check runtime_status == RPM_ACTIVE in
> > > addition
> > > after taking the reference.
> > 
> > Right, and that under the lock.
> 
> Which basically means you can call pm_runtime_resume() just fine,
> because it will do nothing if the status is RPM_ACTIVE already.
> 
> So really, why don't you use pm_runtime_get_sync()?

The difference would be that if the status is not RPM_ACTIVE already we
would drop the reference and report error. The caller would in this
case forego of doing something, since we the device is suspended or on
the way to being suspended. One example of such a scenario is a
watchdog like functionality: the watchdog work would
call pm_runtime_get_noidle() and check if the device is ok by doing
some HW access, but only if the device is powered. Otherwise the work
item would do nothing (meaning it also won't reschedule itself). The
watchdog work would get rescheduled next time the device is woken up
and some work is submitted to the device.

--Imre

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
> On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen wrote:
> > > Introduce pm_runtime_get_noidle to for situations where it is not
> > > desireable to touch an idling device. One use scenario is periodic
> > > hangchecks performed by the drm/i915 driver which can be omitted
> > > on a device in a runtime idle state.
> > > 
> > > v2:
> > > - Fix inconsistent return value when !CONFIG_PM.
> > > - Update documentation for bool return value
> > > 
> > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > Cc: linux-pm@vger.kernel.org
> > 
> > Well, I don't quite see how this can be used in a non-racy way
> > without doing an additional pm_runtime_resume() or something like
> > that in the same code path.
> 
> We don't want to resume, that would be the whole point. We'd like to
> ensure that we hold a reference _and_ the device is already active. So
> AFAICS we'd need to check runtime_status == RPM_ACTIVE in addition
> after taking the reference.

Right, and that under the lock.

Thanks,
Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki wrote:
> On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
> > On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen wrote:
> > > > Introduce pm_runtime_get_noidle to for situations where it is not
> > > > desireable to touch an idling device. One use scenario is periodic
> > > > hangchecks performed by the drm/i915 driver which can be omitted
> > > > on a device in a runtime idle state.
> > > > 
> > > > v2:
> > > > - Fix inconsistent return value when !CONFIG_PM.
> > > > - Update documentation for bool return value
> > > > 
> > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > > Cc: linux-pm@vger.kernel.org
> > > 
> > > Well, I don't quite see how this can be used in a non-racy way
> > > without doing an additional pm_runtime_resume() or something like
> > > that in the same code path.
> > 
> > We don't want to resume, that would be the whole point. We'd like to
> > ensure that we hold a reference _and_ the device is already active. So
> > AFAICS we'd need to check runtime_status == RPM_ACTIVE in addition
> > after taking the reference.
> 
> Right, and that under the lock.

Which basically means you can call pm_runtime_resume() just fine,
because it will do nothing if the status is RPM_ACTIVE already.

So really, why don't you use pm_runtime_get_sync()?

Thanks,
Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
> On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> > On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki wrote:
> > > On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
> > > > On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki wrote:
> > > > > On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen
> > > > > wrote:
> > > > > > Introduce pm_runtime_get_noidle to for situations where it is
> > > > > > not
> > > > > > desireable to touch an idling device. One use scenario is
> > > > > > periodic
> > > > > > hangchecks performed by the drm/i915 driver which can be
> > > > > > omitted
> > > > > > on a device in a runtime idle state.
> > > > > > 
> > > > > > v2:
> > > > > > - Fix inconsistent return value when !CONFIG_PM.
> > > > > > - Update documentation for bool return value
> > > > > > 
> > > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.c
> > > > > > om>
> > > > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > > > > Cc: linux-pm@vger.kernel.org
> > > > > 
> > > > > Well, I don't quite see how this can be used in a non-racy way
> > > > > without doing an additional pm_runtime_resume() or something
> > > > > like
> > > > > that in the same code path.
> > > > 
> > > > We don't want to resume, that would be the whole point. We'd like
> > > > to
> > > > ensure that we hold a reference _and_ the device is already
> > > > active. So
> > > > AFAICS we'd need to check runtime_status == RPM_ACTIVE in
> > > > addition
> > > > after taking the reference.
> > > 
> > > Right, and that under the lock.
> > 
> > Which basically means you can call pm_runtime_resume() just fine,
> > because it will do nothing if the status is RPM_ACTIVE already.
> > 
> > So really, why don't you use pm_runtime_get_sync()?
> 
> The difference would be that if the status is not RPM_ACTIVE already we
> would drop the reference and report error. The caller would in this
> case forego of doing something, since we the device is suspended or on
> the way to being suspended. One example of such a scenario is a
> watchdog like functionality: the watchdog work would
> call pm_runtime_get_noidle() and check if the device is ok by doing
> some HW access, but only if the device is powered. Otherwise the work
> item would do nothing (meaning it also won't reschedule itself). The
> watchdog work would get rescheduled next time the device is woken up
> and some work is submitted to the device.

So first of all the name "pm_runtime_get_noidle" doesn't make sense.

I guess what you need is something like

bool pm_runtime_get_if_active(struct device *dev)
{
	unsigned log flags;
	bool ret;

	spin_lock_irqsave(&dev->power.lock, flags);

	if (dev->power.runtime_status == RPM_ACTIVE) {
		atomic_inc(&dev->power.usage_count);
		ret = true;
	} else {
		ret = false;
	}

	spin_unlock_irqrestore(&dev->power.lock, flags);
}

and the caller will simply bail out if "false" is returned, but if "true"
is returned, it will have to drop the usage count, right?

Thanks,
Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Dave Gordon]

On 10/12/15 22:14, Rafael J. Wysocki wrote:
> On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
>> On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
>>> On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki wrote:
>>>> On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
>>>>> On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki wrote:
>>>>>> On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen
>>>>>> wrote:
>>>>>>> Introduce pm_runtime_get_noidle to for situations where it is
>>>>>>> not
>>>>>>> desireable to touch an idling device. One use scenario is
>>>>>>> periodic
>>>>>>> hangchecks performed by the drm/i915 driver which can be
>>>>>>> omitted
>>>>>>> on a device in a runtime idle state.
>>>>>>>
>>>>>>> v2:
>>>>>>> - Fix inconsistent return value when !CONFIG_PM.
>>>>>>> - Update documentation for bool return value
>>>>>>>
>>>>>>> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.c
>>>>>>> om>
>>>>>>> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>>>>> Cc: linux-pm@vger.kernel.org
>>>>>>
>>>>>> Well, I don't quite see how this can be used in a non-racy way
>>>>>> without doing an additional pm_runtime_resume() or something
>>>>>> like
>>>>>> that in the same code path.
>>>>>
>>>>> We don't want to resume, that would be the whole point. We'd like
>>>>> to
>>>>> ensure that we hold a reference _and_ the device is already
>>>>> active. So
>>>>> AFAICS we'd need to check runtime_status == RPM_ACTIVE in
>>>>> addition
>>>>> after taking the reference.
>>>>
>>>> Right, and that under the lock.
>>>
>>> Which basically means you can call pm_runtime_resume() just fine,
>>> because it will do nothing if the status is RPM_ACTIVE already.
>>>
>>> So really, why don't you use pm_runtime_get_sync()?
>>
>> The difference would be that if the status is not RPM_ACTIVE already we
>> would drop the reference and report error. The caller would in this
>> case forego of doing something, since we the device is suspended or on
>> the way to being suspended. One example of such a scenario is a
>> watchdog like functionality: the watchdog work would
>> call pm_runtime_get_noidle() and check if the device is ok by doing
>> some HW access, but only if the device is powered. Otherwise the work
>> item would do nothing (meaning it also won't reschedule itself). The
>> watchdog work would get rescheduled next time the device is woken up
>> and some work is submitted to the device.
>
> So first of all the name "pm_runtime_get_noidle" doesn't make sense.

How about pm_runtime_get_unless_idle(), which would be analogous to 
kref_get_unless_zero() ?

.Dave.

> I guess what you need is something like
>
> bool pm_runtime_get_if_active(struct device *dev)
> {
> 	unsigned log flags;
> 	bool ret;
>
> 	spin_lock_irqsave(&dev->power.lock, flags);
>
> 	if (dev->power.runtime_status == RPM_ACTIVE) {
> 		atomic_inc(&dev->power.usage_count);
> 		ret = true;
> 	} else {
> 		ret = false;
> 	}
>
> 	spin_unlock_irqrestore(&dev->power.lock, flags);
> }
>
> and the caller will simply bail out if "false" is returned, but if "true"
> is returned, it will have to drop the usage count, right?
>
> Thanks,
> Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Ulf Hansson]

[...]

>> >
>> > Which basically means you can call pm_runtime_resume() just fine,
>> > because it will do nothing if the status is RPM_ACTIVE already.
>> >
>> > So really, why don't you use pm_runtime_get_sync()?
>>
>> The difference would be that if the status is not RPM_ACTIVE already we
>> would drop the reference and report error. The caller would in this
>> case forego of doing something, since we the device is suspended or on
>> the way to being suspended. One example of such a scenario is a
>> watchdog like functionality: the watchdog work would
>> call pm_runtime_get_noidle() and check if the device is ok by doing
>> some HW access, but only if the device is powered. Otherwise the work
>> item would do nothing (meaning it also won't reschedule itself). The
>> watchdog work would get rescheduled next time the device is woken up
>> and some work is submitted to the device.
>
> So first of all the name "pm_runtime_get_noidle" doesn't make sense.
>
> I guess what you need is something like
>
> bool pm_runtime_get_if_active(struct device *dev)
> {
>         unsigned log flags;
>         bool ret;
>
>         spin_lock_irqsave(&dev->power.lock, flags);
>
>         if (dev->power.runtime_status == RPM_ACTIVE) {
>                 atomic_inc(&dev->power.usage_count);
>                 ret = true;
>         } else {
>                 ret = false;
>         }
>
>         spin_unlock_irqrestore(&dev->power.lock, flags);
> }
>
> and the caller will simply bail out if "false" is returned, but if "true"
> is returned, it will have to drop the usage count, right?
>
> Thanks,
> Rafael
>

Why not just:

pm_runtime_get_noresume():
if (RPM_ACTIVE)
  "do some actions"
pm_runtime_put();

Kind regards
Uffe

Re: [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Imre Deak]

On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
> > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> > > On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki
> > > wrote:
> > > > On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
> > > > > On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen
> > > > > > wrote:
> > > > > > > Introduce pm_runtime_get_noidle to for situations where
> > > > > > > it is
> > > > > > > not
> > > > > > > desireable to touch an idling device. One use scenario is
> > > > > > > periodic
> > > > > > > hangchecks performed by the drm/i915 driver which can be
> > > > > > > omitted
> > > > > > > on a device in a runtime idle state.
> > > > > > > 
> > > > > > > v2:
> > > > > > > - Fix inconsistent return value when !CONFIG_PM.
> > > > > > > - Update documentation for bool return value
> > > > > > > 
> > > > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.int
> > > > > > > el.c
> > > > > > > om>
> > > > > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > > > > > Cc: linux-pm@vger.kernel.org
> > > > > > 
> > > > > > Well, I don't quite see how this can be used in a non-racy
> > > > > > way
> > > > > > without doing an additional pm_runtime_resume() or
> > > > > > something
> > > > > > like
> > > > > > that in the same code path.
> > > > > 
> > > > > We don't want to resume, that would be the whole point. We'd
> > > > > like
> > > > > to
> > > > > ensure that we hold a reference _and_ the device is already
> > > > > active. So
> > > > > AFAICS we'd need to check runtime_status == RPM_ACTIVE in
> > > > > addition
> > > > > after taking the reference.
> > > > 
> > > > Right, and that under the lock.
> > > 
> > > Which basically means you can call pm_runtime_resume() just fine,
> > > because it will do nothing if the status is RPM_ACTIVE already.
> > > 
> > > So really, why don't you use pm_runtime_get_sync()?
> > 
> > The difference would be that if the status is not RPM_ACTIVE
> > already we
> > would drop the reference and report error. The caller would in this
> > case forego of doing something, since we the device is suspended or
> > on
> > the way to being suspended. One example of such a scenario is a
> > watchdog like functionality: the watchdog work would
> > call pm_runtime_get_noidle() and check if the device is ok by doing
> > some HW access, but only if the device is powered. Otherwise the
> > work
> > item would do nothing (meaning it also won't reschedule itself).
> > The
> > watchdog work would get rescheduled next time the device is woken
> > up
> > and some work is submitted to the device.
> 
> So first of all the name "pm_runtime_get_noidle" doesn't make sense.
> 
> I guess what you need is something like
> 
> bool pm_runtime_get_if_active(struct device *dev)
> {
> 	unsigned log flags;
> 	bool ret;
> 
> 	spin_lock_irqsave(&dev->power.lock, flags);
> 
> 	if (dev->power.runtime_status == RPM_ACTIVE) {

But here usage_count could be zero, meaning that the device is already
on the way to be suspended (autosuspend or ASYNC suspend), no? In that
case we don't want to return success. That would unnecessarily prolong
the time the device is kept active.

> 		atomic_inc(&dev->power.usage_count);
> 		ret = true;
> 	} else {
> 		ret = false;
> 	}
> 
> 	spin_unlock_irqrestore(&dev->power.lock, flags);
> }
> 
> and the caller will simply bail out if "false" is returned, but if
> "true"
> is returned, it will have to drop the usage count, right?

Yes.

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

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Friday, December 11, 2015 01:03:50 PM Ulf Hansson wrote:
> [...]
> 
> >> >
> >> > Which basically means you can call pm_runtime_resume() just fine,
> >> > because it will do nothing if the status is RPM_ACTIVE already.
> >> >
> >> > So really, why don't you use pm_runtime_get_sync()?
> >>
> >> The difference would be that if the status is not RPM_ACTIVE already we
> >> would drop the reference and report error. The caller would in this
> >> case forego of doing something, since we the device is suspended or on
> >> the way to being suspended. One example of such a scenario is a
> >> watchdog like functionality: the watchdog work would
> >> call pm_runtime_get_noidle() and check if the device is ok by doing
> >> some HW access, but only if the device is powered. Otherwise the work
> >> item would do nothing (meaning it also won't reschedule itself). The
> >> watchdog work would get rescheduled next time the device is woken up
> >> and some work is submitted to the device.
> >
> > So first of all the name "pm_runtime_get_noidle" doesn't make sense.
> >
> > I guess what you need is something like
> >
> > bool pm_runtime_get_if_active(struct device *dev)
> > {
> >         unsigned log flags;
> >         bool ret;
> >
> >         spin_lock_irqsave(&dev->power.lock, flags);
> >
> >         if (dev->power.runtime_status == RPM_ACTIVE) {
> >                 atomic_inc(&dev->power.usage_count);
> >                 ret = true;
> >         } else {
> >                 ret = false;
> >         }
> >
> >         spin_unlock_irqrestore(&dev->power.lock, flags);
> > }
> >
> > and the caller will simply bail out if "false" is returned, but if "true"
> > is returned, it will have to drop the usage count, right?
> >
> > Thanks,
> > Rafael
> >
> 
> Why not just:
> 
> pm_runtime_get_noresume():
> if (RPM_ACTIVE)
>   "do some actions"
> pm_runtime_put();

Because that's racy?

What if the rpm_suspend() is running for the device, but it hasn't changed
the status yet?

Thanks,
Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Friday, December 11, 2015 02:54:45 PM Imre Deak wrote:
> On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> > On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
> > > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> > > > On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki
> > > > wrote:
> > > > > On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
> > > > > > On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki wrote:
> > > > > > > On Wednesday, December 09, 2015 06:22:19 PM Joonas Lahtinen
> > > > > > > wrote:
> > > > > > > > Introduce pm_runtime_get_noidle to for situations where
> > > > > > > > it is
> > > > > > > > not
> > > > > > > > desireable to touch an idling device. One use scenario is
> > > > > > > > periodic
> > > > > > > > hangchecks performed by the drm/i915 driver which can be
> > > > > > > > omitted
> > > > > > > > on a device in a runtime idle state.
> > > > > > > > 
> > > > > > > > v2:
> > > > > > > > - Fix inconsistent return value when !CONFIG_PM.
> > > > > > > > - Update documentation for bool return value
> > > > > > > > 
> > > > > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.int
> > > > > > > > el.c
> > > > > > > > om>
> > > > > > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > > > > > > Cc: linux-pm@vger.kernel.org
> > > > > > > 
> > > > > > > Well, I don't quite see how this can be used in a non-racy
> > > > > > > way
> > > > > > > without doing an additional pm_runtime_resume() or
> > > > > > > something
> > > > > > > like
> > > > > > > that in the same code path.
> > > > > > 
> > > > > > We don't want to resume, that would be the whole point. We'd
> > > > > > like
> > > > > > to
> > > > > > ensure that we hold a reference _and_ the device is already
> > > > > > active. So
> > > > > > AFAICS we'd need to check runtime_status == RPM_ACTIVE in
> > > > > > addition
> > > > > > after taking the reference.
> > > > > 
> > > > > Right, and that under the lock.
> > > > 
> > > > Which basically means you can call pm_runtime_resume() just fine,
> > > > because it will do nothing if the status is RPM_ACTIVE already.
> > > > 
> > > > So really, why don't you use pm_runtime_get_sync()?
> > > 
> > > The difference would be that if the status is not RPM_ACTIVE
> > > already we
> > > would drop the reference and report error. The caller would in this
> > > case forego of doing something, since we the device is suspended or
> > > on
> > > the way to being suspended. One example of such a scenario is a
> > > watchdog like functionality: the watchdog work would
> > > call pm_runtime_get_noidle() and check if the device is ok by doing
> > > some HW access, but only if the device is powered. Otherwise the
> > > work
> > > item would do nothing (meaning it also won't reschedule itself).
> > > The
> > > watchdog work would get rescheduled next time the device is woken
> > > up
> > > and some work is submitted to the device.
> > 
> > So first of all the name "pm_runtime_get_noidle" doesn't make sense.
> > 
> > I guess what you need is something like
> > 
> > bool pm_runtime_get_if_active(struct device *dev)
> > {
> > 	unsigned log flags;
> > 	bool ret;
> > 
> > 	spin_lock_irqsave(&dev->power.lock, flags);
> > 
> > 	if (dev->power.runtime_status == RPM_ACTIVE) {
> 
> But here usage_count could be zero, meaning that the device is already
> on the way to be suspended (autosuspend or ASYNC suspend), no?

The usage counter equal to 0 need not mean that the device is being suspended
right now.

Also even if that's the case, the usage counter may be incremented at this very
moment by a concurrent thread and you'll lose the opportunity to do what you
want.

> In that case we don't want to return success. That would unnecessarily prolong
> the time the device is kept active.

Why?

If you have something to do if the device is active, then do it is the status
is "active".  It really shouldn't matter when the device is going to be suspended
going forward.

> > 		atomic_inc(&dev->power.usage_count);
> > 		ret = true;
> > 	} else {
> > 		ret = false;
> > 	}
> > 
> > 	spin_unlock_irqrestore(&dev->power.lock, flags);
> > }
> > 
> > and the caller will simply bail out if "false" is returned, but if
> > "true"
> > is returned, it will have to drop the usage count, right?
> 
> Yes.

OK, I'll send a patch adding this function then.

Thanks,
Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Imre Deak]

On pe, 2015-12-11 at 16:40 +0100, Rafael J. Wysocki wrote:
> On Friday, December 11, 2015 02:54:45 PM Imre Deak wrote:
> > On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> > > On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
> > > > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> > > > > On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki
> > > > > wrote:
> > > > > > On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
> > > > > > > On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki
> > > > > > > wrote:
> > > > > > > > On Wednesday, December 09, 2015 06:22:19 PM Joonas
> > > > > > > > Lahtinen
> > > > > > > > wrote:
> > > > > > > > > Introduce pm_runtime_get_noidle to for situations
> > > > > > > > > where
> > > > > > > > > it is
> > > > > > > > > not
> > > > > > > > > desireable to touch an idling device. One use
> > > > > > > > > scenario is
> > > > > > > > > periodic
> > > > > > > > > hangchecks performed by the drm/i915 driver which can
> > > > > > > > > be
> > > > > > > > > omitted
> > > > > > > > > on a device in a runtime idle state.
> > > > > > > > > 
> > > > > > > > > v2:
> > > > > > > > > - Fix inconsistent return value when !CONFIG_PM.
> > > > > > > > > - Update documentation for bool return value
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux
> > > > > > > > > .int
> > > > > > > > > el.c
> > > > > > > > > om>
> > > > > > > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > > > > > > > Cc: linux-pm@vger.kernel.org
> > > > > > > > 
> > > > > > > > Well, I don't quite see how this can be used in a non-
> > > > > > > > racy
> > > > > > > > way
> > > > > > > > without doing an additional pm_runtime_resume() or
> > > > > > > > something
> > > > > > > > like
> > > > > > > > that in the same code path.
> > > > > > > 
> > > > > > > We don't want to resume, that would be the whole point.
> > > > > > > We'd
> > > > > > > like
> > > > > > > to
> > > > > > > ensure that we hold a reference _and_ the device is
> > > > > > > already
> > > > > > > active. So
> > > > > > > AFAICS we'd need to check runtime_status == RPM_ACTIVE in
> > > > > > > addition
> > > > > > > after taking the reference.
> > > > > > 
> > > > > > Right, and that under the lock.
> > > > > 
> > > > > Which basically means you can call pm_runtime_resume() just
> > > > > fine,
> > > > > because it will do nothing if the status is RPM_ACTIVE
> > > > > already.
> > > > > 
> > > > > So really, why don't you use pm_runtime_get_sync()?
> > > > 
> > > > The difference would be that if the status is not RPM_ACTIVE
> > > > already we
> > > > would drop the reference and report error. The caller would in
> > > > this
> > > > case forego of doing something, since we the device is
> > > > suspended or
> > > > on
> > > > the way to being suspended. One example of such a scenario is a
> > > > watchdog like functionality: the watchdog work would
> > > > call pm_runtime_get_noidle() and check if the device is ok by
> > > > doing
> > > > some HW access, but only if the device is powered. Otherwise
> > > > the
> > > > work
> > > > item would do nothing (meaning it also won't reschedule
> > > > itself).
> > > > The
> > > > watchdog work would get rescheduled next time the device is
> > > > woken
> > > > up
> > > > and some work is submitted to the device.
> > > 
> > > So first of all the name "pm_runtime_get_noidle" doesn't make
> > > sense.
> > > 
> > > I guess what you need is something like
> > > 
> > > bool pm_runtime_get_if_active(struct device *dev)
> > > {
> > > 	unsigned log flags;
> > > 	bool ret;
> > > 
> > > 	spin_lock_irqsave(&dev->power.lock, flags);
> > > 
> > > 	if (dev->power.runtime_status == RPM_ACTIVE) {
> > 
> > But here usage_count could be zero, meaning that the device is
> > already
> > on the way to be suspended (autosuspend or ASYNC suspend), no?
> 
> The usage counter equal to 0 need not mean that the device is being
> suspended
> right now.

From the driver's point of view it means there is no need to keep the
device active, and that's the only thing that matters for the driver.
It doesn't matter at what exact point the actual suspend will happen
after the 1->0 transition.

> Also even if that's the case, the usage counter may be incremented at
> this very
> moment by a concurrent thread and you'll lose the opportunity to do
> what you
> want.

In that case the other thread makes sure that the work what we want to
do (run the watchdog check) is rescheduled. We need to handle that kind
of race anyway, since an increment from 0->1 and setting runtime_status
to RPM_ACTIVE could happen even after we have already determined here
that the device is not active and so we return failure.

> > In that case we don't want to return success. That would
> > unnecessarily prolong
> > the time the device is kept active.
> 
> Why?
> 
> If you have something to do if the device is active, then do it is
> the status
> is "active".  It really shouldn't matter when the device is going to
> be suspended
> going forward.

Let's say the last usage_counter reference is dropped and a suspend is
scheduled with some delay. Our watchdog gets to run now and sees that
the device has an RPM_ACTIVE state, while usage_count is 0. We know
that doing the watchdog check at this point is redundant, since nobody
is using the device. Also if the watchdog work would take a rpm
usage_count reference at this point, and the pending rpm_suspend
handler would run while the reference is held, it couldn't suspend the
device, the suspend would be delayed unnecessarily.

--Imre

> > > 		atomic_inc(&dev->power.usage_count);
> > > 		ret = true;
> > > 	} else {
> > > 		ret = false;
> > > 	}
> > > 
> > > 	spin_unlock_irqrestore(&dev->power.lock, flags);
> > > }
> > > 
> > > and the caller will simply bail out if "false" is returned, but
> > > if
> > > "true"
> > > is returned, it will have to drop the usage count, right?
> > 
> > Yes.
> 
> OK, I'll send a patch adding this function then.
> 
> Thanks,
> Rafael
> 

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Ulf Hansson]

On 11 December 2015 at 16:13, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, December 11, 2015 01:03:50 PM Ulf Hansson wrote:
>> [...]
>>
>> >> >
>> >> > Which basically means you can call pm_runtime_resume() just fine,
>> >> > because it will do nothing if the status is RPM_ACTIVE already.
>> >> >
>> >> > So really, why don't you use pm_runtime_get_sync()?
>> >>
>> >> The difference would be that if the status is not RPM_ACTIVE already we
>> >> would drop the reference and report error. The caller would in this
>> >> case forego of doing something, since we the device is suspended or on
>> >> the way to being suspended. One example of such a scenario is a
>> >> watchdog like functionality: the watchdog work would
>> >> call pm_runtime_get_noidle() and check if the device is ok by doing
>> >> some HW access, but only if the device is powered. Otherwise the work
>> >> item would do nothing (meaning it also won't reschedule itself). The
>> >> watchdog work would get rescheduled next time the device is woken up
>> >> and some work is submitted to the device.
>> >
>> > So first of all the name "pm_runtime_get_noidle" doesn't make sense.
>> >
>> > I guess what you need is something like
>> >
>> > bool pm_runtime_get_if_active(struct device *dev)
>> > {
>> >         unsigned log flags;
>> >         bool ret;
>> >
>> >         spin_lock_irqsave(&dev->power.lock, flags);
>> >
>> >         if (dev->power.runtime_status == RPM_ACTIVE) {
>> >                 atomic_inc(&dev->power.usage_count);
>> >                 ret = true;
>> >         } else {
>> >                 ret = false;
>> >         }
>> >
>> >         spin_unlock_irqrestore(&dev->power.lock, flags);
>> > }
>> >
>> > and the caller will simply bail out if "false" is returned, but if "true"
>> > is returned, it will have to drop the usage count, right?
>> >
>> > Thanks,
>> > Rafael
>> >
>>
>> Why not just:
>>
>> pm_runtime_get_noresume():
>> if (RPM_ACTIVE)
>>   "do some actions"
>> pm_runtime_put();
>
> Because that's racy?

Right, that was too easy. :-)

>
> What if the rpm_suspend() is running for the device, but it hasn't changed
> the status yet?

So if we can add a pm_runtime_barrier() or even simplifier, just hold
the spin_lock when checking if the rpm status is RPM_ACTIVE.

Kind regards
Uffe

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Friday, December 11, 2015 05:47:08 PM Imre Deak wrote:
> On pe, 2015-12-11 at 16:40 +0100, Rafael J. Wysocki wrote:
> > On Friday, December 11, 2015 02:54:45 PM Imre Deak wrote:
> > > On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> > > > On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
> > > > > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> > > > > > On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki
> > > > > > wrote:
> > > > > > > On Thursday, December 10, 2015 11:43:50 AM Imre Deak wrote:
> > > > > > > > On Thu, 2015-12-10 at 01:58 +0100, Rafael J. Wysocki
> > > > > > > > wrote:
> > > > > > > > > On Wednesday, December 09, 2015 06:22:19 PM Joonas
> > > > > > > > > Lahtinen
> > > > > > > > > wrote:
> > > > > > > > > > Introduce pm_runtime_get_noidle to for situations
> > > > > > > > > > where
> > > > > > > > > > it is
> > > > > > > > > > not
> > > > > > > > > > desireable to touch an idling device. One use
> > > > > > > > > > scenario is
> > > > > > > > > > periodic
> > > > > > > > > > hangchecks performed by the drm/i915 driver which can
> > > > > > > > > > be
> > > > > > > > > > omitted
> > > > > > > > > > on a device in a runtime idle state.
> > > > > > > > > > 
> > > > > > > > > > v2:
> > > > > > > > > > - Fix inconsistent return value when !CONFIG_PM.
> > > > > > > > > > - Update documentation for bool return value
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux
> > > > > > > > > > .int
> > > > > > > > > > el.c
> > > > > > > > > > om>
> > > > > > > > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > > > > > > > > Cc: linux-pm@vger.kernel.org
> > > > > > > > > 
> > > > > > > > > Well, I don't quite see how this can be used in a non-
> > > > > > > > > racy
> > > > > > > > > way
> > > > > > > > > without doing an additional pm_runtime_resume() or
> > > > > > > > > something
> > > > > > > > > like
> > > > > > > > > that in the same code path.
> > > > > > > > 
> > > > > > > > We don't want to resume, that would be the whole point.
> > > > > > > > We'd
> > > > > > > > like
> > > > > > > > to
> > > > > > > > ensure that we hold a reference _and_ the device is
> > > > > > > > already
> > > > > > > > active. So
> > > > > > > > AFAICS we'd need to check runtime_status == RPM_ACTIVE in
> > > > > > > > addition
> > > > > > > > after taking the reference.
> > > > > > > 
> > > > > > > Right, and that under the lock.
> > > > > > 
> > > > > > Which basically means you can call pm_runtime_resume() just
> > > > > > fine,
> > > > > > because it will do nothing if the status is RPM_ACTIVE
> > > > > > already.
> > > > > > 
> > > > > > So really, why don't you use pm_runtime_get_sync()?
> > > > > 
> > > > > The difference would be that if the status is not RPM_ACTIVE
> > > > > already we
> > > > > would drop the reference and report error. The caller would in
> > > > > this
> > > > > case forego of doing something, since we the device is
> > > > > suspended or
> > > > > on
> > > > > the way to being suspended. One example of such a scenario is a
> > > > > watchdog like functionality: the watchdog work would
> > > > > call pm_runtime_get_noidle() and check if the device is ok by
> > > > > doing
> > > > > some HW access, but only if the device is powered. Otherwise
> > > > > the
> > > > > work
> > > > > item would do nothing (meaning it also won't reschedule
> > > > > itself).
> > > > > The
> > > > > watchdog work would get rescheduled next time the device is
> > > > > woken
> > > > > up
> > > > > and some work is submitted to the device.
> > > > 
> > > > So first of all the name "pm_runtime_get_noidle" doesn't make
> > > > sense.
> > > > 
> > > > I guess what you need is something like
> > > > 
> > > > bool pm_runtime_get_if_active(struct device *dev)
> > > > {
> > > > 	unsigned log flags;
> > > > 	bool ret;
> > > > 
> > > > 	spin_lock_irqsave(&dev->power.lock, flags);
> > > > 
> > > > 	if (dev->power.runtime_status == RPM_ACTIVE) {
> > > 
> > > But here usage_count could be zero, meaning that the device is
> > > already
> > > on the way to be suspended (autosuspend or ASYNC suspend), no?
> > 
> > The usage counter equal to 0 need not mean that the device is being
> > suspended
> > right now.
> 
> From the driver's point of view it means there is no need to keep the
> device active, and that's the only thing that matters for the driver.
> It doesn't matter at what exact point the actual suspend will happen
> after the 1->0 transition.

I'm not following you, sorry.

You seem to be talking about a *specific* driver which I guess is i915.
That surely isn't the case for all drivers.

I don't even think this is the case for i915 to be honest, because some
code paths in the core do pm_runtime_get_noresume() without resuming the
device later or pm_runtime_put_noidle().

> > Also even if that's the case, the usage counter may be incremented at
> > this very
> > moment by a concurrent thread and you'll lose the opportunity to do
> > what you
> > want.
> 
> In that case the other thread makes sure that the work what we want to
> do (run the watchdog check) is rescheduled.

Yes, in *your* driver.  What about other drivers that may want to use this
new function?

If you want something that will be suitable to your driver only, you need
to open code it in there.

> We need to handle that kind
> of race anyway, since an increment from 0->1 and setting runtime_status
> to RPM_ACTIVE could happen even after we have already determined here
> that the device is not active and so we return failure.

Right, but my point is that the usage counter value generally doesn't
indicate what the status is and can change independently of the status
at any time.

Yes, my suggested function can be written like this:

bool pm_runtime_get_if_active(struct device *dev)
{
        unsigned log flags;
        bool ret = false;

        spin_lock_irqsave(&dev->power.lock, flags);

        if (dev->power.runtime_status == RPM_ACTIVE) {
                if (atomic_inc_return(&dev->power.usage_count) > 1)
                	ret = true;
		else
			atomic_dec(&dev->power.usage_count);
        }

        spin_unlock_irqrestore(&dev->power.lock, flags);
	return ret;
}

but this is obviously racy with respect to anyone concurrently changing the
usage counter.

> > > In that case we don't want to return success. That would
> > > unnecessarily prolong
> > > the time the device is kept active.
> > 
> > Why?
> > 
> > If you have something to do if the device is active, then do it is
> > the status
> > is "active".  It really shouldn't matter when the device is going to
> > be suspended
> > going forward.
> 
> Let's say the last usage_counter reference is dropped and a suspend is
> scheduled with some delay. Our watchdog gets to run now and sees that
> the device has an RPM_ACTIVE state, while usage_count is 0. We know
> that doing the watchdog check at this point is redundant, since nobody
> is using the device. Also if the watchdog work would take a rpm
> usage_count reference at this point, and the pending rpm_suspend
> handler would run while the reference is held, it couldn't suspend the
> device, the suspend would be delayed unnecessarily.

I see.  Then what about the function above?

Thanks,
Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Friday, December 11, 2015 04:59:45 PM Ulf Hansson wrote:
> On 11 December 2015 at 16:13, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, December 11, 2015 01:03:50 PM Ulf Hansson wrote:
> >> [...]
> >>
> >> >> >
> >> >> > Which basically means you can call pm_runtime_resume() just fine,
> >> >> > because it will do nothing if the status is RPM_ACTIVE already.
> >> >> >
> >> >> > So really, why don't you use pm_runtime_get_sync()?
> >> >>
> >> >> The difference would be that if the status is not RPM_ACTIVE already we
> >> >> would drop the reference and report error. The caller would in this
> >> >> case forego of doing something, since we the device is suspended or on
> >> >> the way to being suspended. One example of such a scenario is a
> >> >> watchdog like functionality: the watchdog work would
> >> >> call pm_runtime_get_noidle() and check if the device is ok by doing
> >> >> some HW access, but only if the device is powered. Otherwise the work
> >> >> item would do nothing (meaning it also won't reschedule itself). The
> >> >> watchdog work would get rescheduled next time the device is woken up
> >> >> and some work is submitted to the device.
> >> >
> >> > So first of all the name "pm_runtime_get_noidle" doesn't make sense.
> >> >
> >> > I guess what you need is something like
> >> >
> >> > bool pm_runtime_get_if_active(struct device *dev)
> >> > {
> >> >         unsigned log flags;
> >> >         bool ret;
> >> >
> >> >         spin_lock_irqsave(&dev->power.lock, flags);
> >> >
> >> >         if (dev->power.runtime_status == RPM_ACTIVE) {
> >> >                 atomic_inc(&dev->power.usage_count);
> >> >                 ret = true;
> >> >         } else {
> >> >                 ret = false;
> >> >         }
> >> >
> >> >         spin_unlock_irqrestore(&dev->power.lock, flags);
> >> > }
> >> >
> >> > and the caller will simply bail out if "false" is returned, but if "true"
> >> > is returned, it will have to drop the usage count, right?
> >> >
> >> > Thanks,
> >> > Rafael
> >> >
> >>
> >> Why not just:
> >>
> >> pm_runtime_get_noresume():
> >> if (RPM_ACTIVE)
> >>   "do some actions"
> >> pm_runtime_put();
> >
> > Because that's racy?
> 
> Right, that was too easy. :-)
> 
> >
> > What if the rpm_suspend() is running for the device, but it hasn't changed
> > the status yet?
> 
> So if we can add a pm_runtime_barrier() or even simplifier, just hold
> the spin_lock when checking if the rpm status is RPM_ACTIVE.

That would work too, but then we'd need to carry out one extra atomic operation.

Thanks,
Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Saturday, December 12, 2015 12:21:43 AM Rafael J. Wysocki wrote:
> On Friday, December 11, 2015 05:47:08 PM Imre Deak wrote:
> > On pe, 2015-12-11 at 16:40 +0100, Rafael J. Wysocki wrote:
> > > On Friday, December 11, 2015 02:54:45 PM Imre Deak wrote:
> > > > On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> > > > > On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
> > > > > > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> > > > > > > On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki

[cut]

> 
> Yes, my suggested function can be written like this:
> 
> bool pm_runtime_get_if_active(struct device *dev)
> {
>         unsigned log flags;
>         bool ret = false;
> 
>         spin_lock_irqsave(&dev->power.lock, flags);
> 
>         if (dev->power.runtime_status == RPM_ACTIVE) {
>                 if (atomic_inc_return(&dev->power.usage_count) > 1)
>                 	ret = true;
> 		else
> 			atomic_dec(&dev->power.usage_count);
>         }
> 
>         spin_unlock_irqrestore(&dev->power.lock, flags);
> 	return ret;
> }
> 
> but this is obviously racy with respect to anyone concurrently changing the
> usage counter.

Somethng like this would be slightly more efficient:

bool pm_runtime_get_if_in_use(struct device *dev)
{
        unsigned log flags;
        bool ret = false;

        spin_lock_irqsave(&dev->power.lock, flags);

        if (dev->power.runtime_status == RPM_ACTIVE)
                ret = !!atomic_inc_not_zero(&dev->power.usage_count);

        spin_unlock_irqrestore(&dev->power.lock, flags);
        return ret;
}

Thanks,
Rafael

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Saturday, December 12, 2015 12:41:06 AM Rafael J. Wysocki wrote:
> On Saturday, December 12, 2015 12:21:43 AM Rafael J. Wysocki wrote:
> > On Friday, December 11, 2015 05:47:08 PM Imre Deak wrote:
> > > On pe, 2015-12-11 at 16:40 +0100, Rafael J. Wysocki wrote:
> > > > On Friday, December 11, 2015 02:54:45 PM Imre Deak wrote:
> > > > > On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> > > > > > On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
> > > > > > > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> > > > > > > > On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki
> 
> [cut]
> 
> > 
> > Yes, my suggested function can be written like this:
> > 
> > bool pm_runtime_get_if_active(struct device *dev)
> > {
> >         unsigned log flags;
> >         bool ret = false;
> > 
> >         spin_lock_irqsave(&dev->power.lock, flags);
> > 
> >         if (dev->power.runtime_status == RPM_ACTIVE) {
> >                 if (atomic_inc_return(&dev->power.usage_count) > 1)
> >                 	ret = true;
> > 		else
> > 			atomic_dec(&dev->power.usage_count);
> >         }
> > 
> >         spin_unlock_irqrestore(&dev->power.lock, flags);
> > 	return ret;
> > }
> > 
> > but this is obviously racy with respect to anyone concurrently changing the
> > usage counter.
> 
> Somethng like this would be slightly more efficient:

And below is a patch to implement the thing, compile-tested only.

Please let me know if that's what you need.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: PM / runtime: Add new helper for conditional usage count incrementation

Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
that will increment the device's runtime PM usage counter and
return 'true' if its status is RPM_ACTIVE and its usage counter
is greater than 0 at the same time ('false' will be returned
otherwise).

This is useful for things that should only be done if the device
is active (from the runtime PM perspective) and used by somebody
(as indicated by the usage counter) already and engaging the device
otherwise would be overkill.

Requested-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/power/runtime_pm.txt |    5 +++++
 drivers/base/power/runtime.c       |   21 +++++++++++++++++++++
 include/linux/pm_runtime.h         |    5 +++++
 3 files changed, 31 insertions(+)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -966,6 +966,27 @@ int __pm_runtime_resume(struct device *d
 EXPORT_SYMBOL_GPL(__pm_runtime_resume);
 
 /**
+ * pm_runtime_get_if_in_use - Conditionally bump up the device's usage counter.
+ * @dev: Device to handle.
+ *
+ * Increment the device's runtime PM usage counter and return 'true' if its
+ * runtime PM status is RPM_ACTIVE and its usage counter is already different
+ * from zero at the same time.  Otherwise, return 'false'.
+ */
+bool pm_runtime_get_if_in_use(struct device *dev)
+{
+	unsigned long flags;
+	bool retval;
+
+	spin_lock_irqsave(&dev->power.lock, flags);
+	retval = dev->power.runtime_status == RPM_ACTIVE ?
+		!!atomic_inc_not_zero(&dev->power.usage_count) : false;
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+	return retval;
+}
+EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
+
+/**
  * __pm_runtime_set_status - Set runtime PM status of a device.
  * @dev: Device to handle.
  * @status: New runtime PM status of the device.
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -39,6 +39,7 @@ extern int pm_runtime_force_resume(struc
 extern int __pm_runtime_idle(struct device *dev, int rpmflags);
 extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
 extern int __pm_runtime_resume(struct device *dev, int rpmflags);
+extern bool pm_runtime_get_if_in_use(struct device *dev);
 extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
@@ -143,6 +144,10 @@ static inline int pm_schedule_suspend(st
 {
 	return -ENOSYS;
 }
+static inline bool pm_runtime_get_if_in_use(struct device *dev)
+{
+	return true;
+}
 static inline int __pm_runtime_set_status(struct device *dev,
 					    unsigned int status) { return 0; }
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }
Index: linux-pm/Documentation/power/runtime_pm.txt
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.txt
+++ linux-pm/Documentation/power/runtime_pm.txt
@@ -371,6 +371,11 @@ drivers/base/power/runtime.c and include
     - increment the device's usage counter, run pm_runtime_resume(dev) and
       return its result
 
+  bool pm_runtime_get_if_in_use(struct device *dev);
+    - increment the device's usage counter and return 'true' if its runtime PM
+      status is 'active' and its usage counter is greater than 0 at the same
+      time; return 'false' otherwise
+
   void pm_runtime_put_noidle(struct device *dev);
     - decrement the device's usage counter
 

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Imre Deak]

On Sat, 2015-12-12 at 02:51 +0100, Rafael J. Wysocki wrote:
> On Saturday, December 12, 2015 12:41:06 AM Rafael J. Wysocki wrote:
> > On Saturday, December 12, 2015 12:21:43 AM Rafael J. Wysocki wrote:
> > > On Friday, December 11, 2015 05:47:08 PM Imre Deak wrote:
> > > > On pe, 2015-12-11 at 16:40 +0100, Rafael J. Wysocki wrote:
> > > > > On Friday, December 11, 2015 02:54:45 PM Imre Deak wrote:
> > > > > > On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> > > > > > > On Thursday, December 10, 2015 11:20:40 PM Imre Deak
> > > > > > > wrote:
> > > > > > > > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki
> > > > > > > > wrote:
> > > > > > > > > On Thursday, December 10, 2015 10:36:37 PM Rafael J.
> > > > > > > > > Wysocki
> > 
> > [cut]
> > 
> > > 
> > > Yes, my suggested function can be written like this:
> > > 
> > > bool pm_runtime_get_if_active(struct device *dev)
> > > {
> > >         unsigned log flags;
> > >         bool ret = false;
> > > 
> > >         spin_lock_irqsave(&dev->power.lock, flags);
> > > 
> > >         if (dev->power.runtime_status == RPM_ACTIVE) {
> > >                 if (atomic_inc_return(&dev->power.usage_count) >
> > > 1)
> > >                 	ret = true;
> > > 		else
> > > 			atomic_dec(&dev->power.usage_count);
> > >         }
> > > 
> > >         spin_unlock_irqrestore(&dev->power.lock, flags);
> > > 	return ret;
> > > }
> > > 
> > > but this is obviously racy with respect to anyone concurrently
> > > changing the
> > > usage counter.
> > 
> > Somethng like this would be slightly more efficient:
> 
> And below is a patch to implement the thing, compile-tested only.
> 
> Please let me know if that's what you need.
> 
> Thanks,
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: PM / runtime: Add new helper for conditional usage count
> incrementation
> 
> Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
> that will increment the device's runtime PM usage counter and
> return 'true' if its status is RPM_ACTIVE and its usage counter
> is greater than 0 at the same time ('false' will be returned
> otherwise).
> 
> This is useful for things that should only be done if the device
> is active (from the runtime PM perspective) and used by somebody
> (as indicated by the usage counter) already and engaging the device
> otherwise would be overkill.
> 
> Requested-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  Documentation/power/runtime_pm.txt |    5 +++++
>  drivers/base/power/runtime.c       |   21 +++++++++++++++++++++
>  include/linux/pm_runtime.h         |    5 +++++
>  3 files changed, 31 insertions(+)
> 
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -966,6 +966,27 @@ int __pm_runtime_resume(struct device *d
>  EXPORT_SYMBOL_GPL(__pm_runtime_resume);
>  
>  /**
> + * pm_runtime_get_if_in_use - Conditionally bump up the device's
> usage counter.
> + * @dev: Device to handle.
> + *
> + * Increment the device's runtime PM usage counter and return 'true'
> if its
> + * runtime PM status is RPM_ACTIVE and its usage counter is already
> different
> + * from zero at the same time.  Otherwise, return 'false'.
> + */
> +bool pm_runtime_get_if_in_use(struct device *dev)
> +{
> +	unsigned long flags;
> +	bool retval;
> +
> +	spin_lock_irqsave(&dev->power.lock, flags);
> +	retval = dev->power.runtime_status == RPM_ACTIVE ?
> +		!!atomic_inc_not_zero(&dev->power.usage_count) :
> false;
> +	spin_unlock_irqrestore(&dev->power.lock, flags);
> +	return retval;
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> +

To me this looks ok:
Acked-by: Imre Deak <imre.deak@intel.com>

The original idea for this logic came from Chris so he may have some
comments.

Thanks,
Imre

> +/**
>   * __pm_runtime_set_status - Set runtime PM status of a device.
>   * @dev: Device to handle.
>   * @status: New runtime PM status of the device.
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -39,6 +39,7 @@ extern int pm_runtime_force_resume(struc
>  extern int __pm_runtime_idle(struct device *dev, int rpmflags);
>  extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
>  extern int __pm_runtime_resume(struct device *dev, int rpmflags);
> +extern bool pm_runtime_get_if_in_use(struct device *dev);
>  extern int pm_schedule_suspend(struct device *dev, unsigned int
> delay);
>  extern int __pm_runtime_set_status(struct device *dev, unsigned int
> status);
>  extern int pm_runtime_barrier(struct device *dev);
> @@ -143,6 +144,10 @@ static inline int pm_schedule_suspend(st
>  {
>  	return -ENOSYS;
>  }
> +static inline bool pm_runtime_get_if_in_use(struct device *dev)
> +{
> +	return true;
> +}
>  static inline int __pm_runtime_set_status(struct device *dev,
>  					    unsigned int status) {
> return 0; }
>  static inline int pm_runtime_barrier(struct device *dev) { return 0;
> }
> Index: linux-pm/Documentation/power/runtime_pm.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -371,6 +371,11 @@ drivers/base/power/runtime.c and include
>      - increment the device's usage counter, run
> pm_runtime_resume(dev) and
>        return its result
>  
> +  bool pm_runtime_get_if_in_use(struct device *dev);
> +    - increment the device's usage counter and return 'true' if its
> runtime PM
> +      status is 'active' and its usage counter is greater than 0 at
> the same
> +      time; return 'false' otherwise
> +
>    void pm_runtime_put_noidle(struct device *dev);
>      - decrement the device's usage counter
>  
> 

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Chris Wilson]

On Sat, Dec 12, 2015 at 09:40:45PM +0200, Imre Deak wrote:
> On Sat, 2015-12-12 at 02:51 +0100, Rafael J. Wysocki wrote:
> > +bool pm_runtime_get_if_in_use(struct device *dev)
> > +{
> > +	unsigned long flags;
> > +	bool retval;
> > +
> > +	spin_lock_irqsave(&dev->power.lock, flags);
> > +	retval = dev->power.runtime_status == RPM_ACTIVE ?
> > +		!!atomic_inc_not_zero(&dev->power.usage_count) :
> > false;
> > +	spin_unlock_irqrestore(&dev->power.lock, flags);
> > +	return retval;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> > +
> 
> To me this looks ok:
> Acked-by: Imre Deak <imre.deak@intel.com>

Pendant says
retval = (dev->power.runtime_status == RPM_ACTIVE &&
	  atomic_inc_not_zero(&dev->power.usage_count);
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH v2] PM / Runtime: Introduce pm_runtime_get_noidle

[Rafael J. Wysocki]

On Saturday, December 12, 2015 07:49:56 PM Chris Wilson wrote:
> On Sat, Dec 12, 2015 at 09:40:45PM +0200, Imre Deak wrote:
> > On Sat, 2015-12-12 at 02:51 +0100, Rafael J. Wysocki wrote:
> > > +bool pm_runtime_get_if_in_use(struct device *dev)
> > > +{
> > > +	unsigned long flags;
> > > +	bool retval;
> > > +
> > > +	spin_lock_irqsave(&dev->power.lock, flags);
> > > +	retval = dev->power.runtime_status == RPM_ACTIVE ?
> > > +		!!atomic_inc_not_zero(&dev->power.usage_count) :
> > > false;
> > > +	spin_unlock_irqrestore(&dev->power.lock, flags);
> > > +	return retval;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> > > +
> > 
> > To me this looks ok:
> > Acked-by: Imre Deak <imre.deak@intel.com>
> 
> Pendant says
> retval = (dev->power.runtime_status == RPM_ACTIVE &&
> 	  atomic_inc_not_zero(&dev->power.usage_count);

Well, this wouldn't build AFAICS.

retval = dev->power.runtime_status == RPM_ACTIVE &&
	atomic_inc_not_zero(&dev->power.usage_count);

Thanks,
Rafael

[PATCH] PM / runtime: Add new helper for conditional usage count incrementation

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
that will increment the device's runtime PM usage counter and
return 'true' if its status is RPM_ACTIVE and its usage counter
is greater than 0 at the same time ('false' will be returned
otherwise).

This is useful for things that should only be done if the device
is active (from the runtime PM perspective) and used by somebody
(as indicated by the usage counter) already and they are not worth
bothering otherwise.

Requested-by: Imre Deak <imre.deak@intel.com>
Acked-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/power/runtime_pm.txt |    5 +++++
 drivers/base/power/runtime.c       |   21 +++++++++++++++++++++
 include/linux/pm_runtime.h         |    5 +++++
 3 files changed, 31 insertions(+)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -966,6 +966,27 @@ int __pm_runtime_resume(struct device *d
 EXPORT_SYMBOL_GPL(__pm_runtime_resume);
 
 /**
+ * pm_runtime_get_if_in_use - Conditionally bump up the device's usage counter.
+ * @dev: Device to handle.
+ *
+ * Increment the device's runtime PM usage counter and return 'true' if its
+ * runtime PM status is RPM_ACTIVE and its usage counter is already different
+ * from zero at the same time.  Otherwise, return 'false'.
+ */
+bool pm_runtime_get_if_in_use(struct device *dev)
+{
+	unsigned long flags;
+	bool retval;
+
+	spin_lock_irqsave(&dev->power.lock, flags);
+	retval = dev->power.runtime_status == RPM_ACTIVE
+		&& atomic_inc_not_zero(&dev->power.usage_count);
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+	return retval;
+}
+EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
+
+/**
  * __pm_runtime_set_status - Set runtime PM status of a device.
  * @dev: Device to handle.
  * @status: New runtime PM status of the device.
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -39,6 +39,7 @@ extern int pm_runtime_force_resume(struc
 extern int __pm_runtime_idle(struct device *dev, int rpmflags);
 extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
 extern int __pm_runtime_resume(struct device *dev, int rpmflags);
+extern bool pm_runtime_get_if_in_use(struct device *dev);
 extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
@@ -143,6 +144,10 @@ static inline int pm_schedule_suspend(st
 {
 	return -ENOSYS;
 }
+static inline bool pm_runtime_get_if_in_use(struct device *dev)
+{
+	return true;
+}
 static inline int __pm_runtime_set_status(struct device *dev,
 					    unsigned int status) { return 0; }
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }
Index: linux-pm/Documentation/power/runtime_pm.txt
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.txt
+++ linux-pm/Documentation/power/runtime_pm.txt
@@ -371,6 +371,11 @@ drivers/base/power/runtime.c and include
     - increment the device's usage counter, run pm_runtime_resume(dev) and
       return its result
 
+  bool pm_runtime_get_if_in_use(struct device *dev);
+    - increment the device's usage counter and return 'true' if its runtime PM
+      status is 'active' and its usage counter is greater than 0 at the same
+      time; return 'false' otherwise
+
   void pm_runtime_put_noidle(struct device *dev);
     - decrement the device's usage counter
 

Re: [PATCH] PM / runtime: Add new helper for conditional usage count incrementation

[Joonas Lahtinen]

On ma, 2015-12-14 at 23:22 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
> that will increment the device's runtime PM usage counter and
> return 'true' if its status is RPM_ACTIVE and its usage counter
> is greater than 0 at the same time ('false' will be returned
> otherwise).
> 
> This is useful for things that should only be done if the device
> is active (from the runtime PM perspective) and used by somebody
> (as indicated by the usage counter) already and they are not worth
> bothering otherwise.
> 
> Requested-by: Imre Deak <imre.deak@intel.com>
> Acked-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  Documentation/power/runtime_pm.txt |    5 +++++
>  drivers/base/power/runtime.c       |   21 +++++++++++++++++++++
>  include/linux/pm_runtime.h         |    5 +++++
>  3 files changed, 31 insertions(+)
> 
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -966,6 +966,27 @@ int __pm_runtime_resume(struct device *d
>  EXPORT_SYMBOL_GPL(__pm_runtime_resume);
>  
>  /**
> + * pm_runtime_get_if_in_use - Conditionally bump up the device's
> usage counter.
> + * @dev: Device to handle.
> + *
> + * Increment the device's runtime PM usage counter and return 'true'
> if its
> + * runtime PM status is RPM_ACTIVE and its usage counter is already
> different
> + * from zero at the same time.  Otherwise, return 'false'.
> + */
> +bool pm_runtime_get_if_in_use(struct device *dev)
> +{
> +	unsigned long flags;
> +	bool retval;
> +
> +	spin_lock_irqsave(&dev->power.lock, flags);
> +	retval = dev->power.runtime_status == RPM_ACTIVE
> +		&& atomic_inc_not_zero(&dev->power.usage_count);
> +	spin_unlock_irqrestore(&dev->power.lock, flags);
> +	return retval;
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> +
> +/**
>   * __pm_runtime_set_status - Set runtime PM status of a device.
>   * @dev: Device to handle.
>   * @status: New runtime PM status of the device.
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -39,6 +39,7 @@ extern int pm_runtime_force_resume(struc
>  extern int __pm_runtime_idle(struct device *dev, int rpmflags);
>  extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
>  extern int __pm_runtime_resume(struct device *dev, int rpmflags);
> +extern bool pm_runtime_get_if_in_use(struct device *dev);
>  extern int pm_schedule_suspend(struct device *dev, unsigned int
> delay);
>  extern int __pm_runtime_set_status(struct device *dev, unsigned int
> status);
>  extern int pm_runtime_barrier(struct device *dev);
> @@ -143,6 +144,10 @@ static inline int pm_schedule_suspend(st
>  {
>  	return -ENOSYS;
>  }
> +static inline bool pm_runtime_get_if_in_use(struct device *dev)
> +{
> +	return true;
> +}
>  static inline int __pm_runtime_set_status(struct device *dev,
>  					    unsigned int status) {
> return 0; }
>  static inline int pm_runtime_barrier(struct device *dev) { return 0;
> }
> Index: linux-pm/Documentation/power/runtime_pm.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -371,6 +371,11 @@ drivers/base/power/runtime.c and include
>      - increment the device's usage counter, run
> pm_runtime_resume(dev) and
>        return its result
>  
> +  bool pm_runtime_get_if_in_use(struct device *dev);
> +    - increment the device's usage counter and return 'true' if its
> runtime PM
> +      status is 'active' and its usage counter is greater than 0 at
> the same
> +      time; return 'false' otherwise
> +
>    void pm_runtime_put_noidle(struct device *dev);
>      - decrement the device's usage counter
>  
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] PM / runtime: Add new helper for conditional usage count incrementation

[Ulf Hansson]

On 14 December 2015 at 23:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Introduce a new runtime PM function, pm_runtime_get_if_in_use(),

As we already have pm_runtime_set_active() and pm_runtime_active(),
changing the new function name to "pm_runtime_get_if_active" may be
better!?

> that will increment the device's runtime PM usage counter and
> return 'true' if its status is RPM_ACTIVE and its usage counter
> is greater than 0 at the same time ('false' will be returned
> otherwise).
>
> This is useful for things that should only be done if the device
> is active (from the runtime PM perspective) and used by somebody
> (as indicated by the usage counter) already and they are not worth
> bothering otherwise.
>
> Requested-by: Imre Deak <imre.deak@intel.com>
> Acked-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  Documentation/power/runtime_pm.txt |    5 +++++
>  drivers/base/power/runtime.c       |   21 +++++++++++++++++++++
>  include/linux/pm_runtime.h         |    5 +++++
>  3 files changed, 31 insertions(+)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -966,6 +966,27 @@ int __pm_runtime_resume(struct device *d
>  EXPORT_SYMBOL_GPL(__pm_runtime_resume);
>
>  /**
> + * pm_runtime_get_if_in_use - Conditionally bump up the device's usage counter.
> + * @dev: Device to handle.
> + *
> + * Increment the device's runtime PM usage counter and return 'true' if its
> + * runtime PM status is RPM_ACTIVE and its usage counter is already different
> + * from zero at the same time.  Otherwise, return 'false'.
> + */
> +bool pm_runtime_get_if_in_use(struct device *dev)
> +{
> +       unsigned long flags;
> +       bool retval;
> +
> +       spin_lock_irqsave(&dev->power.lock, flags);
> +       retval = dev->power.runtime_status == RPM_ACTIVE
> +               && atomic_inc_not_zero(&dev->power.usage_count);

Don't we also need to check that runtime PM is enabled (&&
!dev->power.disable_depth), or the user of this function don't care
about that?

> +       spin_unlock_irqrestore(&dev->power.lock, flags);
> +       return retval;
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> +

[...]

Kind regards
Uffe

Re: [PATCH] PM / runtime: Add new helper for conditional usage count incrementation

[Alan Stern]

On Mon, 14 Dec 2015, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
> that will increment the device's runtime PM usage counter and
> return 'true' if its status is RPM_ACTIVE and its usage counter
> is greater than 0 at the same time ('false' will be returned
> otherwise).
> 
> This is useful for things that should only be done if the device
> is active (from the runtime PM perspective) and used by somebody
> (as indicated by the usage counter) already and they are not worth
> bothering otherwise.


> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -966,6 +966,27 @@ int __pm_runtime_resume(struct device *d
>  EXPORT_SYMBOL_GPL(__pm_runtime_resume);
>  
>  /**
> + * pm_runtime_get_if_in_use - Conditionally bump up the device's usage counter.
> + * @dev: Device to handle.
> + *
> + * Increment the device's runtime PM usage counter and return 'true' if its
> + * runtime PM status is RPM_ACTIVE and its usage counter is already different
> + * from zero at the same time.  Otherwise, return 'false'.

The phrasing of this comment is slightly ambiguous (it's not clear 
whether the "if" clause applies to both the increment and the return 
or just the return).  IMO it would be somewhat better to write:

	If the runtime PM status is RPM_ACTIVE and the runtime PM usage
	counter is nonzero, increment the counter and return 'true'.
	Otherwise return false without changing the counter.

> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -371,6 +371,11 @@ drivers/base/power/runtime.c and include
>      - increment the device's usage counter, run pm_runtime_resume(dev) and
>        return its result
>  
> +  bool pm_runtime_get_if_in_use(struct device *dev);
> +    - increment the device's usage counter and return 'true' if its runtime PM
> +      status is 'active' and its usage counter is greater than 0 at the same
> +      time; return 'false' otherwise

Same thing here.

Alan Stern

Re: [PATCH] PM / runtime: Add new helper for conditional usage count incrementation

[Rafael J. Wysocki]

On Tuesday, December 15, 2015 03:28:54 PM Ulf Hansson wrote:
> On 14 December 2015 at 23:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
> 
> As we already have pm_runtime_set_active() and pm_runtime_active(),
> changing the new function name to "pm_runtime_get_if_active" may be
> better!?

"In use" is supposed to mean "active and reference counted".

> > that will increment the device's runtime PM usage counter and
> > return 'true' if its status is RPM_ACTIVE and its usage counter
> > is greater than 0 at the same time ('false' will be returned
> > otherwise).
> >
> > This is useful for things that should only be done if the device
> > is active (from the runtime PM perspective) and used by somebody
> > (as indicated by the usage counter) already and they are not worth
> > bothering otherwise.
> >
> > Requested-by: Imre Deak <imre.deak@intel.com>
> > Acked-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  Documentation/power/runtime_pm.txt |    5 +++++
> >  drivers/base/power/runtime.c       |   21 +++++++++++++++++++++
> >  include/linux/pm_runtime.h         |    5 +++++
> >  3 files changed, 31 insertions(+)
> >
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -966,6 +966,27 @@ int __pm_runtime_resume(struct device *d
> >  EXPORT_SYMBOL_GPL(__pm_runtime_resume);
> >
> >  /**
> > + * pm_runtime_get_if_in_use - Conditionally bump up the device's usage counter.
> > + * @dev: Device to handle.
> > + *
> > + * Increment the device's runtime PM usage counter and return 'true' if its
> > + * runtime PM status is RPM_ACTIVE and its usage counter is already different
> > + * from zero at the same time.  Otherwise, return 'false'.
> > + */
> > +bool pm_runtime_get_if_in_use(struct device *dev)
> > +{
> > +       unsigned long flags;
> > +       bool retval;
> > +
> > +       spin_lock_irqsave(&dev->power.lock, flags);
> > +       retval = dev->power.runtime_status == RPM_ACTIVE
> > +               && atomic_inc_not_zero(&dev->power.usage_count);
> 
> Don't we also need to check that runtime PM is enabled (&&
> !dev->power.disable_depth), or the user of this function don't care
> about that?

The user probably cares, but calling this for devices with runtime PM
disabled doesn't really make sense to me (the status is not meaningful
then).

> > +       spin_unlock_irqrestore(&dev->power.lock, flags);
> > +       return retval;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> > +
> 
> [...]

Thanks,
Rafael

Re: [PATCH] PM / runtime: Add new helper for conditional usage count incrementation

[Rafael J. Wysocki]

On Tuesday, December 15, 2015 10:06:33 AM Alan Stern wrote:
> On Mon, 14 Dec 2015, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
> > that will increment the device's runtime PM usage counter and
> > return 'true' if its status is RPM_ACTIVE and its usage counter
> > is greater than 0 at the same time ('false' will be returned
> > otherwise).
> > 
> > This is useful for things that should only be done if the device
> > is active (from the runtime PM perspective) and used by somebody
> > (as indicated by the usage counter) already and they are not worth
> > bothering otherwise.
> 
> 
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -966,6 +966,27 @@ int __pm_runtime_resume(struct device *d
> >  EXPORT_SYMBOL_GPL(__pm_runtime_resume);
> >  
> >  /**
> > + * pm_runtime_get_if_in_use - Conditionally bump up the device's usage counter.
> > + * @dev: Device to handle.
> > + *
> > + * Increment the device's runtime PM usage counter and return 'true' if its
> > + * runtime PM status is RPM_ACTIVE and its usage counter is already different
> > + * from zero at the same time.  Otherwise, return 'false'.
> 
> The phrasing of this comment is slightly ambiguous (it's not clear 
> whether the "if" clause applies to both the increment and the return 
> or just the return).  IMO it would be somewhat better to write:
> 
> 	If the runtime PM status is RPM_ACTIVE and the runtime PM usage
> 	counter is nonzero, increment the counter and return 'true'.
> 	Otherwise return false without changing the counter.

Yes, that sounds better, thanks!

I'll resend the patch with that modification.

Thanks,
Rafael

[PATCH v2] PM / runtime: Add new helper for conditional usage count incrementation

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
that will increment the device's runtime PM usage counter and
return 'true' if its status is RPM_ACTIVE and its usage counter
is greater than 0 at the same time ('false' will be returned
otherwise).

This is useful for things that should only be done if the device
is active (from the runtime PM perspective) and used by somebody
(as indicated by the usage counter) already and they are not worth
bothering otherwise.

Requested-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Changes from v1:
- kerneldoc and runtime PM doc changes as suggested by Alan.
- The new function returns an int now and -EINVAL is returned for devices
  with runtime PM disabled.

---
 Documentation/power/runtime_pm.txt |    6 ++++++
 drivers/base/power/runtime.c       |   24 ++++++++++++++++++++++++
 include/linux/pm_runtime.h         |    5 +++++
 3 files changed, 35 insertions(+)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -966,6 +966,30 @@ int __pm_runtime_resume(struct device *d
 EXPORT_SYMBOL_GPL(__pm_runtime_resume);
 
 /**
+ * pm_runtime_get_if_in_use - Conditionally bump up the device's usage counter.
+ * @dev: Device to handle.
+ *
+ * Return -EINVAL if runtime PM is disabled for the device.
+ *
+ * If that's not the case and if the device's runtime PM status is RPM_ACTIVE
+ * and the runtime PM usage counter is nonzero, increment the counter and
+ * return 1.  Otherwise return 0 without changing the counter.
+ */
+int pm_runtime_get_if_in_use(struct device *dev)
+{
+	unsigned long flags;
+	int retval;
+
+	spin_lock_irqsave(&dev->power.lock, flags);
+	retval = dev->power.disable_depth > 0 ? -EINVAL :
+		dev->power.runtime_status == RPM_ACTIVE
+			&& atomic_inc_not_zero(&dev->power.usage_count);
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+	return retval;
+}
+EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
+
+/**
  * __pm_runtime_set_status - Set runtime PM status of a device.
  * @dev: Device to handle.
  * @status: New runtime PM status of the device.
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -39,6 +39,7 @@ extern int pm_runtime_force_resume(struc
 extern int __pm_runtime_idle(struct device *dev, int rpmflags);
 extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
 extern int __pm_runtime_resume(struct device *dev, int rpmflags);
+extern int pm_runtime_get_if_in_use(struct device *dev);
 extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
@@ -143,6 +144,10 @@ static inline int pm_schedule_suspend(st
 {
 	return -ENOSYS;
 }
+static inline int pm_runtime_get_if_in_use(struct device *dev)
+{
+	return -EINVAL;
+}
 static inline int __pm_runtime_set_status(struct device *dev,
 					    unsigned int status) { return 0; }
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }
Index: linux-pm/Documentation/power/runtime_pm.txt
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.txt
+++ linux-pm/Documentation/power/runtime_pm.txt
@@ -371,6 +371,12 @@ drivers/base/power/runtime.c and include
     - increment the device's usage counter, run pm_runtime_resume(dev) and
       return its result
 
+  int pm_runtime_get_if_in_use(struct device *dev);
+    - return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
+      runtime PM status is RPM_ACTIVE and the runtime PM usage counter is
+      nonzero, increment the counter and return 1; otherwise return 0 without
+      changing the counter
+
   void pm_runtime_put_noidle(struct device *dev);
     - decrement the device's usage counter
 

Re: [PATCH v2] PM / runtime: Add new helper for conditional usage count incrementation

[Ulf Hansson]

On 17 December 2015 at 02:54, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
> that will increment the device's runtime PM usage counter and
> return 'true' if its status is RPM_ACTIVE and its usage counter
> is greater than 0 at the same time ('false' will be returned
> otherwise).
>
> This is useful for things that should only be done if the device
> is active (from the runtime PM perspective) and used by somebody
> (as indicated by the usage counter) already and they are not worth
> bothering otherwise.
>
> Requested-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>
> Changes from v1:
> - kerneldoc and runtime PM doc changes as suggested by Alan.
> - The new function returns an int now and -EINVAL is returned for devices
>   with runtime PM disabled.
>
> ---
>  Documentation/power/runtime_pm.txt |    6 ++++++
>  drivers/base/power/runtime.c       |   24 ++++++++++++++++++++++++
>  include/linux/pm_runtime.h         |    5 +++++
>  3 files changed, 35 insertions(+)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -966,6 +966,30 @@ int __pm_runtime_resume(struct device *d
>  EXPORT_SYMBOL_GPL(__pm_runtime_resume);
>
>  /**
> + * pm_runtime_get_if_in_use - Conditionally bump up the device's usage counter.
> + * @dev: Device to handle.
> + *
> + * Return -EINVAL if runtime PM is disabled for the device.
> + *
> + * If that's not the case and if the device's runtime PM status is RPM_ACTIVE
> + * and the runtime PM usage counter is nonzero, increment the counter and
> + * return 1.  Otherwise return 0 without changing the counter.
> + */
> +int pm_runtime_get_if_in_use(struct device *dev)
> +{
> +       unsigned long flags;
> +       int retval;
> +
> +       spin_lock_irqsave(&dev->power.lock, flags);
> +       retval = dev->power.disable_depth > 0 ? -EINVAL :
> +               dev->power.runtime_status == RPM_ACTIVE
> +                       && atomic_inc_not_zero(&dev->power.usage_count);
> +       spin_unlock_irqrestore(&dev->power.lock, flags);
> +       return retval;
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> +
> +/**
>   * __pm_runtime_set_status - Set runtime PM status of a device.
>   * @dev: Device to handle.
>   * @status: New runtime PM status of the device.
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -39,6 +39,7 @@ extern int pm_runtime_force_resume(struc
>  extern int __pm_runtime_idle(struct device *dev, int rpmflags);
>  extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
>  extern int __pm_runtime_resume(struct device *dev, int rpmflags);
> +extern int pm_runtime_get_if_in_use(struct device *dev);
>  extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
>  extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
>  extern int pm_runtime_barrier(struct device *dev);
> @@ -143,6 +144,10 @@ static inline int pm_schedule_suspend(st
>  {
>         return -ENOSYS;
>  }
> +static inline int pm_runtime_get_if_in_use(struct device *dev)
> +{
> +       return -EINVAL;
> +}
>  static inline int __pm_runtime_set_status(struct device *dev,
>                                             unsigned int status) { return 0; }
>  static inline int pm_runtime_barrier(struct device *dev) { return 0; }
> Index: linux-pm/Documentation/power/runtime_pm.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -371,6 +371,12 @@ drivers/base/power/runtime.c and include
>      - increment the device's usage counter, run pm_runtime_resume(dev) and
>        return its result
>
> +  int pm_runtime_get_if_in_use(struct device *dev);
> +    - return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
> +      runtime PM status is RPM_ACTIVE and the runtime PM usage counter is
> +      nonzero, increment the counter and return 1; otherwise return 0 without
> +      changing the counter
> +
>    void pm_runtime_put_noidle(struct device *dev);
>      - decrement the device's usage counter
>
>