* [PATCH v2 0/7] Small Runtime PM API changes
@ 2023-11-17 11:14 Sakari Ailus
2023-11-17 11:14 ` [PATCH v2 1/7] pm: runtime: Simplify pm_runtime_get_if_active() usage Sakari Ailus
` (6 more replies)
0 siblings, 7 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-11-17 11:14 UTC (permalink / raw
To: linux-acpi; +Cc: linux-media, rafael, jacopo.mondi, laurent.pinchart
Hi folks,
This small set happily mixes Runtime PM and media patches.
The set does two main things Runtime PM API-wise. Firstly,
pm_runtime_get_if_active() is made more user-friendly by removing the
ign_use_count argument so the users no longer need to call it with that
set to true. Secondly, pm_runtime_put_mark_busy_autosusp() helper is added
to avoid drivers having to call pm_runtime_mark_last_busy() only to be
followed by pm_runtime_autosuspend().
The vast majority of the users of pm_runtime_autosuspend() would probably
have been fine with making pm_runtime_autosuspend() do the last busy
stamping, too, but given the sheer number of users it's hard to tell if
there could be problems here and there. On the other hand, there are
probably a sizable proportion of call sites where the missing
pm_runtime_mark_last_busy() call is simply a bug.
The three last patches are addressing Runtime PM issues in a few sensor
drivers.
Comments would be welcome.
since v1:
- Fix a compilation issue when CONFIG_PM is disabled in the first patch.
- Improve the documentation patch, assume the use of autosuspend (this
generally makes sense for camera sensor drivers).
- Keep using pm_runtime_get_if_in_use() in imx319 and imx219 drivers (they
don't use autosuspend).
- Add a patch to document acpi_dev_state_d0() in conjunction of non-D0
probe.
Sakari Ailus (7):
pm: runtime: Simplify pm_runtime_get_if_active() usage
pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
ACPI: Documentation: Document acpi_dev_state_d0()
media: Documentation: Improve camera sensor runtime PM documentation
media: ov8858: Use pm_runtime_get_if_active(), put usage_count
correctly
media: imx319: Put usage_count correctly in s_ctrl callback
media: imx219: Put usage_count correctly in s_ctrl callback
.../driver-api/media/camera-sensor.rst | 76 +++++++++++++------
.../firmware-guide/acpi/non-d0-probe.rst | 10 +++
Documentation/power/runtime_pm.rst | 5 +-
drivers/base/power/runtime.c | 9 ++-
drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
drivers/media/i2c/ccs/ccs-core.c | 2 +-
drivers/media/i2c/imx219.c | 8 +-
drivers/media/i2c/imx319.c | 8 +-
drivers/media/i2c/ov8858.c | 8 +-
drivers/net/ipa/ipa_smp2p.c | 2 +-
drivers/pci/pci.c | 2 +-
include/linux/pm_runtime.h | 49 ++++++++++--
sound/hda/hdac_device.c | 2 +-
13 files changed, 133 insertions(+), 50 deletions(-)
base-commit: 3e238417254bfdcc23fe207780b59cbb08656762
--
2.39.2
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/7] pm: runtime: Simplify pm_runtime_get_if_active() usage
2023-11-17 11:14 [PATCH v2 0/7] Small Runtime PM API changes Sakari Ailus
@ 2023-11-17 11:14 ` Sakari Ailus
2023-11-18 17:46 ` Laurent Pinchart
2023-11-17 11:14 ` [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper Sakari Ailus
` (5 subsequent siblings)
6 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-17 11:14 UTC (permalink / raw
To: linux-acpi; +Cc: linux-media, rafael, jacopo.mondi, laurent.pinchart
The majority of the callers currently using pm_runtime_get_if_active()
call it with ign_usage_count argument set to true. There's only one driver
using this feature (and natually implementation of
pm_runtime_get_if_in_use()).
To make this function more practical to use, remove the ign_usage_count
argument from the function. The main implementation is renamed as
__pm_runtime_get_conditional().
This function is expected to gain a large number of users in the future
--- camera sensor drivers using runtime autosuspend have a need to get the
device's usage_count conditionally if it is enabled. Most of these
currently use pm_runtime_get_if_in_use(), which is a bug.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v1:
- Fix __pm_runtime_get_if_conditional() un-CONFIG_PM implementation.
---
Documentation/power/runtime_pm.rst | 5 ++--
drivers/base/power/runtime.c | 9 ++++---
drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
drivers/media/i2c/ccs/ccs-core.c | 2 +-
drivers/net/ipa/ipa_smp2p.c | 2 +-
drivers/pci/pci.c | 2 +-
include/linux/pm_runtime.h | 32 +++++++++++++++++++++----
sound/hda/hdac_device.c | 2 +-
8 files changed, 40 insertions(+), 16 deletions(-)
diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
index 65b86e487afe..da99379071a4 100644
--- a/Documentation/power/runtime_pm.rst
+++ b/Documentation/power/runtime_pm.rst
@@ -396,10 +396,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
nonzero, increment the counter and return 1; otherwise return 0 without
changing the counter
- `int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count);`
+ `int pm_runtime_get_if_active(struct device *dev);`
- return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
- runtime PM status is RPM_ACTIVE, and either ign_usage_count is true
- or the device's usage_count is non-zero, increment the counter and
+ runtime PM status is RPM_ACTIVE, increment the counter and
return 1; otherwise return 0 without changing the counter
`void pm_runtime_put_noidle(struct device *dev);`
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 4545669cb973..8b56468eca9d 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1175,7 +1175,7 @@ int __pm_runtime_resume(struct device *dev, int rpmflags)
EXPORT_SYMBOL_GPL(__pm_runtime_resume);
/**
- * pm_runtime_get_if_active - Conditionally bump up device usage counter.
+ * __pm_runtime_get_conditional - Conditionally bump up device usage counter.
* @dev: Device to handle.
* @ign_usage_count: Whether or not to look at the current usage counter value.
*
@@ -1195,8 +1195,11 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume);
*
* The caller is responsible for decrementing the runtime PM usage counter of
* @dev after this function has returned a positive value for it.
+ *
+ * This function is not intended to be called by drivers, use
+ * pm_runtime_get_if_active() or pm_runtime_get_if_in_use() instead.
*/
-int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count)
+int __pm_runtime_get_conditional(struct device *dev, bool ign_usage_count)
{
unsigned long flags;
int retval;
@@ -1217,7 +1220,7 @@ int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count)
return retval;
}
-EXPORT_SYMBOL_GPL(pm_runtime_get_if_active);
+EXPORT_SYMBOL_GPL(__pm_runtime_get_conditional);
/**
* __pm_runtime_set_status - Set runtime PM status of a device.
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6d8e5e5c0cba..b163efe80975 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -434,7 +434,7 @@ static intel_wakeref_t __intel_runtime_pm_get_if_active(struct intel_runtime_pm
* function, since the power state is undefined. This applies
* atm to the late/early system suspend/resume handlers.
*/
- if (pm_runtime_get_if_active(rpm->kdev, ignore_usecount) <= 0)
+ if (__pm_runtime_get_conditional(rpm->kdev, ignore_usecount) <= 0)
return 0;
}
diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 12e6f0a26fc8..45701a18c845 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -664,7 +664,7 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl)
break;
}
- pm_status = pm_runtime_get_if_active(&client->dev, true);
+ pm_status = pm_runtime_get_if_active(&client->dev);
if (!pm_status)
return 0;
diff --git a/drivers/net/ipa/ipa_smp2p.c b/drivers/net/ipa/ipa_smp2p.c
index 5620dc271fac..cbf3d4761ce3 100644
--- a/drivers/net/ipa/ipa_smp2p.c
+++ b/drivers/net/ipa/ipa_smp2p.c
@@ -92,7 +92,7 @@ static void ipa_smp2p_notify(struct ipa_smp2p *smp2p)
return;
dev = &smp2p->ipa->pdev->dev;
- smp2p->power_on = pm_runtime_get_if_active(dev, true) > 0;
+ smp2p->power_on = pm_runtime_get_if_active(dev) > 0;
/* Signal whether the IPA power is enabled */
mask = BIT(smp2p->enabled_bit);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 59c01d68c6d5..d8d4abbc936f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2439,7 +2439,7 @@ static void pci_pme_list_scan(struct work_struct *work)
* If the device is in a low power state it
* should not be polled either.
*/
- pm_status = pm_runtime_get_if_active(dev, true);
+ pm_status = pm_runtime_get_if_active(dev);
if (!pm_status)
continue;
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 7c9b35448563..13cd526634c1 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -72,7 +72,8 @@ extern int pm_runtime_force_resume(struct device *dev);
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_active(struct device *dev, bool ign_usage_count);
+extern int __pm_runtime_get_conditional(struct device *dev,
+ bool ign_usage_count);
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);
@@ -94,16 +95,33 @@ extern void pm_runtime_release_supplier(struct device_link *link);
extern int devm_pm_runtime_enable(struct device *dev);
+/**
+ * pm_runtime_get_if_active - Bump up runtime PM usage counter if the device is
+ * in active state
+ * @dev: Target device.
+ *
+ * Increment the runtime PM usage counter of @dev if its runtime PM status is
+ * %RPM_ACTIVE, in which case it returns 1. If the device is in a different
+ * state, 0 is returned. -EINVAL is returned if runtime PM is disabled for the
+ * device.
+ */
+static inline int pm_runtime_get_if_active(struct device *dev)
+{
+ return __pm_runtime_get_conditional(dev, true);
+}
+
/**
* pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
* @dev: Target device.
*
* Increment the runtime PM usage counter of @dev if its runtime PM status is
- * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
+ * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
+ * it returns 1. If the device is in a different state or its usage_count is 0,
+ * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device.
*/
static inline int pm_runtime_get_if_in_use(struct device *dev)
{
- return pm_runtime_get_if_active(dev, false);
+ return __pm_runtime_get_conditional(dev, false);
}
/**
@@ -275,8 +293,12 @@ static inline int pm_runtime_get_if_in_use(struct device *dev)
{
return -EINVAL;
}
-static inline int pm_runtime_get_if_active(struct device *dev,
- bool ign_usage_count)
+static inline int pm_runtime_get_if_active(struct device *dev)
+{
+ return -EINVAL;
+}
+static inline int __pm_runtime_get_conditional(struct device *dev,
+ bool ign_usage_count)
{
return -EINVAL;
}
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index bbf7bcdb449a..0a9223c18d77 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -611,7 +611,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_power_up_pm);
int snd_hdac_keep_power_up(struct hdac_device *codec)
{
if (!atomic_inc_not_zero(&codec->in_pm)) {
- int ret = pm_runtime_get_if_active(&codec->dev, true);
+ int ret = pm_runtime_get_if_active(&codec->dev);
if (!ret)
return -1;
if (ret < 0)
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
2023-11-17 11:14 [PATCH v2 0/7] Small Runtime PM API changes Sakari Ailus
2023-11-17 11:14 ` [PATCH v2 1/7] pm: runtime: Simplify pm_runtime_get_if_active() usage Sakari Ailus
@ 2023-11-17 11:14 ` Sakari Ailus
2023-11-18 17:49 ` Laurent Pinchart
2023-11-17 11:14 ` [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0() Sakari Ailus
` (4 subsequent siblings)
6 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-17 11:14 UTC (permalink / raw
To: linux-acpi; +Cc: linux-media, rafael, jacopo.mondi, laurent.pinchart
Add pm_runtime_put_mark_busy_autosusp() helper function for users that
wish to set the last_busy timestamp to current time and put the
usage_count of the device and set the autosuspend timer.
Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to
calling first pm_runtime_mark_last_busy() and then
pm_runtime_put_autosuspend().
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
include/linux/pm_runtime.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 13cd526634c1..4afe7b0b9d7e 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -495,6 +495,23 @@ static inline int pm_runtime_put_autosuspend(struct device *dev)
RPM_GET_PUT | RPM_ASYNC | RPM_AUTO);
}
+/**
+ * pm_runtime_put_mark_busy_autosusp - Update the last access time of a device
+ * and drop device usage counter and queue
+ * autosuspend if 0.
+ * @dev: Target device.
+ *
+ * Update the last access time of @dev using pm_runtime_mark_last_busy(), then
+ * decrement the runtime PM usage counter of @dev and if it turns out to be
+ * equal to 0, queue up a work item for @dev like in pm_request_autosuspend().
+ */
+static inline int pm_runtime_put_mark_busy_autosusp(struct device *dev)
+{
+ pm_runtime_mark_last_busy(dev);
+
+ return pm_runtime_autosuspend(dev);
+}
+
/**
* pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0.
* @dev: Target device.
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0()
2023-11-17 11:14 [PATCH v2 0/7] Small Runtime PM API changes Sakari Ailus
2023-11-17 11:14 ` [PATCH v2 1/7] pm: runtime: Simplify pm_runtime_get_if_active() usage Sakari Ailus
2023-11-17 11:14 ` [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper Sakari Ailus
@ 2023-11-17 11:14 ` Sakari Ailus
2023-11-18 18:50 ` Laurent Pinchart
2023-11-17 11:14 ` [PATCH v2 4/7] media: Documentation: Improve camera sensor runtime PM documentation Sakari Ailus
` (3 subsequent siblings)
6 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-17 11:14 UTC (permalink / raw
To: linux-acpi; +Cc: linux-media, rafael, jacopo.mondi, laurent.pinchart
Document that acpi_dev_state_d0() can be used to tell if the device was
powered on for probe.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
index 7afd16701a02..815bcc8db69f 100644
--- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
+++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
@@ -24,6 +24,14 @@ there's a problem with the device, the driver likely probes just fine but the
first user will find out the device doesn't work, instead of a failure at probe
time. This feature should thus be used sparingly.
+ACPI framework
+--------------
+
+Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
+whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
+returns true if the device is powered on, false otherwise. For non-ACPI backed
+devices it returns true always.
+
I²C
---
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 4/7] media: Documentation: Improve camera sensor runtime PM documentation
2023-11-17 11:14 [PATCH v2 0/7] Small Runtime PM API changes Sakari Ailus
` (2 preceding siblings ...)
2023-11-17 11:14 ` [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0() Sakari Ailus
@ 2023-11-17 11:14 ` Sakari Ailus
2023-11-18 18:49 ` Laurent Pinchart
2023-11-17 11:14 ` [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly Sakari Ailus
` (2 subsequent siblings)
6 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-17 11:14 UTC (permalink / raw
To: linux-acpi; +Cc: linux-media, rafael, jacopo.mondi, laurent.pinchart
Endorse the use of pm_runtime_get_sync() in order to resume the device and
pm_runtime_get_if_active() to increment its usage_count if the device is
in RPM_ACTIVE state.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
.../driver-api/media/camera-sensor.rst | 76 +++++++++++++------
.../firmware-guide/acpi/non-d0-probe.rst | 2 +
2 files changed, 53 insertions(+), 25 deletions(-)
diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
index 6456145f96ed..c1bb0de40189 100644
--- a/Documentation/driver-api/media/camera-sensor.rst
+++ b/Documentation/driver-api/media/camera-sensor.rst
@@ -58,35 +58,55 @@ pipeline. They must obey the rules listed herein to ensure coherent power
management over the pipeline.
Camera sensor drivers are responsible for controlling the power state of the
-device they otherwise control as well. They shall use runtime PM to manage
-power states. Runtime PM shall be enabled at probe time and disabled at remove
-time. Drivers should enable runtime PM autosuspend.
+device they otherwise control as well. They shall use runtime PM to manage power
+states. Runtime PM shall be enabled at probe time using
+:c:func:`pm_runtime_enable()` and disabled using :c:func:`pm_runtime_disable()`
+at remove time. Before enabling Runtime PM, the device's Runtime PM status is
+set to ``RPM_ACTIVE`` using :c:func:`pm_runtime_set_active()` and after
+disabling set to ``RPM_SUSPENDED`` using :c:func:`pm_runtime_set_suspended()`.
+Drivers should enable runtime PM autosuspend.
The runtime PM handlers shall handle clocks, regulators, GPIOs, and other
system resources required to power the sensor up and down. For drivers that
don't use any of those resources (such as drivers that support ACPI systems
only), the runtime PM handlers may be left unimplemented.
-In general, the device shall be powered on at least when its registers are
-being accessed and when it is streaming. Drivers should use
-``pm_runtime_resume_and_get()`` when starting streaming and
-``pm_runtime_put()`` or ``pm_runtime_put_autosuspend()`` when stopping
-streaming. They may power the device up at probe time (for example to read
+In general, the device shall be powered on at least when its registers are being
+accessed and when it is streaming. Drivers using autosuspend should use
+:c:func:`pm_runtime_get_sync()` to ensure the device is powered on. The
+function increments Runtime PM ``usage_count`` which the driver is responsible
+for decrementing using e.g. :c:func:`pm_runtime_put_mark_busy_autosusp()`, which
+starts autosuspend timer to power off the device later on when its
+``usage_count`` is 0, or :c:func:`pm_runtime_put()` which proceeds to power off
+the device without a delay when its ``usage_count`` is 0.
+
+Note that runtime PM ``usage_count`` of the device must be put even if
+:c:func:`pm_runtime_get_sync()` fails. :c:func:`pm_runtime_get_sync()` returns 1
+if the device was already powered on, which may be used as a signal for the
+driver that initialisation related registers need to be written to the
+sensor.
+
+Drivers that support Devicetree shall also power on the device explicitly in
+driver's probe() function and power the device off in driver's remove() function
+without relying on Runtime PM.
+
+Drivers may power the device up at probe time (for example to read
identification registers), but should not keep it powered unconditionally after
-probe.
+probe. On ACPI systems I²C devices are normally powered on for probe but
+:ref:`this can be avoided if needed <firmware_acpi_non_d0_probe>`.
At system suspend time, the whole camera pipeline must stop streaming, and
restart when the system is resumed. This requires coordination between the
camera sensor and the rest of the camera pipeline. Bridge drivers are
responsible for this coordination, and instruct camera sensors to stop and
-restart streaming by calling the appropriate subdev operations
-(``.s_stream()``, ``.enable_streams()`` or ``.disable_streams()``). Camera
-sensor drivers shall therefore **not** keep track of the streaming state to
-stop streaming in the PM suspend handler and restart it in the resume handler.
-Drivers should in general not implement the system PM handlers.
+restart streaming by calling the appropriate subdev operations (``.s_stream()``,
+``.enable_streams()`` or ``.disable_streams()``). Camera sensor drivers shall
+therefore **not** keep track of the streaming state to stop streaming in the PM
+suspend handler and restart it in the resume handler. Drivers should in general
+not implement the system PM handlers.
Camera sensor drivers shall **not** implement the subdev ``.s_power()``
-operation, as it is deprecated. While this operation is implemented in some
+operation as it is deprecated. While this operation is implemented in some
existing drivers as they predate the deprecation, new drivers shall use runtime
PM instead. If you feel you need to begin calling ``.s_power()`` from an ISP or
a bridge driver, instead add runtime PM support to the sensor driver you are
@@ -94,20 +114,26 @@ using and drop its ``.s_power()`` handler.
Please also see :ref:`examples <media-camera-sensor-examples>`.
+.. _media_writing_camera_sensor_drives_control_framework:
+
Control framework
~~~~~~~~~~~~~~~~~
``v4l2_ctrl_handler_setup()`` function may not be used in the device's runtime
-PM ``runtime_resume`` callback, as it has no way to figure out the power state
-of the device. This is because the power state of the device is only changed
-after the power state transition has taken place. The ``s_ctrl`` callback can be
-used to obtain device's power state after the power state transition:
-
-.. c:function:: int pm_runtime_get_if_in_use(struct device *dev);
-
-The function returns a non-zero value if it succeeded getting the power count or
-runtime PM was disabled, in either of which cases the driver may proceed to
-access the device.
+PM ``runtime_resume`` callback as it has no way to figure out the power state of
+the device. This is because the Runtime PM status of the device is only changed
+after the Runtime PM status transition has taken place. The ``s_ctrl`` callback
+can be used to obtain device's Runtime PM status once the transition has taken
+place:
+
+.. c:function:: int pm_runtime_get_if_active(struct device *dev);
+
+The function returns a non-zero value if the device is powered on (in which case
+it increments the device's ``usage_count``) or runtime PM was disabled, in
+either of which cases the driver may proceed to access the device. Note that the
+device's ``usage_count`` is not incremented if the function returns an error, in
+which case the ``usage_count`` also must not be put using
+:c:func:`pm_runtime_put()` or its variants.
Rotation, orientation and flipping
----------------------------------
diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
index 815bcc8db69f..f0669059101f 100644
--- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
+++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
@@ -1,5 +1,7 @@
.. SPDX-License-Identifier: GPL-2.0
+.. _firmware_acpi_non_d0_probe:
+
========================================
Probing devices in other D states than 0
========================================
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly
2023-11-17 11:14 [PATCH v2 0/7] Small Runtime PM API changes Sakari Ailus
` (3 preceding siblings ...)
2023-11-17 11:14 ` [PATCH v2 4/7] media: Documentation: Improve camera sensor runtime PM documentation Sakari Ailus
@ 2023-11-17 11:14 ` Sakari Ailus
2023-11-17 15:30 ` Jacopo Mondi
2023-11-18 8:42 ` kernel test robot
2023-11-17 11:14 ` [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback Sakari Ailus
2023-11-17 11:14 ` [PATCH v2 7/7] media: imx219: " Sakari Ailus
6 siblings, 2 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-11-17 11:14 UTC (permalink / raw
To: linux-acpi; +Cc: linux-media, rafael, jacopo.mondi, laurent.pinchart
Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
and set controls, then use runtime PM autosuspend once the controls have
been set (instead of likely transitioning to suspended state immediately).
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/ov8858.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
index 3af6125a2eee..a99b91700a8d 100644
--- a/drivers/media/i2c/ov8858.c
+++ b/drivers/media/i2c/ov8858.c
@@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
struct v4l2_subdev_state *state;
u16 digi_gain;
s64 max_exp;
- int ret;
+ int ret, pm_status;
/*
* The control handler and the subdev state use the same mutex and the
@@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
break;
}
- if (!pm_runtime_get_if_in_use(&client->dev))
+ pm_status = pm_runtime_get_if_active(&client->dev);
+ if (!pm_status)
return 0;
switch (ctrl->id) {
@@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
break;
}
- pm_runtime_put(&client->dev);
+ if (pm_status > 0)
+ pm_runtime_mark_busy_autosusp(&client->dev);
return ret;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback
2023-11-17 11:14 [PATCH v2 0/7] Small Runtime PM API changes Sakari Ailus
` (4 preceding siblings ...)
2023-11-17 11:14 ` [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly Sakari Ailus
@ 2023-11-17 11:14 ` Sakari Ailus
2023-11-18 18:52 ` Laurent Pinchart
2023-11-17 11:14 ` [PATCH v2 7/7] media: imx219: " Sakari Ailus
6 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-17 11:14 UTC (permalink / raw
To: linux-acpi; +Cc: linux-media, rafael, jacopo.mondi, laurent.pinchart
pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for
the device, in which case it won't increment the use count.
pm_runtime_put() does that unconditionally however. Only call
pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value >
0.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/imx319.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 5378f607f340..e7b2d0c20d29 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -1880,8 +1880,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
struct imx319 *imx319 = container_of(ctrl->handler,
struct imx319, ctrl_handler);
struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
+ int ret, pm_status;
s64 max;
- int ret;
/* Propagate change of current control to all related controls */
switch (ctrl->id) {
@@ -1898,7 +1898,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
* Applying V4L2 control value only happens
* when power is up for streaming
*/
- if (!pm_runtime_get_if_in_use(&client->dev))
+ pm_status = pm_runtime_get_if_in_use(&client->dev);
+ if (!pm_status)
return 0;
switch (ctrl->id) {
@@ -1937,7 +1938,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
break;
}
- pm_runtime_put(&client->dev);
+ if (pm_status > 0)
+ pm_runtime_put(&client->dev);
return ret;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 7/7] media: imx219: Put usage_count correctly in s_ctrl callback
2023-11-17 11:14 [PATCH v2 0/7] Small Runtime PM API changes Sakari Ailus
` (5 preceding siblings ...)
2023-11-17 11:14 ` [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback Sakari Ailus
@ 2023-11-17 11:14 ` Sakari Ailus
2023-11-17 14:20 ` Dave Stevenson
6 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-17 11:14 UTC (permalink / raw
To: linux-acpi; +Cc: linux-media, rafael, jacopo.mondi, laurent.pinchart
pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for
the device, in which case it won't increment the use count.
pm_runtime_put() does that unconditionally however. Only call
pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value >
0.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/imx219.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 8436880dcf7a..9865d0b41244 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -371,7 +371,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
const struct v4l2_mbus_framefmt *format;
struct v4l2_subdev_state *state;
- int ret = 0;
+ int ret = 0, pm_status;
state = v4l2_subdev_get_locked_active_state(&imx219->sd);
format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
@@ -393,7 +393,8 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
* Applying V4L2 control value only happens
* when power is up for streaming
*/
- if (pm_runtime_get_if_in_use(&client->dev) == 0)
+ pm_status = pm_runtime_get_if_in_use(&client->dev);
+ if (!pm_status)
return 0;
switch (ctrl->id) {
@@ -446,7 +447,8 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
break;
}
- pm_runtime_put(&client->dev);
+ if (pm_status > 0)
+ pm_runtime_put(&client->dev);
return ret;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 7/7] media: imx219: Put usage_count correctly in s_ctrl callback
2023-11-17 11:14 ` [PATCH v2 7/7] media: imx219: " Sakari Ailus
@ 2023-11-17 14:20 ` Dave Stevenson
0 siblings, 0 replies; 36+ messages in thread
From: Dave Stevenson @ 2023-11-17 14:20 UTC (permalink / raw
To: Sakari Ailus
Cc: linux-acpi, linux-media, rafael, jacopo.mondi, laurent.pinchart
On Fri, 17 Nov 2023 at 11:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for
> the device, in which case it won't increment the use count.
> pm_runtime_put() does that unconditionally however. Only call
> pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value >
> 0.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
> drivers/media/i2c/imx219.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 8436880dcf7a..9865d0b41244 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -371,7 +371,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> const struct v4l2_mbus_framefmt *format;
> struct v4l2_subdev_state *state;
> - int ret = 0;
> + int ret = 0, pm_status;
>
> state = v4l2_subdev_get_locked_active_state(&imx219->sd);
> format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
> @@ -393,7 +393,8 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> * Applying V4L2 control value only happens
> * when power is up for streaming
> */
> - if (pm_runtime_get_if_in_use(&client->dev) == 0)
> + pm_status = pm_runtime_get_if_in_use(&client->dev);
> + if (!pm_status)
> return 0;
>
> switch (ctrl->id) {
> @@ -446,7 +447,8 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> break;
> }
>
> - pm_runtime_put(&client->dev);
> + if (pm_status > 0)
> + pm_runtime_put(&client->dev);
>
> return ret;
> }
> --
> 2.39.2
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly
2023-11-17 11:14 ` [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly Sakari Ailus
@ 2023-11-17 15:30 ` Jacopo Mondi
2023-11-18 11:12 ` Sakari Ailus
2023-11-18 8:42 ` kernel test robot
1 sibling, 1 reply; 36+ messages in thread
From: Jacopo Mondi @ 2023-11-17 15:30 UTC (permalink / raw
To: Sakari Ailus
Cc: linux-acpi, linux-media, rafael, jacopo.mondi, laurent.pinchart
Hi Sakari
On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> and set controls, then use runtime PM autosuspend once the controls have
> been set (instead of likely transitioning to suspended state immediately).
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/i2c/ov8858.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> index 3af6125a2eee..a99b91700a8d 100644
> --- a/drivers/media/i2c/ov8858.c
> +++ b/drivers/media/i2c/ov8858.c
> @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> struct v4l2_subdev_state *state;
> u16 digi_gain;
> s64 max_exp;
> - int ret;
> + int ret, pm_status;
>
> /*
> * The control handler and the subdev state use the same mutex and the
> @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> break;
> }
>
> - if (!pm_runtime_get_if_in_use(&client->dev))
> + pm_status = pm_runtime_get_if_active(&client->dev);
> + if (!pm_status)
> return 0;
>
> switch (ctrl->id) {
> @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> break;
> }
>
> - pm_runtime_put(&client->dev);
> + if (pm_status > 0)
I'm not 100% sure I get this bit.
If we get here it means pm_status is either -EINVAL or > 0, otherwise
we would have exited earlier.
What's the point of checking for > 0 here ?
There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
the the below call is a nop, or if pm_runtime has not been enabled by
the driver, which means the driver doesn't use pm_runtime at all.
Are there other cases I have missed that require checking here for
pm_status > 0 ?
> + pm_runtime_mark_busy_autosusp(&client->dev);
>
> return ret;
> }
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly
2023-11-17 11:14 ` [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly Sakari Ailus
2023-11-17 15:30 ` Jacopo Mondi
@ 2023-11-18 8:42 ` kernel test robot
1 sibling, 0 replies; 36+ messages in thread
From: kernel test robot @ 2023-11-18 8:42 UTC (permalink / raw
To: Sakari Ailus; +Cc: llvm, oe-kbuild-all
Hi Sakari,
kernel test robot noticed the following build errors:
[auto build test ERROR on 3e238417254bfdcc23fe207780b59cbb08656762]
url: https://github.com/intel-lab-lkp/linux/commits/Sakari-Ailus/pm-runtime-Simplify-pm_runtime_get_if_active-usage/20231117-191654
base: 3e238417254bfdcc23fe207780b59cbb08656762
patch link: https://lore.kernel.org/r/20231117111433.1561669-6-sakari.ailus%40linux.intel.com
patch subject: [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20231118/202311181620.Oz3Uibrc-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231118/202311181620.Oz3Uibrc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311181620.Oz3Uibrc-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/media/i2c/ov8858.c:1606:3: error: call to undeclared function 'pm_runtime_mark_busy_autosusp'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
pm_runtime_mark_busy_autosusp(&client->dev);
^
drivers/media/i2c/ov8858.c:1606:3: note: did you mean 'pm_runtime_put_mark_busy_autosusp'?
include/linux/pm_runtime.h:508:19: note: 'pm_runtime_put_mark_busy_autosusp' declared here
static inline int pm_runtime_put_mark_busy_autosusp(struct device *dev)
^
1 error generated.
vim +/pm_runtime_mark_busy_autosusp +1606 drivers/media/i2c/ov8858.c
1530
1531 static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
1532 {
1533 struct ov8858 *ov8858 = container_of(ctrl->handler,
1534 struct ov8858, ctrl_handler);
1535
1536 struct i2c_client *client = v4l2_get_subdevdata(&ov8858->subdev);
1537 struct v4l2_mbus_framefmt *format;
1538 struct v4l2_subdev_state *state;
1539 u16 digi_gain;
1540 s64 max_exp;
1541 int ret, pm_status;
1542
1543 /*
1544 * The control handler and the subdev state use the same mutex and the
1545 * mutex is guaranteed to be locked:
1546 * - by the core when s_ctrl is called int the VIDIOC_S_CTRL call path
1547 * - by the driver when s_ctrl is called in the s_stream(1) call path
1548 */
1549 state = v4l2_subdev_get_locked_active_state(&ov8858->subdev);
1550 format = v4l2_subdev_get_pad_format(&ov8858->subdev, state, 0);
1551
1552 /* Propagate change of current control to all related controls */
1553 switch (ctrl->id) {
1554 case V4L2_CID_VBLANK:
1555 /* Update max exposure while meeting expected vblanking */
1556 max_exp = format->height + ctrl->val - OV8858_EXPOSURE_MARGIN;
1557 __v4l2_ctrl_modify_range(ov8858->exposure,
1558 ov8858->exposure->minimum, max_exp,
1559 ov8858->exposure->step,
1560 ov8858->exposure->default_value);
1561 break;
1562 }
1563
1564 pm_status = pm_runtime_get_if_active(&client->dev);
1565 if (!pm_status)
1566 return 0;
1567
1568 switch (ctrl->id) {
1569 case V4L2_CID_EXPOSURE:
1570 /* 4 least significant bits of exposure are fractional part */
1571 ret = ov8858_write(ov8858, OV8858_REG_LONG_EXPO,
1572 ctrl->val << 4, NULL);
1573 break;
1574 case V4L2_CID_ANALOGUE_GAIN:
1575 ret = ov8858_write(ov8858, OV8858_REG_LONG_GAIN,
1576 ctrl->val, NULL);
1577 break;
1578 case V4L2_CID_DIGITAL_GAIN:
1579 /*
1580 * Digital gain is assembled as:
1581 * 0x350a[7:0] = dgain[13:6]
1582 * 0x350b[5:0] = dgain[5:0]
1583 * Reassemble the control value to write it in one go.
1584 */
1585 digi_gain = (ctrl->val & OV8858_LONG_DIGIGAIN_L_MASK)
1586 | ((ctrl->val & OV8858_LONG_DIGIGAIN_H_MASK) <<
1587 OV8858_LONG_DIGIGAIN_H_SHIFT);
1588 ret = ov8858_write(ov8858, OV8858_REG_LONG_DIGIGAIN,
1589 digi_gain, NULL);
1590 break;
1591 case V4L2_CID_VBLANK:
1592 ret = ov8858_write(ov8858, OV8858_REG_VTS,
1593 ctrl->val + format->height, NULL);
1594 break;
1595 case V4L2_CID_TEST_PATTERN:
1596 ret = ov8858_enable_test_pattern(ov8858, ctrl->val);
1597 break;
1598 default:
1599 ret = -EINVAL;
1600 dev_warn(&client->dev, "%s Unhandled id: 0x%x\n",
1601 __func__, ctrl->id);
1602 break;
1603 }
1604
1605 if (pm_status > 0)
> 1606 pm_runtime_mark_busy_autosusp(&client->dev);
1607
1608 return ret;
1609 }
1610
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly
2023-11-17 15:30 ` Jacopo Mondi
@ 2023-11-18 11:12 ` Sakari Ailus
2023-11-18 17:33 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-18 11:12 UTC (permalink / raw
To: Jacopo Mondi; +Cc: linux-acpi, linux-media, rafael, laurent.pinchart
Hi Jacopo,
On Fri, Nov 17, 2023 at 04:30:15PM +0100, Jacopo Mondi wrote:
> Hi Sakari
>
> On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> > Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> > and set controls, then use runtime PM autosuspend once the controls have
> > been set (instead of likely transitioning to suspended state immediately).
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/i2c/ov8858.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> > index 3af6125a2eee..a99b91700a8d 100644
> > --- a/drivers/media/i2c/ov8858.c
> > +++ b/drivers/media/i2c/ov8858.c
> > @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > struct v4l2_subdev_state *state;
> > u16 digi_gain;
> > s64 max_exp;
> > - int ret;
> > + int ret, pm_status;
> >
> > /*
> > * The control handler and the subdev state use the same mutex and the
> > @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > break;
> > }
> >
> > - if (!pm_runtime_get_if_in_use(&client->dev))
> > + pm_status = pm_runtime_get_if_active(&client->dev);
> > + if (!pm_status)
> > return 0;
> >
> > switch (ctrl->id) {
> > @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > break;
> > }
> >
> > - pm_runtime_put(&client->dev);
> > + if (pm_status > 0)
>
> I'm not 100% sure I get this bit.
>
> If we get here it means pm_status is either -EINVAL or > 0, otherwise
> we would have exited earlier.
>
> What's the point of checking for > 0 here ?
>
> There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
> the the below call is a nop, or if pm_runtime has not been enabled by
> the driver, which means the driver doesn't use pm_runtime at all.
>
> Are there other cases I have missed that require checking here for
> pm_status > 0 ?
Other than Runtime PM being disabled, I don't think that should happen.
So as such this patch does not fix a bug. I just prefer to be extra
cautious when it comes to use counts.
>
> > + pm_runtime_mark_busy_autosusp(&client->dev);
> >
> > return ret;
> > }
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly
2023-11-18 11:12 ` Sakari Ailus
@ 2023-11-18 17:33 ` Laurent Pinchart
2023-11-20 8:31 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-18 17:33 UTC (permalink / raw
To: Sakari Ailus; +Cc: Jacopo Mondi, linux-acpi, linux-media, rafael
Hi Sakari
On Sat, Nov 18, 2023 at 11:12:41AM +0000, Sakari Ailus wrote:
> On Fri, Nov 17, 2023 at 04:30:15PM +0100, Jacopo Mondi wrote:
> > On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> > > Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> > > and set controls, then use runtime PM autosuspend once the controls have
> > > been set (instead of likely transitioning to suspended state immediately).
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/i2c/ov8858.c | 8 +++++---
> > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> > > index 3af6125a2eee..a99b91700a8d 100644
> > > --- a/drivers/media/i2c/ov8858.c
> > > +++ b/drivers/media/i2c/ov8858.c
> > > @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > struct v4l2_subdev_state *state;
> > > u16 digi_gain;
> > > s64 max_exp;
> > > - int ret;
> > > + int ret, pm_status;
> > >
> > > /*
> > > * The control handler and the subdev state use the same mutex and the
> > > @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > break;
> > > }
> > >
> > > - if (!pm_runtime_get_if_in_use(&client->dev))
> > > + pm_status = pm_runtime_get_if_active(&client->dev);
> > > + if (!pm_status)
> > > return 0;
> > >
> > > switch (ctrl->id) {
> > > @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > break;
> > > }
> > >
> > > - pm_runtime_put(&client->dev);
> > > + if (pm_status > 0)
> >
> > I'm not 100% sure I get this bit.
> >
> > If we get here it means pm_status is either -EINVAL or > 0, otherwise
> > we would have exited earlier.
> >
> > What's the point of checking for > 0 here ?
> >
> > There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
> > the the below call is a nop, or if pm_runtime has not been enabled by
> > the driver, which means the driver doesn't use pm_runtime at all.
> >
> > Are there other cases I have missed that require checking here for
> > pm_status > 0 ?
>
> Other than Runtime PM being disabled, I don't think that should happen.
>
> So as such this patch does not fix a bug. I just prefer to be extra
> cautious when it comes to use counts.
What happened to the old motto of "if it's not broken, don't fix it" ?
:-) I like how this series (slightly) simplifies the runtime PM API by
giving pm_runtime_get_if_active() the right behaviour for the most
common use cases. Let's continue in that direction, and evolve the API
to simplify driver, not render them more complex.
I would prefer refactoring this series to first switch drivers to
pm_runtime_get_if_active(), and then use autosuspend at the end of the
.s_ctrl() handler. That can be two patches, each modifying all relevant
sensor driver.
> > > + pm_runtime_mark_busy_autosusp(&client->dev);
> > >
> > > return ret;
> > > }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/7] pm: runtime: Simplify pm_runtime_get_if_active() usage
2023-11-17 11:14 ` [PATCH v2 1/7] pm: runtime: Simplify pm_runtime_get_if_active() usage Sakari Ailus
@ 2023-11-18 17:46 ` Laurent Pinchart
0 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-18 17:46 UTC (permalink / raw
To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
Hi Sakari,
Thank you for the patch.
On Fri, Nov 17, 2023 at 01:14:27PM +0200, Sakari Ailus wrote:
> The majority of the callers currently using pm_runtime_get_if_active()
> call it with ign_usage_count argument set to true. There's only one driver
> using this feature (and natually implementation of
> pm_runtime_get_if_in_use()).
>
> To make this function more practical to use, remove the ign_usage_count
> argument from the function. The main implementation is renamed as
> __pm_runtime_get_conditional().
>
> This function is expected to gain a large number of users in the future
> --- camera sensor drivers using runtime autosuspend have a need to get the
s/---/-/
> device's usage_count conditionally if it is enabled. Most of these
> currently use pm_runtime_get_if_in_use(), which is a bug.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> ---
>
> since v1:
>
> - Fix __pm_runtime_get_if_conditional() un-CONFIG_PM implementation.
> ---
> Documentation/power/runtime_pm.rst | 5 ++--
> drivers/base/power/runtime.c | 9 ++++---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
> drivers/media/i2c/ccs/ccs-core.c | 2 +-
> drivers/net/ipa/ipa_smp2p.c | 2 +-
> drivers/pci/pci.c | 2 +-
> include/linux/pm_runtime.h | 32 +++++++++++++++++++++----
> sound/hda/hdac_device.c | 2 +-
> 8 files changed, 40 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> index 65b86e487afe..da99379071a4 100644
> --- a/Documentation/power/runtime_pm.rst
> +++ b/Documentation/power/runtime_pm.rst
> @@ -396,10 +396,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
> nonzero, increment the counter and return 1; otherwise return 0 without
> changing the counter
>
> - `int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count);`
> + `int pm_runtime_get_if_active(struct device *dev);`
> - return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
> - runtime PM status is RPM_ACTIVE, and either ign_usage_count is true
> - or the device's usage_count is non-zero, increment the counter and
> + runtime PM status is RPM_ACTIVE, increment the counter and
> return 1; otherwise return 0 without changing the counter
>
> `void pm_runtime_put_noidle(struct device *dev);`
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 4545669cb973..8b56468eca9d 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1175,7 +1175,7 @@ int __pm_runtime_resume(struct device *dev, int rpmflags)
> EXPORT_SYMBOL_GPL(__pm_runtime_resume);
>
> /**
> - * pm_runtime_get_if_active - Conditionally bump up device usage counter.
> + * __pm_runtime_get_conditional - Conditionally bump up device usage counter.
> * @dev: Device to handle.
> * @ign_usage_count: Whether or not to look at the current usage counter value.
> *
> @@ -1195,8 +1195,11 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume);
> *
> * The caller is responsible for decrementing the runtime PM usage counter of
> * @dev after this function has returned a positive value for it.
> + *
> + * This function is not intended to be called by drivers, use
> + * pm_runtime_get_if_active() or pm_runtime_get_if_in_use() instead.
> */
> -int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count)
> +int __pm_runtime_get_conditional(struct device *dev, bool ign_usage_count)
> {
> unsigned long flags;
> int retval;
> @@ -1217,7 +1220,7 @@ int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count)
>
> return retval;
> }
> -EXPORT_SYMBOL_GPL(pm_runtime_get_if_active);
> +EXPORT_SYMBOL_GPL(__pm_runtime_get_conditional);
>
> /**
> * __pm_runtime_set_status - Set runtime PM status of a device.
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6d8e5e5c0cba..b163efe80975 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -434,7 +434,7 @@ static intel_wakeref_t __intel_runtime_pm_get_if_active(struct intel_runtime_pm
> * function, since the power state is undefined. This applies
> * atm to the late/early system suspend/resume handlers.
> */
> - if (pm_runtime_get_if_active(rpm->kdev, ignore_usecount) <= 0)
> + if (__pm_runtime_get_conditional(rpm->kdev, ignore_usecount) <= 0)
> return 0;
> }
>
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index 12e6f0a26fc8..45701a18c845 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -664,7 +664,7 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl)
> break;
> }
>
> - pm_status = pm_runtime_get_if_active(&client->dev, true);
> + pm_status = pm_runtime_get_if_active(&client->dev);
> if (!pm_status)
> return 0;
>
> diff --git a/drivers/net/ipa/ipa_smp2p.c b/drivers/net/ipa/ipa_smp2p.c
> index 5620dc271fac..cbf3d4761ce3 100644
> --- a/drivers/net/ipa/ipa_smp2p.c
> +++ b/drivers/net/ipa/ipa_smp2p.c
> @@ -92,7 +92,7 @@ static void ipa_smp2p_notify(struct ipa_smp2p *smp2p)
> return;
>
> dev = &smp2p->ipa->pdev->dev;
> - smp2p->power_on = pm_runtime_get_if_active(dev, true) > 0;
> + smp2p->power_on = pm_runtime_get_if_active(dev) > 0;
>
> /* Signal whether the IPA power is enabled */
> mask = BIT(smp2p->enabled_bit);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 59c01d68c6d5..d8d4abbc936f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2439,7 +2439,7 @@ static void pci_pme_list_scan(struct work_struct *work)
> * If the device is in a low power state it
> * should not be polled either.
> */
> - pm_status = pm_runtime_get_if_active(dev, true);
> + pm_status = pm_runtime_get_if_active(dev);
> if (!pm_status)
> continue;
>
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 7c9b35448563..13cd526634c1 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -72,7 +72,8 @@ extern int pm_runtime_force_resume(struct device *dev);
> 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_active(struct device *dev, bool ign_usage_count);
> +extern int __pm_runtime_get_conditional(struct device *dev,
> + bool ign_usage_count);
> 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);
> @@ -94,16 +95,33 @@ extern void pm_runtime_release_supplier(struct device_link *link);
>
> extern int devm_pm_runtime_enable(struct device *dev);
>
> +/**
> + * pm_runtime_get_if_active - Bump up runtime PM usage counter if the device is
> + * in active state
> + * @dev: Target device.
> + *
> + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> + * %RPM_ACTIVE, in which case it returns 1. If the device is in a different
> + * state, 0 is returned. -EINVAL is returned if runtime PM is disabled for the
> + * device.
> + */
> +static inline int pm_runtime_get_if_active(struct device *dev)
> +{
> + return __pm_runtime_get_conditional(dev, true);
> +}
> +
> /**
> * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> * @dev: Target device.
> *
> * Increment the runtime PM usage counter of @dev if its runtime PM status is
> - * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
> + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
> + * it returns 1. If the device is in a different state or its usage_count is 0,
> + * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device.
It would be nice to explain the difference between RPM_ACTIVE and
usage_count. It can be done in a separate patch.
> */
> static inline int pm_runtime_get_if_in_use(struct device *dev)
> {
> - return pm_runtime_get_if_active(dev, false);
> + return __pm_runtime_get_conditional(dev, false);
> }
>
> /**
> @@ -275,8 +293,12 @@ static inline int pm_runtime_get_if_in_use(struct device *dev)
> {
> return -EINVAL;
> }
> -static inline int pm_runtime_get_if_active(struct device *dev,
> - bool ign_usage_count)
> +static inline int pm_runtime_get_if_active(struct device *dev)
> +{
> + return -EINVAL;
> +}
The pm_runtime_get_if_in_use() and pm_runtime_get_if_active() wrappers
could be defined once, outside of the CONFIG_PM conditional section,
nistead of having a stub version when !CONFIG_PM. Lowering the amount of
conditional code helps minimizing the risk of build errors in exotic
configurations. This can also be done in a separate patch.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +static inline int __pm_runtime_get_conditional(struct device *dev,
> + bool ign_usage_count)
> {
> return -EINVAL;
> }
> diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
> index bbf7bcdb449a..0a9223c18d77 100644
> --- a/sound/hda/hdac_device.c
> +++ b/sound/hda/hdac_device.c
> @@ -611,7 +611,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_power_up_pm);
> int snd_hdac_keep_power_up(struct hdac_device *codec)
> {
> if (!atomic_inc_not_zero(&codec->in_pm)) {
> - int ret = pm_runtime_get_if_active(&codec->dev, true);
> + int ret = pm_runtime_get_if_active(&codec->dev);
> if (!ret)
> return -1;
> if (ret < 0)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
2023-11-17 11:14 ` [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper Sakari Ailus
@ 2023-11-18 17:49 ` Laurent Pinchart
2023-11-18 21:20 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-18 17:49 UTC (permalink / raw
To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
Hi Sakari,
Thank you for the patch.
On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote:
> Add pm_runtime_put_mark_busy_autosusp() helper function for users that
> wish to set the last_busy timestamp to current time and put the
> usage_count of the device and set the autosuspend timer.
>
> Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to
> calling first pm_runtime_mark_last_busy() and then
> pm_runtime_put_autosuspend().
The vast majority if the pm_runtime_put_autosuspend() users call
pm_runtime_mark_last_busy() right before. Let's make the
pm_runtime_put_autosuspend() function do that by default, and add a
__pm_runtime_put_autosuspend() (name to be bikshedded) for the minority
of cases where updating the last busy timestamp isn't desired. We want
to simplify the API, not make it more complex.
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> include/linux/pm_runtime.h | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 13cd526634c1..4afe7b0b9d7e 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -495,6 +495,23 @@ static inline int pm_runtime_put_autosuspend(struct device *dev)
> RPM_GET_PUT | RPM_ASYNC | RPM_AUTO);
> }
>
> +/**
> + * pm_runtime_put_mark_busy_autosusp - Update the last access time of a device
> + * and drop device usage counter and queue
> + * autosuspend if 0.
> + * @dev: Target device.
> + *
> + * Update the last access time of @dev using pm_runtime_mark_last_busy(), then
> + * decrement the runtime PM usage counter of @dev and if it turns out to be
> + * equal to 0, queue up a work item for @dev like in pm_request_autosuspend().
> + */
> +static inline int pm_runtime_put_mark_busy_autosusp(struct device *dev)
> +{
> + pm_runtime_mark_last_busy(dev);
> +
> + return pm_runtime_autosuspend(dev);
> +}
> +
> /**
> * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0.
> * @dev: Target device.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/7] media: Documentation: Improve camera sensor runtime PM documentation
2023-11-17 11:14 ` [PATCH v2 4/7] media: Documentation: Improve camera sensor runtime PM documentation Sakari Ailus
@ 2023-11-18 18:49 ` Laurent Pinchart
0 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-18 18:49 UTC (permalink / raw
To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
Hi Sakari,
Thank you for the patch.
On Fri, Nov 17, 2023 at 01:14:30PM +0200, Sakari Ailus wrote:
> Endorse the use of pm_runtime_get_sync() in order to resume the device and
Drivers should use pm_runtime_resume_and_get(), not
pm_runtime_get_sync().
> pm_runtime_get_if_active() to increment its usage_count if the device is
> in RPM_ACTIVE state.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> .../driver-api/media/camera-sensor.rst | 76 +++++++++++++------
> .../firmware-guide/acpi/non-d0-probe.rst | 2 +
> 2 files changed, 53 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> index 6456145f96ed..c1bb0de40189 100644
> --- a/Documentation/driver-api/media/camera-sensor.rst
> +++ b/Documentation/driver-api/media/camera-sensor.rst
> @@ -58,35 +58,55 @@ pipeline. They must obey the rules listed herein to ensure coherent power
> management over the pipeline.
>
> Camera sensor drivers are responsible for controlling the power state of the
> -device they otherwise control as well. They shall use runtime PM to manage
> -power states. Runtime PM shall be enabled at probe time and disabled at remove
> -time. Drivers should enable runtime PM autosuspend.
> +device they otherwise control as well. They shall use runtime PM to manage power
> +states. Runtime PM shall be enabled at probe time using
> +:c:func:`pm_runtime_enable()` and disabled using :c:func:`pm_runtime_disable()`
> +at remove time. Before enabling Runtime PM, the device's Runtime PM status is
> +set to ``RPM_ACTIVE`` using :c:func:`pm_runtime_set_active()` and after
> +disabling set to ``RPM_SUSPENDED`` using :c:func:`pm_runtime_set_suspended()`.
That's correct only if the driver powers the device up manually, which
isn't documented here. I recommend adding code examples, and explaining
why this particular initialization sequence is preferred.
> +Drivers should enable runtime PM autosuspend.
A rationale would also be useful.
>
> The runtime PM handlers shall handle clocks, regulators, GPIOs, and other
> system resources required to power the sensor up and down. For drivers that
> don't use any of those resources (such as drivers that support ACPI systems
> only), the runtime PM handlers may be left unimplemented.
Unrelated to this patch, but given how many times I've been told to make
drivers developed for OF-based systems ready for ACPI in case anyone
would ever need that, I think we should require DT support for new
sensor drivers even if they're developed on ACPI systems. It's just
unfair otherwise.
>
> -In general, the device shall be powered on at least when its registers are
> -being accessed and when it is streaming. Drivers should use
> -``pm_runtime_resume_and_get()`` when starting streaming and
> -``pm_runtime_put()`` or ``pm_runtime_put_autosuspend()`` when stopping
> -streaming. They may power the device up at probe time (for example to read
> +In general, the device shall be powered on at least when its registers are being
> +accessed and when it is streaming. Drivers using autosuspend should use
> +:c:func:`pm_runtime_get_sync()` to ensure the device is powered on.
This is applicable to all drivers, not just drivers using autosuspend,
and the correct function is pm_runtime_resume_and_get().
> The
> +function increments Runtime PM ``usage_count`` which the driver is responsible
> +for decrementing using e.g. :c:func:`pm_runtime_put_mark_busy_autosusp()`, which
> +starts autosuspend timer to power off the device later on when its
> +``usage_count`` is 0, or :c:func:`pm_runtime_put()` which proceeds to power off
> +the device without a delay when its ``usage_count`` is 0.
> +
> +Note that runtime PM ``usage_count`` of the device must be put even if
> +:c:func:`pm_runtime_get_sync()` fails.
Let's not go back there. If pm_runtime_resume_and_get() doesn't provide
the right return value, fix it. I do not want to see a call to any RPM
put function in case the get function fails. That was too error prone,
and that hasn't changed. Fix the RPM API instead. I know it's hard given
the number of users, but that's not an excuse. If it requires the help
of coccinelle, and touching hundreds of drivers in a large tree-wide
patch, so be it. I'm getting tired of the pain inflicted on hundreds, of
not thousands, of driver developers because the RPM API has been badly
designed. This is a hard blocker for me.
> :c:func:`pm_runtime_get_sync()` returns 1
> +if the device was already powered on, which may be used as a signal for the
> +driver that initialisation related registers need to be written to the
> +sensor.
> +
> +Drivers that support Devicetree shall also power on the device explicitly in
> +driver's probe() function and power the device off in driver's remove() function
> +without relying on Runtime PM.
This belongs to the probe/remove section above, with a clearer
explanation.
> +
> +Drivers may power the device up at probe time (for example to read
> identification registers), but should not keep it powered unconditionally after
> -probe.
> +probe. On ACPI systems I²C devices are normally powered on for probe but
> +:ref:`this can be avoided if needed <firmware_acpi_non_d0_probe>`.
From a driver point of view, I think you should say that some firmware
implementations request the device not to be powered up at probe time,
and explain what the driver needs to do to handle that properly.
>
> At system suspend time, the whole camera pipeline must stop streaming, and
> restart when the system is resumed. This requires coordination between the
> camera sensor and the rest of the camera pipeline. Bridge drivers are
> responsible for this coordination, and instruct camera sensors to stop and
> -restart streaming by calling the appropriate subdev operations
> -(``.s_stream()``, ``.enable_streams()`` or ``.disable_streams()``). Camera
> -sensor drivers shall therefore **not** keep track of the streaming state to
> -stop streaming in the PM suspend handler and restart it in the resume handler.
> -Drivers should in general not implement the system PM handlers.
> +restart streaming by calling the appropriate subdev operations (``.s_stream()``,
> +``.enable_streams()`` or ``.disable_streams()``). Camera sensor drivers shall
> +therefore **not** keep track of the streaming state to stop streaming in the PM
> +suspend handler and restart it in the resume handler. Drivers should in general
> +not implement the system PM handlers.
Reflowing text without making any other change makes review more
difficult, you may want to split that to another patch (or drop it).
>
> Camera sensor drivers shall **not** implement the subdev ``.s_power()``
> -operation, as it is deprecated. While this operation is implemented in some
> +operation as it is deprecated. While this operation is implemented in some
I don't think this change is needed.
> existing drivers as they predate the deprecation, new drivers shall use runtime
> PM instead. If you feel you need to begin calling ``.s_power()`` from an ISP or
> a bridge driver, instead add runtime PM support to the sensor driver you are
> @@ -94,20 +114,26 @@ using and drop its ``.s_power()`` handler.
>
> Please also see :ref:`examples <media-camera-sensor-examples>`.
>
> +.. _media_writing_camera_sensor_drives_control_framework:
> +
> Control framework
> ~~~~~~~~~~~~~~~~~
>
> ``v4l2_ctrl_handler_setup()`` function may not be used in the device's runtime
> -PM ``runtime_resume`` callback, as it has no way to figure out the power state
> -of the device. This is because the power state of the device is only changed
> -after the power state transition has taken place. The ``s_ctrl`` callback can be
> -used to obtain device's power state after the power state transition:
> -
> -.. c:function:: int pm_runtime_get_if_in_use(struct device *dev);
> -
> -The function returns a non-zero value if it succeeded getting the power count or
> -runtime PM was disabled, in either of which cases the driver may proceed to
> -access the device.
> +PM ``runtime_resume`` callback as it has no way to figure out the power state of
> +the device. This is because the Runtime PM status of the device is only changed
> +after the Runtime PM status transition has taken place. The ``s_ctrl`` callback
> +can be used to obtain device's Runtime PM status once the transition has taken
s/device/the device/
> +place:
> +
> +.. c:function:: int pm_runtime_get_if_active(struct device *dev);
> +
> +The function returns a non-zero value if the device is powered on (in which case
> +it increments the device's ``usage_count``) or runtime PM was disabled, in
> +either of which cases the driver may proceed to access the device. Note that the
> +device's ``usage_count`` is not incremented if the function returns an error, in
> +which case the ``usage_count`` also must not be put using
> +:c:func:`pm_runtime_put()` or its variants.
As commented separately in another patch that makes a corresponding
change in a sensor driver, I don't like this. Less complexity in drivers
please, not more.
>
> Rotation, orientation and flipping
> ----------------------------------
> diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> index 815bcc8db69f..f0669059101f 100644
> --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> @@ -1,5 +1,7 @@
> .. SPDX-License-Identifier: GPL-2.0
>
> +.. _firmware_acpi_non_d0_probe:
> +
> ========================================
> Probing devices in other D states than 0
> ========================================
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0()
2023-11-17 11:14 ` [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0() Sakari Ailus
@ 2023-11-18 18:50 ` Laurent Pinchart
2023-11-20 9:31 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-18 18:50 UTC (permalink / raw
To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
Hi Sakari,
Thank you for the patch.
On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> Document that acpi_dev_state_d0() can be used to tell if the device was
> powered on for probe.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> index 7afd16701a02..815bcc8db69f 100644
> --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> @@ -24,6 +24,14 @@ there's a problem with the device, the driver likely probes just fine but the
> first user will find out the device doesn't work, instead of a failure at probe
> time. This feature should thus be used sparingly.
>
> +ACPI framework
> +--------------
> +
> +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> +returns true if the device is powered on, false otherwise. For non-ACPI backed
> +devices it returns true always.
> +
While this is true, I don't want to see drivers having to call
ACPI-specific functions, the same way you dislike OF-specific functions
in drivers. Please find a better way to handle this.
> I²C
> ---
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback
2023-11-17 11:14 ` [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback Sakari Ailus
@ 2023-11-18 18:52 ` Laurent Pinchart
2023-11-20 9:32 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-18 18:52 UTC (permalink / raw
To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
Hi Sakari,
Thank you for the patch.
On Fri, Nov 17, 2023 at 01:14:32PM +0200, Sakari Ailus wrote:
> pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for
> the device, in which case it won't increment the use count.
> pm_runtime_put() does that unconditionally however. Only call
> pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value >
> 0.
Why don't you use pm_runtime_get_if_active() ?
Other than that, same comment as for patch 5/7, I don't like the
increased complexity.
These comments apply to 7/7 as well.
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/i2c/imx319.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> index 5378f607f340..e7b2d0c20d29 100644
> --- a/drivers/media/i2c/imx319.c
> +++ b/drivers/media/i2c/imx319.c
> @@ -1880,8 +1880,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> struct imx319 *imx319 = container_of(ctrl->handler,
> struct imx319, ctrl_handler);
> struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
> + int ret, pm_status;
> s64 max;
> - int ret;
>
> /* Propagate change of current control to all related controls */
> switch (ctrl->id) {
> @@ -1898,7 +1898,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> * Applying V4L2 control value only happens
> * when power is up for streaming
> */
> - if (!pm_runtime_get_if_in_use(&client->dev))
> + pm_status = pm_runtime_get_if_in_use(&client->dev);
> + if (!pm_status)
> return 0;
>
> switch (ctrl->id) {
> @@ -1937,7 +1938,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> break;
> }
>
> - pm_runtime_put(&client->dev);
> + if (pm_status > 0)
> + pm_runtime_put(&client->dev);
>
> return ret;
> }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
2023-11-18 17:49 ` Laurent Pinchart
@ 2023-11-18 21:20 ` Rafael J. Wysocki
2023-11-18 21:30 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-11-18 21:20 UTC (permalink / raw
To: Laurent Pinchart
Cc: Sakari Ailus, linux-acpi, linux-media, rafael, jacopo.mondi
On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Sakari,
>
> Thank you for the patch.
>
> On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote:
> > Add pm_runtime_put_mark_busy_autosusp() helper function for users that
> > wish to set the last_busy timestamp to current time and put the
> > usage_count of the device and set the autosuspend timer.
> >
> > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to
> > calling first pm_runtime_mark_last_busy() and then
> > pm_runtime_put_autosuspend().
>
> The vast majority if the pm_runtime_put_autosuspend() users call
> pm_runtime_mark_last_busy() right before. Let's make the
> pm_runtime_put_autosuspend() function do that by default, and add a
> __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority
> of cases where updating the last busy timestamp isn't desired. We want
> to simplify the API, not make it more complex.
I would also prefer it to be done this way if not too problematic.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
2023-11-18 21:20 ` Rafael J. Wysocki
@ 2023-11-18 21:30 ` Laurent Pinchart
2023-11-20 9:27 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-18 21:30 UTC (permalink / raw
To: Rafael J. Wysocki; +Cc: Sakari Ailus, linux-acpi, linux-media, jacopo.mondi
Hi Rafael,
On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote:
> On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote:
> > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote:
> > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that
> > > wish to set the last_busy timestamp to current time and put the
> > > usage_count of the device and set the autosuspend timer.
> > >
> > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to
> > > calling first pm_runtime_mark_last_busy() and then
> > > pm_runtime_put_autosuspend().
> >
> > The vast majority if the pm_runtime_put_autosuspend() users call
> > pm_runtime_mark_last_busy() right before. Let's make the
> > pm_runtime_put_autosuspend() function do that by default, and add a
> > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority
> > of cases where updating the last busy timestamp isn't desired. We want
> > to simplify the API, not make it more complex.
>
> I would also prefer it to be done this way if not too problematic.
I'm glad you agree :-) The change will probably be a bit painful, but I
think it's for the best. Sakari, please let me know if I can help.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly
2023-11-18 17:33 ` Laurent Pinchart
@ 2023-11-20 8:31 ` Sakari Ailus
0 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-11-20 8:31 UTC (permalink / raw
To: Laurent Pinchart; +Cc: Jacopo Mondi, linux-acpi, linux-media, rafael
Hi Laurent,
On Sat, Nov 18, 2023 at 07:33:15PM +0200, Laurent Pinchart wrote:
> Hi Sakari
>
> On Sat, Nov 18, 2023 at 11:12:41AM +0000, Sakari Ailus wrote:
> > On Fri, Nov 17, 2023 at 04:30:15PM +0100, Jacopo Mondi wrote:
> > > On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> > > > Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> > > > and set controls, then use runtime PM autosuspend once the controls have
> > > > been set (instead of likely transitioning to suspended state immediately).
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > > drivers/media/i2c/ov8858.c | 8 +++++---
> > > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> > > > index 3af6125a2eee..a99b91700a8d 100644
> > > > --- a/drivers/media/i2c/ov8858.c
> > > > +++ b/drivers/media/i2c/ov8858.c
> > > > @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > struct v4l2_subdev_state *state;
> > > > u16 digi_gain;
> > > > s64 max_exp;
> > > > - int ret;
> > > > + int ret, pm_status;
> > > >
> > > > /*
> > > > * The control handler and the subdev state use the same mutex and the
> > > > @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > break;
> > > > }
> > > >
> > > > - if (!pm_runtime_get_if_in_use(&client->dev))
> > > > + pm_status = pm_runtime_get_if_active(&client->dev);
> > > > + if (!pm_status)
> > > > return 0;
> > > >
> > > > switch (ctrl->id) {
> > > > @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > break;
> > > > }
> > > >
> > > > - pm_runtime_put(&client->dev);
> > > > + if (pm_status > 0)
> > >
> > > I'm not 100% sure I get this bit.
> > >
> > > If we get here it means pm_status is either -EINVAL or > 0, otherwise
> > > we would have exited earlier.
> > >
> > > What's the point of checking for > 0 here ?
> > >
> > > There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
> > > the the below call is a nop, or if pm_runtime has not been enabled by
> > > the driver, which means the driver doesn't use pm_runtime at all.
> > >
> > > Are there other cases I have missed that require checking here for
> > > pm_status > 0 ?
> >
> > Other than Runtime PM being disabled, I don't think that should happen.
> >
> > So as such this patch does not fix a bug. I just prefer to be extra
> > cautious when it comes to use counts.
>
> What happened to the old motto of "if it's not broken, don't fix it" ?
People tend to take patterns used in drivers and this indeed works here,
but only when taken with the rest of the Runtime PM API usage.
> :-) I like how this series (slightly) simplifies the runtime PM API by
> giving pm_runtime_get_if_active() the right behaviour for the most
> common use cases. Let's continue in that direction, and evolve the API
> to simplify driver, not render them more complex.
>
> I would prefer refactoring this series to first switch drivers to
> pm_runtime_get_if_active(), and then use autosuspend at the end of the
> .s_ctrl() handler. That can be two patches, each modifying all relevant
> sensor driver.
I think it should be fine to do both in the same patch indeed.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
2023-11-18 21:30 ` Laurent Pinchart
@ 2023-11-20 9:27 ` Sakari Ailus
2023-11-20 9:47 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-20 9:27 UTC (permalink / raw
To: Laurent Pinchart; +Cc: Rafael J. Wysocki, linux-acpi, linux-media, jacopo.mondi
Hi Laurent,
On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote:
> Hi Rafael,
>
> On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote:
> > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote:
> > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote:
> > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that
> > > > wish to set the last_busy timestamp to current time and put the
> > > > usage_count of the device and set the autosuspend timer.
> > > >
> > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to
> > > > calling first pm_runtime_mark_last_busy() and then
> > > > pm_runtime_put_autosuspend().
> > >
> > > The vast majority if the pm_runtime_put_autosuspend() users call
> > > pm_runtime_mark_last_busy() right before. Let's make the
> > > pm_runtime_put_autosuspend() function do that by default, and add a
> > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority
> > > of cases where updating the last busy timestamp isn't desired. We want
> > > to simplify the API, not make it more complex.
> >
> > I would also prefer it to be done this way if not too problematic.
>
> I'm glad you agree :-) The change will probably be a bit painful, but I
> think it's for the best. Sakari, please let me know if I can help.
I actually do prefer this approach, too.
There about 350 drivers using pm_runtime_autosuspend() currently. Around
150 uses pm_runtime_autosuspend() which is not preceded by
pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330.
I checked some of what's left: most do still call both, but in a way that
evades Coccinelle matching. Some omissions seem to remain.
Given that there are way more users that do also call
pm_runtime_mark_last_busy(), I think I'll try to introduce
__pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend()
documentation change first and then rename the callers that don't use
pm_runtime_mark_last_busy().
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0()
2023-11-18 18:50 ` Laurent Pinchart
@ 2023-11-20 9:31 ` Sakari Ailus
2023-11-20 12:52 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-20 9:31 UTC (permalink / raw
To: Laurent Pinchart; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
Hi Laurent,
On Sat, Nov 18, 2023 at 08:50:49PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> > Document that acpi_dev_state_d0() can be used to tell if the device was
> > powered on for probe.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > index 7afd16701a02..815bcc8db69f 100644
> > --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > @@ -24,6 +24,14 @@ there's a problem with the device, the driver likely probes just fine but the
> > first user will find out the device doesn't work, instead of a failure at probe
> > time. This feature should thus be used sparingly.
> >
> > +ACPI framework
> > +--------------
> > +
> > +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> > +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> > +returns true if the device is powered on, false otherwise. For non-ACPI backed
> > +devices it returns true always.
> > +
>
> While this is true, I don't want to see drivers having to call
> ACPI-specific functions, the same way you dislike OF-specific functions
> in drivers. Please find a better way to handle this.
The functionality is only available on ACPI and the function does the right
thing on non-ACPI platforms. I don't see an issue here.
Feel free to post DT binding patches on suggested device power state during
probe. :-) I think DT would benefit from this as well: the at24 driver is
widely used and suddenly making probe() not talk to the chip (or even power
it up) at all would probably be seen as a regression.
>
> > I²C
> > ---
> >
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback
2023-11-18 18:52 ` Laurent Pinchart
@ 2023-11-20 9:32 ` Sakari Ailus
2023-11-20 9:45 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-20 9:32 UTC (permalink / raw
To: Laurent Pinchart; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
Hi Laurent,
On Sat, Nov 18, 2023 at 08:52:48PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Fri, Nov 17, 2023 at 01:14:32PM +0200, Sakari Ailus wrote:
> > pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for
> > the device, in which case it won't increment the use count.
> > pm_runtime_put() does that unconditionally however. Only call
> > pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value >
> > 0.
>
> Why don't you use pm_runtime_get_if_active() ?
It's only meaningful if the driver uses autosuspend. The imx319 driver does
not.
>
> Other than that, same comment as for patch 5/7, I don't like the
> increased complexity.
>
> These comments apply to 7/7 as well.
>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/i2c/imx319.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> > index 5378f607f340..e7b2d0c20d29 100644
> > --- a/drivers/media/i2c/imx319.c
> > +++ b/drivers/media/i2c/imx319.c
> > @@ -1880,8 +1880,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > struct imx319 *imx319 = container_of(ctrl->handler,
> > struct imx319, ctrl_handler);
> > struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
> > + int ret, pm_status;
> > s64 max;
> > - int ret;
> >
> > /* Propagate change of current control to all related controls */
> > switch (ctrl->id) {
> > @@ -1898,7 +1898,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > * Applying V4L2 control value only happens
> > * when power is up for streaming
> > */
> > - if (!pm_runtime_get_if_in_use(&client->dev))
> > + pm_status = pm_runtime_get_if_in_use(&client->dev);
> > + if (!pm_status)
> > return 0;
> >
> > switch (ctrl->id) {
> > @@ -1937,7 +1938,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > break;
> > }
> >
> > - pm_runtime_put(&client->dev);
> > + if (pm_status > 0)
> > + pm_runtime_put(&client->dev);
> >
> > return ret;
> > }
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback
2023-11-20 9:32 ` Sakari Ailus
@ 2023-11-20 9:45 ` Laurent Pinchart
2023-11-21 8:18 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-20 9:45 UTC (permalink / raw
To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
On Mon, Nov 20, 2023 at 09:32:10AM +0000, Sakari Ailus wrote:
> On Sat, Nov 18, 2023 at 08:52:48PM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 17, 2023 at 01:14:32PM +0200, Sakari Ailus wrote:
> > > pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for
> > > the device, in which case it won't increment the use count.
> > > pm_runtime_put() does that unconditionally however. Only call
> > > pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value >
> > > 0.
> >
> > Why don't you use pm_runtime_get_if_active() ?
>
> It's only meaningful if the driver uses autosuspend. The imx319 driver does
> not.
Does pm_runtime_get_if_active() causes issues with the driver uses
autosuspend ? Standardizing on a single API that covers all the use
cases would increase consistency and make the code base easier to
understand. Beside, the driver should switch to autosuspend :-) Using
the correct RPM calls already is a good thing, if they don't introduce
any issue.
> > Other than that, same comment as for patch 5/7, I don't like the
> > increased complexity.
> >
> > These comments apply to 7/7 as well.
> >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/i2c/imx319.c | 8 +++++---
> > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> > > index 5378f607f340..e7b2d0c20d29 100644
> > > --- a/drivers/media/i2c/imx319.c
> > > +++ b/drivers/media/i2c/imx319.c
> > > @@ -1880,8 +1880,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > struct imx319 *imx319 = container_of(ctrl->handler,
> > > struct imx319, ctrl_handler);
> > > struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
> > > + int ret, pm_status;
> > > s64 max;
> > > - int ret;
> > >
> > > /* Propagate change of current control to all related controls */
> > > switch (ctrl->id) {
> > > @@ -1898,7 +1898,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > * Applying V4L2 control value only happens
> > > * when power is up for streaming
> > > */
> > > - if (!pm_runtime_get_if_in_use(&client->dev))
> > > + pm_status = pm_runtime_get_if_in_use(&client->dev);
> > > + if (!pm_status)
> > > return 0;
> > >
> > > switch (ctrl->id) {
> > > @@ -1937,7 +1938,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > break;
> > > }
> > >
> > > - pm_runtime_put(&client->dev);
> > > + if (pm_status > 0)
> > > + pm_runtime_put(&client->dev);
> > >
> > > return ret;
> > > }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
2023-11-20 9:27 ` Sakari Ailus
@ 2023-11-20 9:47 ` Laurent Pinchart
2023-11-21 8:41 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-20 9:47 UTC (permalink / raw
To: Sakari Ailus; +Cc: Rafael J. Wysocki, linux-acpi, linux-media, jacopo.mondi
Hi Sakari,
On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote:
> On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote:
> > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote:
> > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote:
> > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote:
> > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that
> > > > > wish to set the last_busy timestamp to current time and put the
> > > > > usage_count of the device and set the autosuspend timer.
> > > > >
> > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to
> > > > > calling first pm_runtime_mark_last_busy() and then
> > > > > pm_runtime_put_autosuspend().
> > > >
> > > > The vast majority if the pm_runtime_put_autosuspend() users call
> > > > pm_runtime_mark_last_busy() right before. Let's make the
> > > > pm_runtime_put_autosuspend() function do that by default, and add a
> > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority
> > > > of cases where updating the last busy timestamp isn't desired. We want
> > > > to simplify the API, not make it more complex.
> > >
> > > I would also prefer it to be done this way if not too problematic.
> >
> > I'm glad you agree :-) The change will probably be a bit painful, but I
> > think it's for the best. Sakari, please let me know if I can help.
>
> I actually do prefer this approach, too.
>
> There about 350 drivers using pm_runtime_autosuspend() currently. Around
> 150 uses pm_runtime_autosuspend() which is not preceded by
> pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330.
>
> I checked some of what's left: most do still call both, but in a way that
> evades Coccinelle matching. Some omissions seem to remain.
>
> Given that there are way more users that do also call
> pm_runtime_mark_last_busy(), I think I'll try to introduce
> __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend()
> documentation change first and then rename the callers that don't use
> pm_runtime_mark_last_busy().
And also drop pm_runtime_mark_last_busy() from the drivers that call
pm_runtime_put_autosuspend(), right ?
This sounds good to me. Thank you for working on this. Two RPM API
simplifications in a week, it feels like Christmas is coming :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0()
2023-11-20 9:31 ` Sakari Ailus
@ 2023-11-20 12:52 ` Rafael J. Wysocki
2023-11-20 20:03 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-11-20 12:52 UTC (permalink / raw
To: Sakari Ailus
Cc: Laurent Pinchart, linux-acpi, linux-media, rafael, jacopo.mondi
On Mon, Nov 20, 2023 at 10:31 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Laurent,
>
> On Sat, Nov 18, 2023 at 08:50:49PM +0200, Laurent Pinchart wrote:
> > Hi Sakari,
> >
> > Thank you for the patch.
> >
> > On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> > > Document that acpi_dev_state_d0() can be used to tell if the device was
> > > powered on for probe.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > index 7afd16701a02..815bcc8db69f 100644
> > > --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > @@ -24,6 +24,14 @@ there's a problem with the device, the driver likely probes just fine but the
> > > first user will find out the device doesn't work, instead of a failure at probe
> > > time. This feature should thus be used sparingly.
> > >
> > > +ACPI framework
> > > +--------------
> > > +
> > > +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> > > +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> > > +returns true if the device is powered on, false otherwise. For non-ACPI backed
> > > +devices it returns true always.
> > > +
> >
> > While this is true, I don't want to see drivers having to call
> > ACPI-specific functions, the same way you dislike OF-specific functions
> > in drivers. Please find a better way to handle this.
>
> The functionality is only available on ACPI and the function does the right
> thing on non-ACPI platforms. I don't see an issue here.
The issue would be calling an ACPI-specific function from code that's
otherwise firmware-agnostic, AFAICS.
It would be good to have a more generic way of checking whether or not
a device is operational.
> Feel free to post DT binding patches on suggested device power state during
> probe. :-) I think DT would benefit from this as well: the at24 driver is
> widely used and suddenly making probe() not talk to the chip (or even power
> it up) at all would probably be seen as a regression.
In the DT case it is more complicated, though, at least in general,
because there may be multiple clocks and regulators the device depends
on and you may need to toggle a GPIO line too.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0()
2023-11-20 12:52 ` Rafael J. Wysocki
@ 2023-11-20 20:03 ` Sakari Ailus
2023-11-20 20:22 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-20 20:03 UTC (permalink / raw
To: Rafael J. Wysocki; +Cc: Laurent Pinchart, linux-acpi, linux-media, jacopo.mondi
Hi Rafael,
On Mon, Nov 20, 2023 at 01:52:39PM +0100, Rafael J. Wysocki wrote:
> On Mon, Nov 20, 2023 at 10:31 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Laurent,
> >
> > On Sat, Nov 18, 2023 at 08:50:49PM +0200, Laurent Pinchart wrote:
> > > Hi Sakari,
> > >
> > > Thank you for the patch.
> > >
> > > On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> > > > Document that acpi_dev_state_d0() can be used to tell if the device was
> > > > powered on for probe.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > > Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> > > > 1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > index 7afd16701a02..815bcc8db69f 100644
> > > > --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > @@ -24,6 +24,14 @@ there's a problem with the device, the driver likely probes just fine but the
> > > > first user will find out the device doesn't work, instead of a failure at probe
> > > > time. This feature should thus be used sparingly.
> > > >
> > > > +ACPI framework
> > > > +--------------
> > > > +
> > > > +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> > > > +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> > > > +returns true if the device is powered on, false otherwise. For non-ACPI backed
> > > > +devices it returns true always.
> > > > +
> > >
> > > While this is true, I don't want to see drivers having to call
> > > ACPI-specific functions, the same way you dislike OF-specific functions
> > > in drivers. Please find a better way to handle this.
> >
> > The functionality is only available on ACPI and the function does the right
> > thing on non-ACPI platforms. I don't see an issue here.
>
> The issue would be calling an ACPI-specific function from code that's
> otherwise firmware-agnostic, AFAICS.
>
> It would be good to have a more generic way of checking whether or not
> a device is operational.
In DT case it's up to the driver to do that, so the device is powered off.
>
> > Feel free to post DT binding patches on suggested device power state during
> > probe. :-) I think DT would benefit from this as well: the at24 driver is
> > widely used and suddenly making probe() not talk to the chip (or even power
> > it up) at all would probably be seen as a regression.
>
> In the DT case it is more complicated, though, at least in general,
> because there may be multiple clocks and regulators the device depends
> on and you may need to toggle a GPIO line too.
I don't think it is as what's missing is the desired power state during
probe, i.e. whether or not the device will be powered on. It wasn't there
in ACPI either before it was added.
The problem is slightly lesser on DT though as it's up to the driver
whether or not to power on the device. In the example I gave above,
however, e.g. the at24 driver can't be modified to keep the device powered
off and at the same time expected people would remain content with it. So
this information should come from firmware.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0()
2023-11-20 20:03 ` Sakari Ailus
@ 2023-11-20 20:22 ` Rafael J. Wysocki
2023-11-20 20:53 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-11-20 20:22 UTC (permalink / raw
To: Sakari Ailus
Cc: Rafael J. Wysocki, Laurent Pinchart, linux-acpi, linux-media,
jacopo.mondi
Hi Sakari,
On Mon, Nov 20, 2023 at 9:03 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, Nov 20, 2023 at 01:52:39PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Nov 20, 2023 at 10:31 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Laurent,
> > >
> > > On Sat, Nov 18, 2023 at 08:50:49PM +0200, Laurent Pinchart wrote:
> > > > Hi Sakari,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> > > > > Document that acpi_dev_state_d0() can be used to tell if the device was
> > > > > powered on for probe.
> > > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > > Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> > > > > 1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > index 7afd16701a02..815bcc8db69f 100644
> > > > > --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > @@ -24,6 +24,14 @@ there's a problem with the device, the driver likely probes just fine but the
> > > > > first user will find out the device doesn't work, instead of a failure at probe
> > > > > time. This feature should thus be used sparingly.
> > > > >
> > > > > +ACPI framework
> > > > > +--------------
> > > > > +
> > > > > +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> > > > > +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> > > > > +returns true if the device is powered on, false otherwise. For non-ACPI backed
> > > > > +devices it returns true always.
> > > > > +
> > > >
> > > > While this is true, I don't want to see drivers having to call
> > > > ACPI-specific functions, the same way you dislike OF-specific functions
> > > > in drivers. Please find a better way to handle this.
> > >
> > > The functionality is only available on ACPI and the function does the right
> > > thing on non-ACPI platforms. I don't see an issue here.
> >
> > The issue would be calling an ACPI-specific function from code that's
> > otherwise firmware-agnostic, AFAICS.
> >
> > It would be good to have a more generic way of checking whether or not
> > a device is operational.
>
> In DT case it's up to the driver to do that, so the device is powered off.
Unless the boot loader (or whatever happens to run before the kernel)
turns it on for some reason (whatever that may be).
I guess the original point has been that in the ACPI case the generic
enumeration code may power up the device if not instructed otherwise
by the platform firmware, whereas in the DT case this is entirely up
to the driver, but I'm not sure if this really matters.
I would suggest adding a generic wrapper around acpi_dev_state_d0()
that will just always return true in the DT case, something like
device_is_accessible() or device_is_operational(), that can be invoked
from generic code without any visible ACPI connotations.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0()
2023-11-20 20:22 ` Rafael J. Wysocki
@ 2023-11-20 20:53 ` Sakari Ailus
0 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-11-20 20:53 UTC (permalink / raw
To: Rafael J. Wysocki
Cc: Laurent Pinchart, linux-acpi, linux-media, jacopo.mondi,
devicetree
Hi Rafael,
On Mon, Nov 20, 2023 at 09:22:53PM +0100, Rafael J. Wysocki wrote:
> Hi Sakari,
>
> On Mon, Nov 20, 2023 at 9:03 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, Nov 20, 2023 at 01:52:39PM +0100, Rafael J. Wysocki wrote:
> > > On Mon, Nov 20, 2023 at 10:31 AM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Laurent,
> > > >
> > > > On Sat, Nov 18, 2023 at 08:50:49PM +0200, Laurent Pinchart wrote:
> > > > > Hi Sakari,
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> > > > > > Document that acpi_dev_state_d0() can be used to tell if the device was
> > > > > > powered on for probe.
> > > > > >
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > ---
> > > > > > Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> > > > > > 1 file changed, 8 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > > index 7afd16701a02..815bcc8db69f 100644
> > > > > > --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > > +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > > @@ -24,6 +24,14 @@ there's a problem with the device, the driver likely probes just fine but the
> > > > > > first user will find out the device doesn't work, instead of a failure at probe
> > > > > > time. This feature should thus be used sparingly.
> > > > > >
> > > > > > +ACPI framework
> > > > > > +--------------
> > > > > > +
> > > > > > +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> > > > > > +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> > > > > > +returns true if the device is powered on, false otherwise. For non-ACPI backed
> > > > > > +devices it returns true always.
> > > > > > +
> > > > >
> > > > > While this is true, I don't want to see drivers having to call
> > > > > ACPI-specific functions, the same way you dislike OF-specific functions
> > > > > in drivers. Please find a better way to handle this.
> > > >
> > > > The functionality is only available on ACPI and the function does the right
> > > > thing on non-ACPI platforms. I don't see an issue here.
> > >
> > > The issue would be calling an ACPI-specific function from code that's
> > > otherwise firmware-agnostic, AFAICS.
> > >
> > > It would be good to have a more generic way of checking whether or not
> > > a device is operational.
> >
> > In DT case it's up to the driver to do that, so the device is powered off.
>
> Unless the boot loader (or whatever happens to run before the kernel)
> turns it on for some reason (whatever that may be).
There are probably some exceptions but they should be quite rare.
If the boot loader already powered on the device, then it'd be no use
avoiding accessing it. That doesn't mean the rest of the device shouldn't
be accessed though.
>
> I guess the original point has been that in the ACPI case the generic
> enumeration code may power up the device if not instructed otherwise
> by the platform firmware, whereas in the DT case this is entirely up
> to the driver, but I'm not sure if this really matters.
>
> I would suggest adding a generic wrapper around acpi_dev_state_d0()
> that will just always return true in the DT case, something like
> device_is_accessible() or device_is_operational(), that can be invoked
> from generic code without any visible ACPI connotations.
The DT case may need a different API for that: telling whether the device
should be powered on for probe (by the driver) rather what
acpi_dev_state_d0() does.
And on ACPI we've only needed this for I²C but likely I3C will follow. It
appears to be lacking ACPI support at the moment.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback
2023-11-20 9:45 ` Laurent Pinchart
@ 2023-11-21 8:18 ` Sakari Ailus
2023-11-21 8:25 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-21 8:18 UTC (permalink / raw
To: Laurent Pinchart; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
Hi Laurent,
On Mon, Nov 20, 2023 at 11:45:03AM +0200, Laurent Pinchart wrote:
> On Mon, Nov 20, 2023 at 09:32:10AM +0000, Sakari Ailus wrote:
> > On Sat, Nov 18, 2023 at 08:52:48PM +0200, Laurent Pinchart wrote:
> > > On Fri, Nov 17, 2023 at 01:14:32PM +0200, Sakari Ailus wrote:
> > > > pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for
> > > > the device, in which case it won't increment the use count.
> > > > pm_runtime_put() does that unconditionally however. Only call
> > > > pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value >
> > > > 0.
> > >
> > > Why don't you use pm_runtime_get_if_active() ?
> >
> > It's only meaningful if the driver uses autosuspend. The imx319 driver does
> > not.
>
> Does pm_runtime_get_if_active() causes issues with the driver uses
> autosuspend ? Standardizing on a single API that covers all the use
> cases would increase consistency and make the code base easier to
> understand. Beside, the driver should switch to autosuspend :-) Using
> the correct RPM calls already is a good thing, if they don't introduce
> any issue.
Both are fine but they're there for a different purpose. The driver should
consistently use either usage_count or status based calls (for autosuspend
to make sense).
>
> > > Other than that, same comment as for patch 5/7, I don't like the
> > > increased complexity.
> > >
> > > These comments apply to 7/7 as well.
> > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > > drivers/media/i2c/imx319.c | 8 +++++---
> > > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> > > > index 5378f607f340..e7b2d0c20d29 100644
> > > > --- a/drivers/media/i2c/imx319.c
> > > > +++ b/drivers/media/i2c/imx319.c
> > > > @@ -1880,8 +1880,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > struct imx319 *imx319 = container_of(ctrl->handler,
> > > > struct imx319, ctrl_handler);
> > > > struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
> > > > + int ret, pm_status;
> > > > s64 max;
> > > > - int ret;
> > > >
> > > > /* Propagate change of current control to all related controls */
> > > > switch (ctrl->id) {
> > > > @@ -1898,7 +1898,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > * Applying V4L2 control value only happens
> > > > * when power is up for streaming
> > > > */
> > > > - if (!pm_runtime_get_if_in_use(&client->dev))
> > > > + pm_status = pm_runtime_get_if_in_use(&client->dev);
> > > > + if (!pm_status)
> > > > return 0;
> > > >
> > > > switch (ctrl->id) {
> > > > @@ -1937,7 +1938,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > break;
> > > > }
> > > >
> > > > - pm_runtime_put(&client->dev);
> > > > + if (pm_status > 0)
> > > > + pm_runtime_put(&client->dev);
> > > >
> > > > return ret;
> > > > }
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback
2023-11-21 8:18 ` Sakari Ailus
@ 2023-11-21 8:25 ` Laurent Pinchart
2023-11-21 8:44 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-21 8:25 UTC (permalink / raw
To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
On Tue, Nov 21, 2023 at 08:18:24AM +0000, Sakari Ailus wrote:
> Hi Laurent,
>
> On Mon, Nov 20, 2023 at 11:45:03AM +0200, Laurent Pinchart wrote:
> > On Mon, Nov 20, 2023 at 09:32:10AM +0000, Sakari Ailus wrote:
> > > On Sat, Nov 18, 2023 at 08:52:48PM +0200, Laurent Pinchart wrote:
> > > > On Fri, Nov 17, 2023 at 01:14:32PM +0200, Sakari Ailus wrote:
> > > > > pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for
> > > > > the device, in which case it won't increment the use count.
> > > > > pm_runtime_put() does that unconditionally however. Only call
> > > > > pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value >
> > > > > 0.
> > > >
> > > > Why don't you use pm_runtime_get_if_active() ?
> > >
> > > It's only meaningful if the driver uses autosuspend. The imx319 driver does
> > > not.
> >
> > Does pm_runtime_get_if_active() causes issues with the driver uses
> > autosuspend ? Standardizing on a single API that covers all the use
> > cases would increase consistency and make the code base easier to
> > understand. Beside, the driver should switch to autosuspend :-) Using
> > the correct RPM calls already is a good thing, if they don't introduce
> > any issue.
>
> Both are fine but they're there for a different purpose. The driver should
> consistently use either usage_count or status based calls (for autosuspend
> to make sense).
But what's the drawback of using pm_runtime_get_if_active() in all cases
(for camera sensor drivers) ? The semantics in the .s_ctrl() handler is
"I want to proceed only if the device is powered on", and that's exactly
what pm_runtime_get_if_active() gives us. The semantics of using
pm_runtime_get_if_in_use() is "I want to proceed if someone else is
holding a reference". It happens to give the same practical result as
pm_runtime_get_if_active() when not using autosuspend, but it's still
not the right semantics.
> > > > Other than that, same comment as for patch 5/7, I don't like the
> > > > increased complexity.
> > > >
> > > > These comments apply to 7/7 as well.
> > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > > drivers/media/i2c/imx319.c | 8 +++++---
> > > > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> > > > > index 5378f607f340..e7b2d0c20d29 100644
> > > > > --- a/drivers/media/i2c/imx319.c
> > > > > +++ b/drivers/media/i2c/imx319.c
> > > > > @@ -1880,8 +1880,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > struct imx319 *imx319 = container_of(ctrl->handler,
> > > > > struct imx319, ctrl_handler);
> > > > > struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
> > > > > + int ret, pm_status;
> > > > > s64 max;
> > > > > - int ret;
> > > > >
> > > > > /* Propagate change of current control to all related controls */
> > > > > switch (ctrl->id) {
> > > > > @@ -1898,7 +1898,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > * Applying V4L2 control value only happens
> > > > > * when power is up for streaming
> > > > > */
> > > > > - if (!pm_runtime_get_if_in_use(&client->dev))
> > > > > + pm_status = pm_runtime_get_if_in_use(&client->dev);
> > > > > + if (!pm_status)
> > > > > return 0;
> > > > >
> > > > > switch (ctrl->id) {
> > > > > @@ -1937,7 +1938,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > break;
> > > > > }
> > > > >
> > > > > - pm_runtime_put(&client->dev);
> > > > > + if (pm_status > 0)
> > > > > + pm_runtime_put(&client->dev);
> > > > >
> > > > > return ret;
> > > > > }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
2023-11-20 9:47 ` Laurent Pinchart
@ 2023-11-21 8:41 ` Sakari Ailus
2023-11-21 8:50 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-11-21 8:41 UTC (permalink / raw
To: Laurent Pinchart; +Cc: Rafael J. Wysocki, linux-acpi, linux-media, jacopo.mondi
Hi Laurent,
On Mon, Nov 20, 2023 at 11:47:43AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote:
> > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote:
> > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote:
> > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote:
> > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote:
> > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that
> > > > > > wish to set the last_busy timestamp to current time and put the
> > > > > > usage_count of the device and set the autosuspend timer.
> > > > > >
> > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to
> > > > > > calling first pm_runtime_mark_last_busy() and then
> > > > > > pm_runtime_put_autosuspend().
> > > > >
> > > > > The vast majority if the pm_runtime_put_autosuspend() users call
> > > > > pm_runtime_mark_last_busy() right before. Let's make the
> > > > > pm_runtime_put_autosuspend() function do that by default, and add a
> > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority
> > > > > of cases where updating the last busy timestamp isn't desired. We want
> > > > > to simplify the API, not make it more complex.
> > > >
> > > > I would also prefer it to be done this way if not too problematic.
> > >
> > > I'm glad you agree :-) The change will probably be a bit painful, but I
> > > think it's for the best. Sakari, please let me know if I can help.
> >
> > I actually do prefer this approach, too.
> >
> > There about 350 drivers using pm_runtime_autosuspend() currently. Around
> > 150 uses pm_runtime_autosuspend() which is not preceded by
> > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330.
> >
> > I checked some of what's left: most do still call both, but in a way that
> > evades Coccinelle matching. Some omissions seem to remain.
> >
> > Given that there are way more users that do also call
> > pm_runtime_mark_last_busy(), I think I'll try to introduce
> > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend()
> > documentation change first and then rename the callers that don't use
> > pm_runtime_mark_last_busy().
>
> And also drop pm_runtime_mark_last_busy() from the drivers that call
> pm_runtime_put_autosuspend(), right ?
That should be done but as it doesn't affect the functionality, it can (and
may only) be done later on --- the current users need to be converted to
use the to-be-added __pm_runtime_put_autosuspend() first.
>
> This sounds good to me. Thank you for working on this. Two RPM API
> simplifications in a week, it feels like Christmas is coming :-)
Yes. And it's always the case actually! Only the time that it takes
differs.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback
2023-11-21 8:25 ` Laurent Pinchart
@ 2023-11-21 8:44 ` Sakari Ailus
0 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-11-21 8:44 UTC (permalink / raw
To: Laurent Pinchart; +Cc: linux-acpi, linux-media, rafael, jacopo.mondi
Hi Laurent,
On Tue, Nov 21, 2023 at 10:25:35AM +0200, Laurent Pinchart wrote:
> On Tue, Nov 21, 2023 at 08:18:24AM +0000, Sakari Ailus wrote:
> > Hi Laurent,
> >
> > On Mon, Nov 20, 2023 at 11:45:03AM +0200, Laurent Pinchart wrote:
> > > On Mon, Nov 20, 2023 at 09:32:10AM +0000, Sakari Ailus wrote:
> > > > On Sat, Nov 18, 2023 at 08:52:48PM +0200, Laurent Pinchart wrote:
> > > > > On Fri, Nov 17, 2023 at 01:14:32PM +0200, Sakari Ailus wrote:
> > > > > > pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for
> > > > > > the device, in which case it won't increment the use count.
> > > > > > pm_runtime_put() does that unconditionally however. Only call
> > > > > > pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value >
> > > > > > 0.
> > > > >
> > > > > Why don't you use pm_runtime_get_if_active() ?
> > > >
> > > > It's only meaningful if the driver uses autosuspend. The imx319 driver does
> > > > not.
> > >
> > > Does pm_runtime_get_if_active() causes issues with the driver uses
> > > autosuspend ? Standardizing on a single API that covers all the use
> > > cases would increase consistency and make the code base easier to
> > > understand. Beside, the driver should switch to autosuspend :-) Using
> > > the correct RPM calls already is a good thing, if they don't introduce
> > > any issue.
> >
> > Both are fine but they're there for a different purpose. The driver should
> > consistently use either usage_count or status based calls (for autosuspend
> > to make sense).
>
> But what's the drawback of using pm_runtime_get_if_active() in all cases
> (for camera sensor drivers) ? The semantics in the .s_ctrl() handler is
> "I want to proceed only if the device is powered on", and that's exactly
> what pm_runtime_get_if_active() gives us. The semantics of using
> pm_runtime_get_if_in_use() is "I want to proceed if someone else is
> holding a reference". It happens to give the same practical result as
> pm_runtime_get_if_active() when not using autosuspend, but it's still
> not the right semantics.
Well, using the _active() variant here may introduce an unneeded write
operation so that isn't actually an issue. But using the _if_in_use()
variant in a driver using autosuspend may lead to missing a write --- as it
returns 0 if the usage_count is 0 even if the device is in active state. Oh
well.
I can change the driver to use autosuspend but I think I'll split the set
into two, one doing the Runtime PM API changes and another to address
issues in sensor drivers.
>
> > > > > Other than that, same comment as for patch 5/7, I don't like the
> > > > > increased complexity.
> > > > >
> > > > > These comments apply to 7/7 as well.
> > > > >
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > ---
> > > > > > drivers/media/i2c/imx319.c | 8 +++++---
> > > > > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> > > > > > index 5378f607f340..e7b2d0c20d29 100644
> > > > > > --- a/drivers/media/i2c/imx319.c
> > > > > > +++ b/drivers/media/i2c/imx319.c
> > > > > > @@ -1880,8 +1880,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > struct imx319 *imx319 = container_of(ctrl->handler,
> > > > > > struct imx319, ctrl_handler);
> > > > > > struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
> > > > > > + int ret, pm_status;
> > > > > > s64 max;
> > > > > > - int ret;
> > > > > >
> > > > > > /* Propagate change of current control to all related controls */
> > > > > > switch (ctrl->id) {
> > > > > > @@ -1898,7 +1898,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > * Applying V4L2 control value only happens
> > > > > > * when power is up for streaming
> > > > > > */
> > > > > > - if (!pm_runtime_get_if_in_use(&client->dev))
> > > > > > + pm_status = pm_runtime_get_if_in_use(&client->dev);
> > > > > > + if (!pm_status)
> > > > > > return 0;
> > > > > >
> > > > > > switch (ctrl->id) {
> > > > > > @@ -1937,7 +1938,8 @@ static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > break;
> > > > > > }
> > > > > >
> > > > > > - pm_runtime_put(&client->dev);
> > > > > > + if (pm_status > 0)
> > > > > > + pm_runtime_put(&client->dev);
> > > > > >
> > > > > > return ret;
> > > > > > }
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
2023-11-21 8:41 ` Sakari Ailus
@ 2023-11-21 8:50 ` Laurent Pinchart
2023-11-21 10:00 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2023-11-21 8:50 UTC (permalink / raw
To: Sakari Ailus; +Cc: Rafael J. Wysocki, linux-acpi, linux-media, jacopo.mondi
Hi Sakari,
On Tue, Nov 21, 2023 at 08:41:34AM +0000, Sakari Ailus wrote:
> On Mon, Nov 20, 2023 at 11:47:43AM +0200, Laurent Pinchart wrote:
> > On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote:
> > > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote:
> > > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote:
> > > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote:
> > > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote:
> > > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that
> > > > > > > wish to set the last_busy timestamp to current time and put the
> > > > > > > usage_count of the device and set the autosuspend timer.
> > > > > > >
> > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to
> > > > > > > calling first pm_runtime_mark_last_busy() and then
> > > > > > > pm_runtime_put_autosuspend().
> > > > > >
> > > > > > The vast majority if the pm_runtime_put_autosuspend() users call
> > > > > > pm_runtime_mark_last_busy() right before. Let's make the
> > > > > > pm_runtime_put_autosuspend() function do that by default, and add a
> > > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority
> > > > > > of cases where updating the last busy timestamp isn't desired. We want
> > > > > > to simplify the API, not make it more complex.
> > > > >
> > > > > I would also prefer it to be done this way if not too problematic.
> > > >
> > > > I'm glad you agree :-) The change will probably be a bit painful, but I
> > > > think it's for the best. Sakari, please let me know if I can help.
> > >
> > > I actually do prefer this approach, too.
> > >
> > > There about 350 drivers using pm_runtime_autosuspend() currently. Around
> > > 150 uses pm_runtime_autosuspend() which is not preceded by
> > > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330.
> > >
> > > I checked some of what's left: most do still call both, but in a way that
> > > evades Coccinelle matching. Some omissions seem to remain.
> > >
> > > Given that there are way more users that do also call
> > > pm_runtime_mark_last_busy(), I think I'll try to introduce
> > > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend()
> > > documentation change first and then rename the callers that don't use
> > > pm_runtime_mark_last_busy().
> >
> > And also drop pm_runtime_mark_last_busy() from the drivers that call
> > pm_runtime_put_autosuspend(), right ?
>
> That should be done but as it doesn't affect the functionality, it can (and
> may only) be done later on --- the current users need to be converted to
> use the to-be-added __pm_runtime_put_autosuspend() first.
True. If you're going to send a series that change things tree-wide I
was thinking that it would be best to address multiple tree-wide changes
at the same time, that would be less churn, especially if it can be
mostly scripted with Coccinelle. That would be my preference (especially
because the issue will then be solved and we'll be able to move to
something else, instead of leaving old code lingering on for a long
time), but it's up to you.
> > This sounds good to me. Thank you for working on this. Two RPM API
> > simplifications in a week, it feels like Christmas is coming :-)
>
> Yes. And it's always the case actually! Only the time that it takes
> differs.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper
2023-11-21 8:50 ` Laurent Pinchart
@ 2023-11-21 10:00 ` Sakari Ailus
0 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-11-21 10:00 UTC (permalink / raw
To: Laurent Pinchart; +Cc: Rafael J. Wysocki, linux-acpi, linux-media, jacopo.mondi
Hi Laurent,
On Tue, Nov 21, 2023 at 10:50:56AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tue, Nov 21, 2023 at 08:41:34AM +0000, Sakari Ailus wrote:
> > On Mon, Nov 20, 2023 at 11:47:43AM +0200, Laurent Pinchart wrote:
> > > On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote:
> > > > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote:
> > > > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote:
> > > > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote:
> > > > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that
> > > > > > > > wish to set the last_busy timestamp to current time and put the
> > > > > > > > usage_count of the device and set the autosuspend timer.
> > > > > > > >
> > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to
> > > > > > > > calling first pm_runtime_mark_last_busy() and then
> > > > > > > > pm_runtime_put_autosuspend().
> > > > > > >
> > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call
> > > > > > > pm_runtime_mark_last_busy() right before. Let's make the
> > > > > > > pm_runtime_put_autosuspend() function do that by default, and add a
> > > > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority
> > > > > > > of cases where updating the last busy timestamp isn't desired. We want
> > > > > > > to simplify the API, not make it more complex.
> > > > > >
> > > > > > I would also prefer it to be done this way if not too problematic.
> > > > >
> > > > > I'm glad you agree :-) The change will probably be a bit painful, but I
> > > > > think it's for the best. Sakari, please let me know if I can help.
> > > >
> > > > I actually do prefer this approach, too.
> > > >
> > > > There about 350 drivers using pm_runtime_autosuspend() currently. Around
> > > > 150 uses pm_runtime_autosuspend() which is not preceded by
> > > > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330.
> > > >
> > > > I checked some of what's left: most do still call both, but in a way that
> > > > evades Coccinelle matching. Some omissions seem to remain.
> > > >
> > > > Given that there are way more users that do also call
> > > > pm_runtime_mark_last_busy(), I think I'll try to introduce
> > > > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend()
> > > > documentation change first and then rename the callers that don't use
> > > > pm_runtime_mark_last_busy().
> > >
> > > And also drop pm_runtime_mark_last_busy() from the drivers that call
> > > pm_runtime_put_autosuspend(), right ?
> >
> > That should be done but as it doesn't affect the functionality, it can (and
> > may only) be done later on --- the current users need to be converted to
> > use the to-be-added __pm_runtime_put_autosuspend() first.
>
> True. If you're going to send a series that change things tree-wide I
> was thinking that it would be best to address multiple tree-wide changes
> at the same time, that would be less churn, especially if it can be
> mostly scripted with Coccinelle. That would be my preference (especially
> because the issue will then be solved and we'll be able to move to
> something else, instead of leaving old code lingering on for a long
> time), but it's up to you.
This will mean around 1000 changed lines in 350 files.
Given the number of changes and how they're scattered around, I'd expect
to merge first the Runtime PM API change, then later on the driver specific
changes via their own trees. Doing it differently risks a large number of
conflicts.
Hopefully faster than changing the I²C driver probe function though.
We also need to have some time before all users of
pm_runtime_put_autosuspend() have been converted, including new ones merged
meanwhile.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2023-11-21 10:00 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-17 11:14 [PATCH v2 0/7] Small Runtime PM API changes Sakari Ailus
2023-11-17 11:14 ` [PATCH v2 1/7] pm: runtime: Simplify pm_runtime_get_if_active() usage Sakari Ailus
2023-11-18 17:46 ` Laurent Pinchart
2023-11-17 11:14 ` [PATCH v2 2/7] pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper Sakari Ailus
2023-11-18 17:49 ` Laurent Pinchart
2023-11-18 21:20 ` Rafael J. Wysocki
2023-11-18 21:30 ` Laurent Pinchart
2023-11-20 9:27 ` Sakari Ailus
2023-11-20 9:47 ` Laurent Pinchart
2023-11-21 8:41 ` Sakari Ailus
2023-11-21 8:50 ` Laurent Pinchart
2023-11-21 10:00 ` Sakari Ailus
2023-11-17 11:14 ` [PATCH v2 3/7] ACPI: Documentation: Document acpi_dev_state_d0() Sakari Ailus
2023-11-18 18:50 ` Laurent Pinchart
2023-11-20 9:31 ` Sakari Ailus
2023-11-20 12:52 ` Rafael J. Wysocki
2023-11-20 20:03 ` Sakari Ailus
2023-11-20 20:22 ` Rafael J. Wysocki
2023-11-20 20:53 ` Sakari Ailus
2023-11-17 11:14 ` [PATCH v2 4/7] media: Documentation: Improve camera sensor runtime PM documentation Sakari Ailus
2023-11-18 18:49 ` Laurent Pinchart
2023-11-17 11:14 ` [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly Sakari Ailus
2023-11-17 15:30 ` Jacopo Mondi
2023-11-18 11:12 ` Sakari Ailus
2023-11-18 17:33 ` Laurent Pinchart
2023-11-20 8:31 ` Sakari Ailus
2023-11-18 8:42 ` kernel test robot
2023-11-17 11:14 ` [PATCH v2 6/7] media: imx319: Put usage_count correctly in s_ctrl callback Sakari Ailus
2023-11-18 18:52 ` Laurent Pinchart
2023-11-20 9:32 ` Sakari Ailus
2023-11-20 9:45 ` Laurent Pinchart
2023-11-21 8:18 ` Sakari Ailus
2023-11-21 8:25 ` Laurent Pinchart
2023-11-21 8:44 ` Sakari Ailus
2023-11-17 11:14 ` [PATCH v2 7/7] media: imx219: " Sakari Ailus
2023-11-17 14:20 ` Dave Stevenson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.