linux-kernel - 回复: [PATCH v2 5/5] staging: media: starfive: Add system PM support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <ZQ0PR01MB13020F080F8D1B024D211971F2AD2@ZQ0PR01MB1302.CHNPR01.prod.partner.outlook.cn>
Date: Fri, 19 Jul 2024 02:08:20 +0000
From: Changhuang Liang <changhuang.liang@...rfivetech.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
CC: Mauro Carvalho Chehab <mchehab@...nel.org>, Maxime Ripard
	<mripard@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Hans
 Verkuil <hverkuil-cisco@...all.nl>, Laurent Pinchart
	<laurent.pinchart@...asonboard.com>, Tomi Valkeinen
	<tomi.valkeinen+renesas@...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:
 回复: [PATCH v2 5/5] staging: media: starfive: Add system PM support

Hi Jacopo,

Thanks for your comments.

> 
> Hi Changhuang
> 
> On Wed, Jul 17, 2024 at 08:28:34PM GMT, Changhuang Liang wrote:
> > This patch implements system suspend and system resume operation for
> > StarFive Camera Subsystem. It supports hibernation during streaming
> > and restarts streaming at system resume time.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@...rfivetech.com>
> > ---
> >  .../staging/media/starfive/camss/stf-camss.c  | 49
> > +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/staging/media/starfive/camss/stf-camss.c
> > b/drivers/staging/media/starfive/camss/stf-camss.c
> > index fecd3e67c7a1..8dcd35aef69d 100644
> > --- a/drivers/staging/media/starfive/camss/stf-camss.c
> > +++ b/drivers/staging/media/starfive/camss/stf-camss.c
> > @@ -416,10 +416,59 @@ static int __maybe_unused
> stfcamss_runtime_resume(struct device *dev)
> >  	return 0;
> >  }
> >
> > +static int __maybe_unused stfcamss_suspend(struct device *dev) {
> > +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> > +	struct stfcamss_video *video;
> 
> Can be declared inside the for loop
> 
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
> 
> Likewise, if you like it, you can
> 
>         for (unsigned int i...
> 
> > +		video = &stfcamss->captures[i].video;
> > +		if (video->vb2_q.streaming) {
> > +			video->ops->stop_streaming(video);
> > +			video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
> > +		}
> > +	}
> > +
> > +	return pm_runtime_force_suspend(dev); }
> > +
> > +static int __maybe_unused stfcamss_resume(struct device *dev) {
> > +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> > +	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
> > +	struct v4l2_subdev_state *sd_state;
> > +	struct stfcamss_video *video;
> > +	unsigned int i;
> 
> same here
> 
> > +	int ret;
> > +
> > +	ret = pm_runtime_force_resume(dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to resume\n");
> > +		return ret;
> > +	}
> > +
> > +	sd_state = v4l2_subdev_lock_and_get_active_state(&isp_dev->subdev);
> > +
> > +	if (isp_dev->streaming)
> > +		stf_isp_stream_on(isp_dev, sd_state);
> 
> I was wondering if you shouldn't propagate start_streaming along the whole
> pipline, but I presume the connected subdevs have to handle resuming
> streaming after a system resume themselves ?
> 

Currently our Camera subsystem contains ISP subdev , capture_raw video device, and capture_yuv
video device. So you can see only one system PM hook can be used by them.

> 
> > +
> > +	v4l2_subdev_unlock_state(sd_state);
> > +
> > +	for (i = 0; i < STF_CAPTURE_NUM; ++i) {
> > +		video = &stfcamss->captures[i].video;
> > +		if (video->vb2_q.streaming)
> > +			video->ops->start_streaming(video);
> 
> You can use vb2_is_streaming() maybe.
> If the queue is streaming, do you need to keep a 'streaming' flag for the isp ?
> Probably yes, as the ISP subdev is used by several video nodes ?
> 

I set the "streaming" flag in PATCH 4, so it does not affect that even if several video 
nodes use it.

Regards,
Changhuang