| Message ID | 20230821223001.28480-15-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: imx219: Miscellaneous cleanups and improvements | expand |
Hi Laurent On Mon, 21 Aug 2023 at 23:30, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > There is no need to stop streaming at system suspend time, or restart it > when the system is resumed, as the host-side driver is responsible for > stopping and restarting the pipeline in a controlled way by calling the > .s_stream() operation. Drop the system suspend and resume handlers, and > simplify the .s_stream() handler as a result. I'll believe you, but the docs for power management in camera sensor drivers [1] state "Please see examples in e.g. drivers/media/i2c/ov8856.c and drivers/media/i2c/ccs/ccs-core.c. The two drivers work in both ACPI and DT based systems." Looking at CCS we find the suspend hook stopping streaming [2], and resume hook starting it [3]. Same in ov8856 [4]. Could you reference the documentation that states that the host-side driver is responsible for starting and stopping? Is this an ACPI vs DT difference? Functionally the patch does what you say, so would get a R-b tag otherwise. Dave [1] https://www.kernel.org/doc/html/latest/driver-api/media/camera-sensor.html#power-management [2] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ccs/ccs-core.c#L3155-L3173 [3] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ccs/ccs-core.c#L3185-L3186 [4] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov8856.c#L2128-L2164 > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/imx219.c | 56 ++------------------------------------ > 1 file changed, 2 insertions(+), 54 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 227c227cf4ce..da2a8d0210fa 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -359,9 +359,6 @@ struct imx219 { > struct v4l2_ctrl *vblank; > struct v4l2_ctrl *hblank; > > - /* Streaming on/off */ > - bool streaming; > - > /* Two or Four lanes */ > u8 lanes; > }; > @@ -764,24 +761,11 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable) > > state = v4l2_subdev_lock_and_get_active_state(sd); > > - if (imx219->streaming == enable) > - goto unlock; > - > - if (enable) { > - /* > - * Apply default & customized values > - * and then start streaming. > - */ > + if (enable) > ret = imx219_start_streaming(imx219, state); > - if (ret) > - goto unlock; > - } else { > + else > imx219_stop_streaming(imx219); > - } > > - imx219->streaming = enable; > - > -unlock: > v4l2_subdev_unlock_state(state); > return ret; > } > @@ -1015,41 +999,6 @@ static int imx219_power_off(struct device *dev) > return 0; > } > > -static int __maybe_unused imx219_suspend(struct device *dev) > -{ > - struct v4l2_subdev *sd = dev_get_drvdata(dev); > - struct imx219 *imx219 = to_imx219(sd); > - > - if (imx219->streaming) > - imx219_stop_streaming(imx219); > - > - return 0; > -} > - > -static int __maybe_unused imx219_resume(struct device *dev) > -{ > - struct v4l2_subdev *sd = dev_get_drvdata(dev); > - struct imx219 *imx219 = to_imx219(sd); > - struct v4l2_subdev_state *state; > - int ret; > - > - if (imx219->streaming) { > - state = v4l2_subdev_lock_and_get_active_state(sd); > - ret = imx219_start_streaming(imx219, state); > - v4l2_subdev_unlock_state(state); > - if (ret) > - goto error; > - } > - > - return 0; > - > -error: > - imx219_stop_streaming(imx219); > - imx219->streaming = false; > - > - return ret; > -} > - > /* ----------------------------------------------------------------------------- > * Probe & remove > */ > @@ -1295,7 +1244,6 @@ static const struct of_device_id imx219_dt_ids[] = { > MODULE_DEVICE_TABLE(of, imx219_dt_ids); > > static const struct dev_pm_ops imx219_pm_ops = { > - SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume) > SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL) > }; > > -- > Regards, > > Laurent Pinchart >
Hi Dave, Laurent, On Tue, Aug 22, 2023 at 06:58:47PM +0100, Dave Stevenson wrote: > Hi Laurent > > On Mon, 21 Aug 2023 at 23:30, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > There is no need to stop streaming at system suspend time, or restart it > > when the system is resumed, as the host-side driver is responsible for > > stopping and restarting the pipeline in a controlled way by calling the > > .s_stream() operation. Drop the system suspend and resume handlers, and > > simplify the .s_stream() handler as a result. > > I'll believe you, but the docs for power management in camera sensor > drivers [1] state > "Please see examples in e.g. drivers/media/i2c/ov8856.c and > drivers/media/i2c/ccs/ccs-core.c. The two drivers work in both ACPI > and DT based systems." > > Looking at CCS we find the suspend hook stopping streaming [2], and > resume hook starting it [3]. Same in ov8856 [4]. > > Could you reference the documentation that states that the host-side > driver is responsible for starting and stopping? Is this an ACPI vs DT > difference? There's no difference between DT and ACPI, no. Starting streaming on resume from system suspend is haphazard as there's no guarantee on the timing of resuming devices (if the suspend and resume independently), or similarly if the receiver driver explicitly starts streaming, there's no guarantee the sub-device driver has resumed. So I think we'd need more than what currently exists on the framework side to do this reliably --- at least when it comes to CSI-2.
Hi Sakari, On Wed, Aug 23, 2023 at 09:01:56AM +0000, Sakari Ailus wrote: > On Tue, Aug 22, 2023 at 06:58:47PM +0100, Dave Stevenson wrote: > > On Mon, 21 Aug 2023 at 23:30, Laurent Pinchart wrote: > > > > > > There is no need to stop streaming at system suspend time, or restart it > > > when the system is resumed, as the host-side driver is responsible for > > > stopping and restarting the pipeline in a controlled way by calling the > > > .s_stream() operation. Drop the system suspend and resume handlers, and > > > simplify the .s_stream() handler as a result. > > > > I'll believe you, but the docs for power management in camera sensor > > drivers [1] state > > "Please see examples in e.g. drivers/media/i2c/ov8856.c and > > drivers/media/i2c/ccs/ccs-core.c. The two drivers work in both ACPI > > and DT based systems." > > > > Looking at CCS we find the suspend hook stopping streaming [2], and > > resume hook starting it [3]. Same in ov8856 [4]. > > > > Could you reference the documentation that states that the host-side > > driver is responsible for starting and stopping? Is this an ACPI vs DT > > difference? > > There's no difference between DT and ACPI, no. The comment about drivers working with both ACPI- and DT-based systems was related, as far as I understand, to usage of runtime PM, I don't think it was related to system PM at all. > Starting streaming on resume from system suspend is haphazard as there's no > guarantee on the timing of resuming devices (if the suspend and resume > independently), or similarly if the receiver driver explicitly starts > streaming, there's no guarantee the sub-device driver has resumed. > > So I think we'd need more than what currently exists on the framework side > to do this reliably --- at least when it comes to CSI-2. Device links help here. See for instance mxc_isi_async_notifier_bound() in drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c. Ideally this shouldn't be handled by drivers but by the V4L2 core though. In any case, the sensors don't need to deal with it, enforcing correct ordering is an issue for the connected receiver. Sakari, you didn't explicitly answer whether you think that system suspend/resume handlers are needed or not for camera sensor drivers. What is your opinion on this ?
Hi Laurent, On Wed, Aug 23, 2023 at 12:14:46PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Aug 23, 2023 at 09:01:56AM +0000, Sakari Ailus wrote: > > On Tue, Aug 22, 2023 at 06:58:47PM +0100, Dave Stevenson wrote: > > > On Mon, 21 Aug 2023 at 23:30, Laurent Pinchart wrote: > > > > > > > > There is no need to stop streaming at system suspend time, or restart it > > > > when the system is resumed, as the host-side driver is responsible for > > > > stopping and restarting the pipeline in a controlled way by calling the > > > > .s_stream() operation. Drop the system suspend and resume handlers, and > > > > simplify the .s_stream() handler as a result. > > > > > > I'll believe you, but the docs for power management in camera sensor > > > drivers [1] state > > > "Please see examples in e.g. drivers/media/i2c/ov8856.c and > > > drivers/media/i2c/ccs/ccs-core.c. The two drivers work in both ACPI > > > and DT based systems." > > > > > > Looking at CCS we find the suspend hook stopping streaming [2], and > > > resume hook starting it [3]. Same in ov8856 [4]. > > > > > > Could you reference the documentation that states that the host-side > > > driver is responsible for starting and stopping? Is this an ACPI vs DT > > > difference? > > > > There's no difference between DT and ACPI, no. > > The comment about drivers working with both ACPI- and DT-based systems > was related, as far as I understand, to usage of runtime PM, I don't > think it was related to system PM at all. > > > Starting streaming on resume from system suspend is haphazard as there's no > > guarantee on the timing of resuming devices (if the suspend and resume > > independently), or similarly if the receiver driver explicitly starts > > streaming, there's no guarantee the sub-device driver has resumed. > > > > So I think we'd need more than what currently exists on the framework side > > to do this reliably --- at least when it comes to CSI-2. > > Device links help here. See for instance mxc_isi_async_notifier_bound() > in drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c. Ideally this > shouldn't be handled by drivers but by the V4L2 core though. In any > case, the sensors don't need to deal with it, enforcing correct ordering > is an issue for the connected receiver. Nice idea. Perhaps this could be integrated somehow to the V4L2 async framework? And it depends on streaming control --- if you don't have streaming ongoing, then you don't need that dependency. Also errors in resuming should only be related to the devices that failed to resume. It's going to get complicated. > > Sakari, you didn't explicitly answer whether you think that system > suspend/resume handlers are needed or not for camera sensor drivers. > What is your opinion on this ? Streaming needs to be explicitly started and stopped by the downstream pipeline, so camera sensors shouldn't need suspend or resume handlers (at least for this reason).
On Wed, Aug 23, 2023 at 09:36:10AM +0000, Sakari Ailus wrote: > On Wed, Aug 23, 2023 at 12:14:46PM +0300, Laurent Pinchart wrote: > > On Wed, Aug 23, 2023 at 09:01:56AM +0000, Sakari Ailus wrote: > > > On Tue, Aug 22, 2023 at 06:58:47PM +0100, Dave Stevenson wrote: > > > > On Mon, 21 Aug 2023 at 23:30, Laurent Pinchart wrote: > > > > > > > > > > There is no need to stop streaming at system suspend time, or restart it > > > > > when the system is resumed, as the host-side driver is responsible for > > > > > stopping and restarting the pipeline in a controlled way by calling the > > > > > .s_stream() operation. Drop the system suspend and resume handlers, and > > > > > simplify the .s_stream() handler as a result. > > > > > > > > I'll believe you, but the docs for power management in camera sensor > > > > drivers [1] state > > > > "Please see examples in e.g. drivers/media/i2c/ov8856.c and > > > > drivers/media/i2c/ccs/ccs-core.c. The two drivers work in both ACPI > > > > and DT based systems." > > > > > > > > Looking at CCS we find the suspend hook stopping streaming [2], and > > > > resume hook starting it [3]. Same in ov8856 [4]. > > > > > > > > Could you reference the documentation that states that the host-side > > > > driver is responsible for starting and stopping? Is this an ACPI vs DT > > > > difference? > > > > > > There's no difference between DT and ACPI, no. > > > > The comment about drivers working with both ACPI- and DT-based systems > > was related, as far as I understand, to usage of runtime PM, I don't > > think it was related to system PM at all. > > > > > Starting streaming on resume from system suspend is haphazard as there's no > > > guarantee on the timing of resuming devices (if the suspend and resume > > > independently), or similarly if the receiver driver explicitly starts > > > streaming, there's no guarantee the sub-device driver has resumed. > > > > > > So I think we'd need more than what currently exists on the framework side > > > to do this reliably --- at least when it comes to CSI-2. > > > > Device links help here. See for instance mxc_isi_async_notifier_bound() > > in drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c. Ideally this > > shouldn't be handled by drivers but by the V4L2 core though. In any > > case, the sensors don't need to deal with it, enforcing correct ordering > > is an issue for the connected receiver. > > Nice idea. Perhaps this could be integrated somehow to the V4L2 async > framework? And it depends on streaming control --- if you don't have > streaming ongoing, then you don't need that dependency. Also errors in > resuming should only be related to the devices that failed to resume. It's > going to get complicated. I think the dependency should always be there, regardless of whether there's an active stream or not. It doesn't hurt to ensure the CSI-2 RX will always get suspended before the sensor. > > Sakari, you didn't explicitly answer whether you think that system > > suspend/resume handlers are needed or not for camera sensor drivers. > > What is your opinion on this ? > > Streaming needs to be explicitly started and stopped by the downstream > pipeline, so camera sensors shouldn't need suspend or resume handlers (at > least for this reason). \o/ That's the answer I was hoping for :-)
Hi Laurent, On Wed, Aug 23, 2023 at 12:57:00PM +0300, Laurent Pinchart wrote: > On Wed, Aug 23, 2023 at 09:36:10AM +0000, Sakari Ailus wrote: > > On Wed, Aug 23, 2023 at 12:14:46PM +0300, Laurent Pinchart wrote: > > > On Wed, Aug 23, 2023 at 09:01:56AM +0000, Sakari Ailus wrote: > > > > On Tue, Aug 22, 2023 at 06:58:47PM +0100, Dave Stevenson wrote: > > > > > On Mon, 21 Aug 2023 at 23:30, Laurent Pinchart wrote: > > > > > > > > > > > > There is no need to stop streaming at system suspend time, or restart it > > > > > > when the system is resumed, as the host-side driver is responsible for > > > > > > stopping and restarting the pipeline in a controlled way by calling the > > > > > > .s_stream() operation. Drop the system suspend and resume handlers, and > > > > > > simplify the .s_stream() handler as a result. > > > > > > > > > > I'll believe you, but the docs for power management in camera sensor > > > > > drivers [1] state > > > > > "Please see examples in e.g. drivers/media/i2c/ov8856.c and > > > > > drivers/media/i2c/ccs/ccs-core.c. The two drivers work in both ACPI > > > > > and DT based systems." > > > > > > > > > > Looking at CCS we find the suspend hook stopping streaming [2], and > > > > > resume hook starting it [3]. Same in ov8856 [4]. > > > > > > > > > > Could you reference the documentation that states that the host-side > > > > > driver is responsible for starting and stopping? Is this an ACPI vs DT > > > > > difference? > > > > > > > > There's no difference between DT and ACPI, no. > > > > > > The comment about drivers working with both ACPI- and DT-based systems > > > was related, as far as I understand, to usage of runtime PM, I don't > > > think it was related to system PM at all. > > > > > > > Starting streaming on resume from system suspend is haphazard as there's no > > > > guarantee on the timing of resuming devices (if the suspend and resume > > > > independently), or similarly if the receiver driver explicitly starts > > > > streaming, there's no guarantee the sub-device driver has resumed. > > > > > > > > So I think we'd need more than what currently exists on the framework side > > > > to do this reliably --- at least when it comes to CSI-2. > > > > > > Device links help here. See for instance mxc_isi_async_notifier_bound() > > > in drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c. Ideally this > > > shouldn't be handled by drivers but by the V4L2 core though. In any > > > case, the sensors don't need to deal with it, enforcing correct ordering > > > is an issue for the connected receiver. > > > > Nice idea. Perhaps this could be integrated somehow to the V4L2 async > > framework? And it depends on streaming control --- if you don't have > > streaming ongoing, then you don't need that dependency. Also errors in > > resuming should only be related to the devices that failed to resume. It's > > going to get complicated. > > I think the dependency should always be there, regardless of whether > there's an active stream or not. It doesn't hurt to ensure the CSI-2 RX > will always get suspended before the sensor. If you have multiple sensor sub-devices and only a single receiver device (with multiple receivers), what do you do if one of the sensors fails to resume? I'd think this should be handled similarly to probe case --- but we know we have things to fix there. > > > > Sakari, you didn't explicitly answer whether you think that system > > > suspend/resume handlers are needed or not for camera sensor drivers. > > > What is your opinion on this ? > > > > Streaming needs to be explicitly started and stopped by the downstream > > pipeline, so camera sensors shouldn't need suspend or resume handlers (at > > least for this reason). > > \o/ That's the answer I was hoping for :-) I don't think it could work otherwise. :-) This should be easy, too: receiver drivers do this already when starting and stopping streaming.
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index 227c227cf4ce..da2a8d0210fa 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -359,9 +359,6 @@ struct imx219 { struct v4l2_ctrl *vblank; struct v4l2_ctrl *hblank; - /* Streaming on/off */ - bool streaming; - /* Two or Four lanes */ u8 lanes; }; @@ -764,24 +761,11 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable) state = v4l2_subdev_lock_and_get_active_state(sd); - if (imx219->streaming == enable) - goto unlock; - - if (enable) { - /* - * Apply default & customized values - * and then start streaming. - */ + if (enable) ret = imx219_start_streaming(imx219, state); - if (ret) - goto unlock; - } else { + else imx219_stop_streaming(imx219); - } - imx219->streaming = enable; - -unlock: v4l2_subdev_unlock_state(state); return ret; } @@ -1015,41 +999,6 @@ static int imx219_power_off(struct device *dev) return 0; } -static int __maybe_unused imx219_suspend(struct device *dev) -{ - struct v4l2_subdev *sd = dev_get_drvdata(dev); - struct imx219 *imx219 = to_imx219(sd); - - if (imx219->streaming) - imx219_stop_streaming(imx219); - - return 0; -} - -static int __maybe_unused imx219_resume(struct device *dev) -{ - struct v4l2_subdev *sd = dev_get_drvdata(dev); - struct imx219 *imx219 = to_imx219(sd); - struct v4l2_subdev_state *state; - int ret; - - if (imx219->streaming) { - state = v4l2_subdev_lock_and_get_active_state(sd); - ret = imx219_start_streaming(imx219, state); - v4l2_subdev_unlock_state(state); - if (ret) - goto error; - } - - return 0; - -error: - imx219_stop_streaming(imx219); - imx219->streaming = false; - - return ret; -} - /* ----------------------------------------------------------------------------- * Probe & remove */ @@ -1295,7 +1244,6 @@ static const struct of_device_id imx219_dt_ids[] = { MODULE_DEVICE_TABLE(of, imx219_dt_ids); static const struct dev_pm_ops imx219_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume) SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL) };
There is no need to stop streaming at system suspend time, or restart it when the system is resumed, as the host-side driver is responsible for stopping and restarting the pipeline in a controlled way by calling the .s_stream() operation. Drop the system suspend and resume handlers, and simplify the .s_stream() handler as a result. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/i2c/imx219.c | 56 ++------------------------------------ 1 file changed, 2 insertions(+), 54 deletions(-)