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

[Laurent Pinchart]

Hi Kieran,

On Tue, Apr 20, 2021 at 11:48:16AM +0100, Kieran Bingham wrote:
> On 20/04/2021 03:51, Laurent Pinchart wrote:
> > 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 ...

That would be a good way to move the discussion forward.

> > 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,

Laurent Pinchart