| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240722112541.GD13497@pendragon.ideasonboard.com>
Date: Mon, 22 Jul 2024 14:25:41 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Changhuang Liang <changhuang.liang@...rfivetech.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Maxime Ripard <mripard@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Jacopo Mondi <jacopo.mondi@...asonboard.com>,
Jack Zhu <jack.zhu@...rfivetech.com>,
Keith Zhao <keith.zhao@...rfivetech.com>,
Jayshri Pawar <jpawar@...ence.com>, Jai Luthra <j-luthra@...com>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>
Subject: Re: 回复: [PATCH v2 2/5] media:
cadence: csi2rx: Add system PM support
On Mon, Jul 22, 2024 at 11:53:02AM +0300, Tomi Valkeinen wrote:
> Hi,
>
> On 22/07/2024 05:17, Changhuang Liang wrote:
> > Hi, Tomi
> >
> >> Hi,
> >>
> >> On 18/07/2024 06:28, Changhuang Liang wrote:
> >>> Add system PM support make it stopping streaming at system suspend
> >>> time, and restarting streaming at system resume time.
> >>>
> >>> Signed-off-by: Changhuang Liang <changhuang.liang@...rfivetech.com>
> >>> ---
> >>> drivers/media/platform/cadence/cdns-csi2rx.c | 32
> >> ++++++++++++++++++++
> >>> 1 file changed, 32 insertions(+)
> >>>
> >>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> >>> b/drivers/media/platform/cadence/cdns-csi2rx.c
> >>> index 981819adbb3a..81e90b31e9f8 100644
> >>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> >>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> >>> @@ -776,8 +776,40 @@ static int csi2rx_runtime_resume(struct device
> >> *dev)
> >>> return ret;
> >>> }
> >>>
> >>> +static int __maybe_unused csi2rx_suspend(struct device *dev) {
> >>> + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> >>> +
> >>> + mutex_lock(&csi2rx->lock);
> >>> + if (csi2rx->count)
> >>> + csi2rx_stop(csi2rx);
> >>> + mutex_unlock(&csi2rx->lock);
> >>> +
> >>> + pm_runtime_force_suspend(dev);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int __maybe_unused csi2rx_resume(struct device *dev) {
> >>> + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> >>> + int ret;
> >>> +
> >>> + ret = pm_runtime_force_resume(dev);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + mutex_lock(&csi2rx->lock);
> >>> + if (csi2rx->count)
> >>> + csi2rx_start(csi2rx);
> >>> + mutex_unlock(&csi2rx->lock);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> static const struct dev_pm_ops csi2rx_pm_ops = {
> >>> SET_RUNTIME_PM_OPS(csi2rx_runtime_suspend,
> >> csi2rx_runtime_resume,
> >>> NULL)
> >>> + SET_SYSTEM_SLEEP_PM_OPS(csi2rx_suspend, csi2rx_resume)
> >>> };
> >>>
> >>> static const struct of_device_id csi2rx_of_table[] = {
> >>
> >> If I'm not mistaken, this is a subdev driver, and is somewhere in the middle of
> >> the pipeline. Afaiu, only the driver that handles the v4l2 video devices should
> >> have system suspend hooks. The job of that driver is then to disable or enable
> >> the pipeline using v4l2 functions, and for the rest of the pipeline system
> >> suspend looks just like a normal pipeline disable.
> >>
> >
> > I see that the imx219 has a commit:
> >
> > commit b8074db07429b845b805416d261b502f814a80fe
> > Author: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > Date: Thu Sep 14 21:16:49 2023 +0300
> >
> > media: i2c: imx219: Drop system suspend and resume handlers
> >
> > Stopping streaming on a camera pipeline at system suspend time, and
> > restarting it at system resume time, requires coordinated action between
> > the bridge driver and the camera sensor driver. This is handled by the
> > bridge driver calling the sensor's .s_stream() handler at system suspend
> > and resume time. There is thus no need for the sensor to independently
> > implement system sleep PM operations. Drop them.
> >
> > The streaming field of the driver's private structure is now unused,
> > drop it as well.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > Reviewed-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@...ux.intel.com>
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@...all.nl>
> >
> > Implement the system PM of sensor using bridge. This csi2rx is also a bridge.
> > So I add system PM in this driver.
>
> It is not a bridge in the sense that the commit message means. The
> system suspend should be handled in the last (or first, depending on
> which way you think about it) driver in the pipeline, the one that
> handles the VIDIOC_STREAMON.
That's right. Suspending/resuming a running camera pipeline is a complex
operation that requires careful sequencing. For instance, if you resume
a MIPI CSI-2 camera sensor before the CSI-2 receiver, it will switch its
clock and data lanes to HS mode before the CSI-2 receiver is ready to
synchronize.
These sequencing requirements are already handled at VIDIOC_STREAMON and
VIDIOC_STREAMOFF time. That's why we leverage that code for the
suspend/resume implementation, the "main" driver that handles the video
nodes and implements stream start/stop has to stop the pipeline at
suspend time and restart it at resume time. Drivers for components along
the pipeline will then see their .s_stream() operation being called, as
done when starting/stopping the pipeline normally. They don't have to
implement anything specific related to pipeline stop/start in their
system suspend/resume handlers.
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists