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.
>
>> + dev_err(dev, "fail to start sensor streaming\n");
>>
>> return r;
>> }
>> --
>> 2.25.1
>>
