[libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel pending requests correctly

[Kieran Bingham]

Hi Laurent, Hiro,

On 20/04/2021 03:51, Laurent Pinchart wrote:
> Hi Hiro,
> 
> Thank you for the patch.
> 
> On Thu, Apr 08, 2021 at 05:51:01PM +0900, Hirokazu Honda wrote:
>> IPU3CameraData stores requests that are not queued yet to a
>> camera node. They should be reported as cancel upon
>> PipelineHandlerIPU3::stop().
>>
>> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
>> ---
>>  include/libcamera/buffer.h           |  3 +++
>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++++++-
>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
>> index 620f8a66..72c62144 100644
>> --- a/include/libcamera/buffer.h
>> +++ b/include/libcamera/buffer.h
>> @@ -58,6 +58,9 @@ private:
>>  
>>  	friend class V4L2VideoDevice; /* Needed to update metadata_. */
>>  
>> +	/*! HACK! */
>> +	friend class IPU3CameraData; /* Needed to update metadata_. */
> 
> Looks like we need a way to handle this cleanly :-) It may relate to the
> discussion we had earlier, about the rework of the buffer and requests
> state handling. I think a redesign of that mechanism, with a cleaner API
> for completion, would be nice. Making the Request class inherit from
> Extensible, with Request::Private being defined in
> include/libcamera/internal/request.h and offering public functions for
> the pipeline handlers may be part of the solution. The "Private" name in
> the our Extensible design pattern (which implements the p-impl design
> pattern) means private to applications, in the public API, but it can be
> accessed within libcamera.
> 

I have an as-yet-unpublished patch to make Request use Extensible, and I
also have a patch to provide a 'cancel' operation on Buffers.

Perhaps I should send those out ...



> The rest of the patch looks good.
> 
>> +
>>  	std::vector<Plane> planes_;
>>  
>>  	Request *request_;
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 462a0d9b..f89b8f3f 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -66,6 +66,7 @@ public:
>>  	void cio2BufferReady(FrameBuffer *buffer);
>>  	void paramBufferReady(FrameBuffer *buffer);
>>  	void statBufferReady(FrameBuffer *buffer);
>> +	void cancelPendingRequests();
>>  	int queuePendingRequests();
>>  
>>  	CIO2Device cio2_;
>> @@ -768,7 +769,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>>  	IPU3CameraData *data = cameraData(camera);
>>  	int ret = 0;
>>  
>> -	data->pendingRequests_ = {};
>> +	data->cancelPendingRequests();
>>  
>>  	data->ipa_->stop();
>>  
>> @@ -780,6 +781,22 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>>  	freeBuffers(camera);
>>  }
>>  
>> +void IPU3CameraData::cancelPendingRequests()>> +{
>> +	while (!pendingRequests_.empty()) {
>> +		Request *request = pendingRequests_.front();
>> +
>> +		for (auto it : request->buffers()) {
>> +			FrameBuffer *buffer = it.second;
>> +			buffer->metadata_.status = FrameMetadata::FrameCancelled;
>> +			pipe_->completeBuffer(request, buffer);
>> +		}
>> +
>> +		pipe_->completeRequest(request);
>> +		pendingRequests_.pop();
>> +	}
>> +}
>> +
>>  int IPU3CameraData::queuePendingRequests()
>>  {
>>  	while (!pendingRequests_.empty()) {
> 

-- 
Regards
--
Kieran