[libcamera-devel] [PATCH] libcamera: pipeline: ipu3: Stop ImgU and CIO2 on IPA error path

[Kieran Bingham]

Hi Umang, Jacopo,

On 11/01/2021 09:25, Jacopo Mondi wrote:
> Hi Uman,
> 
> On Mon, Jan 11, 2021 at 02:05:00PM +0530, Umang Jain wrote:
>> Hi Jacopo
>>
>> On 1/8/21 11:36 PM, Jacopo Mondi wrote:
>>> Hi Umang,
>>>
>>> On Fri, Jan 08, 2021 at 11:31:13PM +0530, Umang Jain wrote:
>>>> Do not let freeBuffers() run before ImgU and CIO2 are stopped on IPA
>>>> configuration failure path.
>>>>
>>> This is based on Niklas' in-review series, isn't it ?
>>>
>> Right. I realized it now, for some reason, I assumed it's in master :-S
>>>> Signed-off-by: Umang Jain <email at uajain.com>
>>>> Suggested-by: Jacopo Mondi <jacopo at jmondi.org>
>>>> Change-Id: Iadf0c950c24dcd3b6788275e36f2c028fbc53d7b
>>> Ups, not Change-Id please :)
>>>
>>>> ---
>>>>   src/libcamera/pipeline/ipu3/ipu3.cpp | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> index 6cd1879a..3c7f98a9 100644
>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> @@ -694,6 +694,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con
>>>>   	if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) ||
>>>>   	    (result.data.size() != 1) || (result.data.at(0) != 1)) {
>>>>   		LOG(IPU3, Warning) << "Failed to configure IPA";
>>>> +		imgu->stop();
>>>> +		cio2->stop();
>>> I would rather move these two instruction under the error: label and
>>> remove them from the above error paths
>> If we move these two instructions under the error: label :
>>
>> I  spent some time looking into effects of different permutations on error:
>> label, for e.g. if cio2 fails and jumps to error: - it shall imgu->stop()
>> directly (without starting it first). I concluded on the point that stopping
>> Imgu (cio2 for that matter) _directly_ won't be a problem. The stop() in
>> both cases calls to ioctl(VIDIOC_STREAMOFF,..) - which has clear
>> documentation for calling it directly (without having a call to
>> ioctl(VIDIOC_STREAMON, ...)
>> https://www.kernel.org/doc/html/v5.4/media/uapi/v4l/vidioc-streamon.html
>>
>> Does it makes sense?
> 
> Yes, STREAMOFF should be harmeless to be called if STREAMON was not
> called.
> 

I believe that statement is what stopped me from merging:

"libcamera: v4l2_videodevice: Track streaming state"
https://patchwork.libcamera.org/patch/2221/

As I recall it was deemed unnecessary (though has RB tags)


> Otherwise, the error path could be made:
> 
>         ret = cio2->start();
>         if (ret)
>                 return ret;
> 
>         ret = imgu->start();
>         if (ret)
>                 goto error_cio2_stop;
> 
>         ret = ....
>         if (ret)
>                 goto error_imgu_stop;
> 
> error_imgu_stop:
>         imgu->stop();
> error_cio2_stop:
>         cio2->stop();
>         freeBuffers(camera);
>         return ret;
> 
> C++ is sometimes nasty with gotos, I haven't tried compiling this bit.

I would say that if it's safe to call stop() we can just keep it simple
and call stop regardless on all devices.

Otherwise if there is an issue calling stop on a stopped device from
V4L2, we can apply the referenced patch, to track state in the object,
and then we can call stop() regardless ;-)

--
Kieran


> 
>>>
>>> Thanks
>>>    j
>>>
>>>>   		ret = -EINVAL;
>>>>   		goto error;
>>>>   	}
>>>> --
>>>> 2.29.2
>>>>
>>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran