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. -- Regards, Sakari Ailus
