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