Quoting Daniel Scally (2021-10-09 23:18:00)
> Hi Kieran
>
> On 09/10/2021 22:47, Kieran Bingham wrote:
> > Hi Dan,
> >
> > Quoting Daniel Scally (2021-10-09 00:05:13)
> >> The .suspend() and .resume() runtime_pm operations for the ipu3-cio2
> >> driver currently do not handle the sensor's stream. Setting .s_stream() on
> >> or off for the sensor subdev means that sensors will pause and resume the
> >> stream at the appropriate time even if their drivers don't implement those
> >> operations.
> >>
> >> Signed-off-by: Daniel Scally <djrscally@xxxxxxxxx>
> >> ---
> >> Changes since v3:
> >>
> >> - None
> >>
> >> drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 15 ++++++++++++++-
> >> 1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> >> index 47db0ee0fcbf..7bb86e246ebe 100644
> >> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> >> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> >> @@ -1973,12 +1973,19 @@ static int __maybe_unused cio2_suspend(struct device *dev)
> >> struct pci_dev *pci_dev = to_pci_dev(dev);
> >> struct cio2_device *cio2 = pci_get_drvdata(pci_dev);
> >> struct cio2_queue *q = cio2->cur_queue;
> >> + int r;
> >>
> >> dev_dbg(dev, "cio2 suspend\n");
> >> if (!cio2->streaming)
> >> return 0;
> >>
> >> /* Stop stream */
> >> + r = v4l2_subdev_call(q->sensor, video, s_stream, 0);
> >> + if (r) {
> >> + dev_err(dev, "failed to stop sensor streaming\n");
> >> + return r;
> >> + }
> >> +
> >> cio2_hw_exit(cio2, q);
> >> synchronize_irq(pci_dev->irq);
> >>
> >> @@ -2013,8 +2020,14 @@ static int __maybe_unused cio2_resume(struct device *dev)
> >> }
> >>
> >> r = cio2_hw_init(cio2, q);
> >> - if (r)
> >> + if (r) {
> >> dev_err(dev, "fail to init cio2 hw\n");
> >> + return r;
> >> + }
> >> +
> >> + r = v4l2_subdev_call(q->sensor, video, s_stream, 1);
> >> + if (r)
> > If this fails, do we need to do anything to undo the effects of
> > cio2_hw_init()?
>
> Ah - thank you yes, I think I should be calling cio2_hw_exit() there.
> I'll add that for v5.
> >
> > Should this always call s_stream, 1? Or should the streaming state be
> > stored during cio2_suspend, and that stored state value be set here?
> > I.e. we shouldn't necessarily call s_stream if it wasn't already
> > streaming?
>
>
> I think it's ok as is, on the grounds that in cio2_resume() there's a
> guard to do nothing if the cio2 isn't streaming:
>
>
> if (!cio2->streaming)
>
> return 0;
>
>
> And in cio2_vb2_start_streaming() (which sets that cio2->streaming flag
> true) there's the same call to v4l2_subdev_call() to set s_stream(1) for
> the sensor. So I think the only time we can reach this point, the sensor
> is expected to be streaming.
Ahh, that's fine then, I didn't see beyond the context in the patch.
So with the cleanup on the cio2_hw_exit():
Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>
> >
> >> + dev_err(dev, "fail to start sensor streaming\n");
> >>
> >> return r;
> >> }
> >> --
> >> 2.25.1
> >>
