[libcamera-devel] [PATCH] libcamera: pipeline: ipu3: Stop IPA before stopping devices

[Kieran Bingham]

On 08/03/2021 16:34, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Mon, Mar 08, 2021 at 04:29:15PM +0000, Kieran Bingham wrote:
>> The IPA should be stopped before the hardware devices to ensure that
>> all asynchronous actions have completed within the IPA before resources
>> are removed and released.
> 
> This makes sense,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> I however wonder if we should set a "stopping" flag before calling
> data->ipa_->stop(), to let the handlers for all the asynchronous actions
> invoked during stop() know that we're stopping. This would for instance
> allow us to avoid requeing buffers unnecessarily. Maybe it's not the
> best way to handle the issue, and in any case, if it's needed, it's a
> candidate for a patch on top.
> 
> Does this solve the buffer queue after buffer free bug ?


It solves the buffer queue after device stop bug - but indeed, pulls out
(or leaves visible) a use-after-free on the FrameBufferAllocator with
what seems like a unique pointer to a FrameBuffer being around after the
Allocator releases all the FrameBuffers...

https://paste.debian.net/1188407/



> 
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 2b4d31500533..498f85634a83 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -764,13 +764,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>>  	IPU3CameraData *data = cameraData(camera);
>>  	int ret = 0;
>>  
>> +	data->ipa_->stop();
>> +
>>  	ret |= data->imgu_->stop();
>>  	ret |= data->cio2_.stop();
>>  	if (ret)
>>  		LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
>>  
>> -	data->ipa_->stop();
>> -
>>  	freeBuffers(camera);
>>  }
>>  
> 

-- 
Regards
--
Kieran