Re: [PATCH] media: ov5640: use pm_runtime_force_suspend/resume for system suspend

[Laurent Pinchart]

On Thu, Sep 14, 2023 at 10:54:40AM +0200, Jacopo Mondi wrote:
> On Wed, Sep 13, 2023 at 11:48:13PM +0300, Andrey Skvortsov wrote:
> > On 23-09-13 15:27, Sakari Ailus wrote:
> > > On Fri, Aug 18, 2023 at 08:34:16PM +0300, Andrey Skvortsov wrote:
> > > > If system was suspended while camera sensor was used, data and
> > > > interrupts were still coming from sensor and that caused unstable
> > > > system. Sometimes system hanged during a resume. Use
> > > > pm_runtime_force_* helpers in order to support system suspend.
> > > >
> > > > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@xxxxxxxxx>
> > >
> > > Thanks for the patch.
> > >
> > > It's not been documented really how system suspend and resume should
> > > work for complex cameras. But I don't think it can be done by drivers
> > > separately as the CSI-2 bus initialisation requires actions from both
> > > sender and receiver drivers, at particular points of time.
> >
> > Thanks for the review.
> >
> > I've tested this on PinePhone A64. It uses DVP, maybe because of that
> > system suspend/resume worked good in my case.
> > Originally I've implemented system suspend/resume similar to this [1]
> > or [2] as I've seen this approach in other mainlined drivers. But some
> > drivers reuse pm_runtime_force_* helpers, so I've went with this.
> >
> > Do you think it would be better to use something like [2] until there
> > is better well defined way for system suspend/resume for complex cameras?
> >
> 
> please don't :)
> https://patchwork.linuxtv.org/project/linux-media/patch/20230913135638.26277-16-laurent.pinchart@xxxxxxxxxxxxxxxx/
> 
> However...
> 
> > > So I think we'll need to initiate this from the driver handling DMA, just
> > > as starting and stopping streaming. Even then, there needs to be a
> > > certainty that the sensor device has resumed before streaming is started. I
> > > recall Laurent suggested device links for that purpose, but I don't think
> > > any work has been done to implement it that way.
> 
> .. as Sakari suggested, the driver handling the DMA should be in
> charge of calling s_stream() on the sensor subdev in its
> suspend/resume handlers. There's the risk the receiver resumes while
> the sensor is still suspended, and at this time there's no solution in
> mainline to handle this correctly.
> 
> Laurent/Sakari, how should this be handled for the time being ?

device_link() should handle this. See mxc_isi_async_notifier_bound() in
drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c.

It would be nice if this could be done in the V4L2 core. I haven't
checked if that's possible.

> Laurent's patch for imx219 (and his forthcoming patch to remove the
> same pattern from all sensor drivers in mainline) might be opening th
> door for unexpected bugs as long as we don't enforce a resume/suspend
> ordering ?

I'd argue that the bugs are there already, at least for CSI-2 sensors
the use a continuous clock. If you resume streaming on the sensor first,
the clock lane will switch to HS mode before the receiver gets resumed,
and the receiver won't be able to synchronize.

System suspend/resume tends to be badly tested in camera sensor drivers,
so lots of bugs creep in :-(

> > 1. https://salsa.debian.org/Mobian-team/devices/kernels/sunxi64-linux/-/blob/mobian-6.1/debian/patches/camera/0076-media-gc2145-implement-system-suspend.patch
> > 2. https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/imx219.c#L1159
> 
> Can we get a link to the receiver driver you're using in your kernel ?

-- 
Regards,

Laurent Pinchart