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

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

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
>
  

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
  

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
  

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