diff mbox series

[v2,5/5] staging: media: starfive: Add system PM support

Message ID 20240718032834.53876-6-changhuang.liang@starfivetech.com
State New
Headers show
Series Add StarFive Camera Subsystem hibernation support | expand

Commit Message

Changhuang Liang July 18, 2024, 3:28 a.m. UTC
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@starfivetech.com>
---
 .../staging/media/starfive/camss/stf-camss.c  | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

Comments

Jacopo Mondi July 18, 2024, 2:55 p.m. UTC | #1
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@starfivetech.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 ?


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

> +	}
> +
> +	return 0;
> +}
> +
>  static const struct dev_pm_ops stfcamss_pm_ops = {
>  	SET_RUNTIME_PM_OPS(stfcamss_runtime_suspend,
>  			   stfcamss_runtime_resume,
>  			   NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(stfcamss_suspend, stfcamss_resume)
>  };
>
>  static struct platform_driver stfcamss_driver = {
> --
> 2.25.1
>
Changhuang Liang July 19, 2024, 2:08 a.m. UTC | #2
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@starfivetech.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
Jacopo Mondi July 22, 2024, 8:57 a.m. UTC | #3
Hi Changhuang

On Fri, Jul 19, 2024 at 02:08:20AM GMT, Changhuang Liang wrote:
> 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@starfivetech.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.
>

Sorry, maybe I was not clear (and I was probably confused as well).

You are right this is the main entry point for system sleep PM hooks

> >
> > > +
> > > +	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);

And here you propagate the start_streaming (and stop_streaming on
suspend) call to all your video devices.

I see your video devices propagating the s_stream call to their
'source_subdev'. And your ISP subdev doing the same in
'isp_set_stream()'.

According to the media graph in
Documentation/admin-guide/media/starfive_camss_graph.dot

your 'capture_yuv' video device is connected to your ISP, and your
'capture_raw' video device is connected to your 'CSI-RX' subdev.

If my understanding is correct, your CSI-RX subdev will receive 2
calls to s_stream() (one from the ISP subdev and one from the
'capture_raw' video device). Am I mistaken maybe ?

Also, if the CSI-RX subdev is already part of a capture pipeline, as
Tomi pointed out in his review of patch [2/5] it doesn't need to
implement handlers for system suspend/resume.


> >
> > You can use vb2_is_streaming() maybe.

I was suggesting to use vb2_is_streaming() instead of openly code

		if (video->vb2_q.streaming)

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

Yeah I was wondering if you could have saved manually tracking the
streaming state in the isp (re-reading the patches, where do you
actually use the 'streaming' flag in the ISP subdev ?) by tracking the
vb2_queue state.

>
> Regards,
> Changhuang
Changhuang Liang July 22, 2024, 9:25 a.m. UTC | #4
Hi, Jacopo

> 
> Hi Changhuang
> 
> On Fri, Jul 19, 2024 at 02:08:20AM GMT, Changhuang Liang wrote:
> > 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@starfivetech.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.
> >
> 
> Sorry, maybe I was not clear (and I was probably confused as well).
> 
> You are right this is the main entry point for system sleep PM hooks
> 
> > >
> > > > +
> > > > +	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);
> 
> And here you propagate the start_streaming (and stop_streaming on
> suspend) call to all your video devices.
> 
> I see your video devices propagating the s_stream call to their
> 'source_subdev'. And your ISP subdev doing the same in 'isp_set_stream()'.
> 
> According to the media graph in
> Documentation/admin-guide/media/starfive_camss_graph.dot
> 
> your 'capture_yuv' video device is connected to your ISP, and your
> 'capture_raw' video device is connected to your 'CSI-RX' subdev.
> 
> If my understanding is correct, your CSI-RX subdev will receive 2 calls to
> s_stream() (one from the ISP subdev and one from the 'capture_raw' video
> device). Am I mistaken maybe ?
> 
> Also, if the CSI-RX subdev is already part of a capture pipeline, as Tomi pointed
> out in his review of patch [2/5] it doesn't need to implement handlers for
> system suspend/resume.
> 

Currently this version start streaming and stop streaming are called

static const struct stfcamss_video_ops stf_capture_ops = {
	.queue_buffer = stf_queue_buffer,
	.flush_buffers = stf_flush_buffers,
	.start_streaming = stf_capture_start,
	.stop_streaming = stf_capture_stop,
};

This two hooks will not propagate streaming.

Maybe I should change to use this in next version:

static const struct vb2_ops stf_video_vb2_q_ops = {
	.queue_setup     = video_queue_setup,
	.wait_prepare    = vb2_ops_wait_prepare,
	.wait_finish     = vb2_ops_wait_finish,
	.buf_init        = video_buf_init,
	.buf_prepare     = video_buf_prepare,
	.buf_queue       = video_buf_queue,
	.start_streaming = video_start_streaming,
	.stop_streaming  = video_stop_streaming,
};

This two hooks will propagate streaming.

> 
> > >
> > > You can use vb2_is_streaming() maybe.
> 
> I was suggesting to use vb2_is_streaming() instead of openly code
> 
> 		if (video->vb2_q.streaming)
> 
> > > 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.
> 
> Yeah I was wondering if you could have saved manually tracking the streaming
> state in the isp (re-reading the patches, where do you actually use the
> 'streaming' flag in the ISP subdev ?) by tracking the vb2_queue state.
> 

If use propagate streaming on pipeline, maybe can drop the streaming flag
for subdev. 

Regards,
Changhuang
diff mbox series

Patch

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;
+	unsigned int i;
+
+	for (i = 0; i < STF_CAPTURE_NUM; ++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;
+	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);
+
+	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);
+	}
+
+	return 0;
+}
+
 static const struct dev_pm_ops stfcamss_pm_ops = {
 	SET_RUNTIME_PM_OPS(stfcamss_runtime_suspend,
 			   stfcamss_runtime_resume,
 			   NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(stfcamss_suspend, stfcamss_resume)
 };
 
 static struct platform_driver stfcamss_driver = {