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

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

[Andrey Skvortsov]

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@gmail.com>
---
 drivers/media/i2c/ov5640.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 5fe85aa2d2ec..8130471caaa6 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -3970,6 +3970,8 @@ static void ov5640_remove(struct i2c_client *client)
 }
 
 static const struct dev_pm_ops ov5640_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
 	SET_RUNTIME_PM_OPS(ov5640_sensor_suspend, ov5640_sensor_resume, NULL)
 };
 
-- 
2.40.1

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

[Sakari Ailus]

Hi Andrey,

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@gmail.com>

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.

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.

-- 
Kind regards,

Sakari Ailus

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

[Andrey Skvortsov]

Hi Sakari,

On 23-09-13 15:27, Sakari Ailus wrote:
> Hi Andrey,
> 
> 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@gmail.com>
> 
> 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?

> 
> 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.

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

-- 
Best regards,
Andrey Skvortsov

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

[Jacopo Mondi]

Hi Andrey

On Wed, Sep 13, 2023 at 11:48:13PM +0300, Andrey Skvortsov wrote:
> Hi Sakari,
>
> On 23-09-13 15:27, Sakari Ailus wrote:
> > Hi Andrey,
> >
> > 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@gmail.com>
> >
> > 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@ideasonboard.com/

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 ?

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 ?

>
> 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 ?

Thanks
  j

>
> --
> Best regards,
> Andrey Skvortsov

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@gmail.com>
> > >
> > > 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@ideasonboard.com/
> 
> 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

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

[Sakari Ailus]

Hi Laurent,

On Thu, Sep 14, 2023 at 12:02:41PM +0300, Laurent Pinchart wrote:
> 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@gmail.com>
> > > >
> > > > 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@ideasonboard.com/
> > 
> > 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.

I can't see why it wouldn't be, and it seems easy, too. We're still early
in the cycle so if someone writes the patches, I can't see why we couldn't
get them for 6.7.

-- 
Regards,

Sakari Ailus