[libcamera-devel] [PATCH v2 2/2] libcamera: pipelines: ipu3: Simplify error bail out path on start()

[Kieran Bingham]

Hi Umang,

On 12/01/2021 05:38, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Tue, Jan 12, 2021 at 10:21:40AM +0530, Umang Jain wrote:
>> On the bail out path, always ensure that ImgU and CIO2 are stopped
>> before freeing the buffers. V4L2VideoDevice class guarantees that
>> calling stop() without having to call start() is harmless, hence use
> 
> s/having to call/having called/
> 
>> this guarantee to simplify error paths.
>>
>> Signed-off-by: Umang Jain <email at uajain.com>
>> ---
>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index f1151733..73304ea7 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -617,15 +617,14 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con
>>  		goto error;
>>  
>>  	ret = imgu->start();
>> -	if (ret) {
>> -		imgu->stop();
>> -		cio2->stop();
>> +	if (ret)
>>  		goto error;
>> -	}
>>  
>>  	return 0;
>>  
>>  error:
>> +	imgu->stop();
>> +	cio2->stop();
> 
> cio2->stop() also calls V4L2VideoDevice::requestBuffers(), which calls
> VIDIOC_REQBUFS(0). This shouldn't be an issue, but we could possibly
> optimize it. It can be done on top if needed.

I think we can do optimisations later, this series fixes a bug from what
I understand it - so I'll apply with the corrections to the commit log
above.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>>  	freeBuffers(camera);
>>  	LOG(IPU3, Error) << "Failed to start camera " << camera->id();
>>  
> 

-- 
Regards
--
Kieran