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@xxxxxxxxxxxxxxx> > --- > .../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
