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

[Jacopo Mondi]

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@xxxxxxxxxxxxxxxx>
> ---
>  .../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
>