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

[Umang Jain]

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?
>
> Thanks
>    j
>
>>   		ret = -EINVAL;
>>   		goto error;
>>   	}
>> --
>> 2.29.2
>>