[libcamera-devel] [PATCH] pipeline: raspberrypi: Simplify RPiCameraData::clearIncompleteRequests()

[Kieran Bingham]

Hi Naush,

On 03/06/2021 13:43, Naushir Patuck wrote:
> With the addition of FrameBuffer::cancel(), the logic to clear and return
> pending requests can be simplified by not having to queue all the request
> buffers to the device before calling streamOff().
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 46 ++++---------------
>  1 file changed, 8 insertions(+), 38 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index a65b4568256c..887f8d0f7404 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera)
>  	/* Disable SOF event generation. */
>  	data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);
>  
> -	/* This also stops the streams. */
> +	for (auto const stream : data->streams_)
> +		stream->dev()->streamOff();

That's nicer than hiding it in clearIncompleteRequests...


> +
>  	data->clearIncompleteRequests();
>  	data->bayerQueue_ = {};
>  	data->embeddedQueue_ = {};
> @@ -1477,49 +1479,17 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  
>  void RPiCameraData::clearIncompleteRequests()
>  {
> -	/*
> -	 * Queue up any buffers passed in the request.
> -	 * This is needed because streamOff() will then mark the buffers as
> -	 * cancelled.
> -	 */
> -	for (auto const request : requestQueue_) {
> -		for (auto const stream : streams_) {
> -			if (!stream->isExternal())
> -				continue;
> -
> -			FrameBuffer *buffer = request->findBuffer(stream);
> -			if (buffer)
> -				stream->queueBuffer(buffer);
> -		}
> -	}
> -
> -	/* Stop all streams. */
> -	for (auto const stream : streams_)
> -		stream->dev()->streamOff();
> -
>  	/*
>  	 * All outstanding requests (and associated buffers) must be returned
> -	 * back to the pipeline. The buffers would have been marked as
> -	 * cancelled by the call to streamOff() earlier.
> +	 * back to the pipeline.
>  	 */
>  	while (!requestQueue_.empty()) {
>  		Request *request = requestQueue_.front();
> -		/*
> -		 * A request could be partially complete,
> -		 * i.e. we have returned some buffers, but still waiting
> -		 * for others or waiting for metadata.
> -		 */
> -		for (auto const stream : streams_) {
> -			if (!stream->isExternal())
> -				continue;
>  
> -			FrameBuffer *buffer = request->findBuffer(stream);
> -			/*
> -			 * Has the buffer already been handed back to the
> -			 * request? If not, do so now.
> -			 */
> -			if (buffer && buffer->request())
> -				pipe_->completeBuffer(request, buffer);
> +		for (auto &b : request->buffers()) {
> +			FrameBuffer *buffer = b.second;
> +			buffer->cancel();
> +			pipe_->completeBuffer(request, buffer);

Looks good to me.
As far as I can tell, items on the requestQueue haven't been handed to
any hardware yet, is that correct? (Which means there's no risk of any
other action occurring from a stream on these buffers).

And in fact, all streams are called with streamOff() before this anyway
- so it should be good.

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


>  		}
>  
>  		pipe_->completeRequest(request);
> 

-- 
Regards
--
Kieran