[libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler: Handle fences

[Umang Jain]

Hi Jacopo,

Thank you for the patch

On 10/28/21 4:45 PM, Jacopo Mondi wrote:
> Before queueing a request to the device, any synchronization fence from
> the Request framebuffers has to be waited on.
>
> Start by moving the fences to the Request, in order to reset the ones
> in the FrameBuffers and then enable all of them one by one.
>
> When a fence completes, make sure all fences in the Request have been
> waited on or have expired and only after that actually queue the Request
> to the device.


Here it's said:

     waited on OR expired  => queue the Request to device

>
> If any fence in the Request has expired cancel the request and do not
> queue it to the device.


And here it's said:

     If fence expired => Cancel the request and do not queue.

Am I missing something?

The patch looks good to me overall.

>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>   include/libcamera/internal/pipeline_handler.h |  2 +
>   src/libcamera/pipeline_handler.cpp            | 78 ++++++++++++++++++-
>   2 files changed, 77 insertions(+), 3 deletions(-)
>
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 5c3c0bc5a8b3..5b98011ac4f6 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -29,6 +29,7 @@ class CameraConfiguration;
>   class CameraManager;
>   class DeviceEnumerator;
>   class DeviceMatch;
> +class Fence;
>   class FrameBuffer;
>   class MediaDevice;
>   class PipelineHandler;
> @@ -79,6 +80,7 @@ private:
>   	void mediaDeviceDisconnected(MediaDevice *media);
>   	virtual void disconnect();
>   
> +	void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence);
>   	void doQueueRequest();
>   
>   	std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 0f9fec1b618f..39cb680e5386 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -18,6 +18,7 @@
>   #include <libcamera/framebuffer.h>
>   
>   #include "libcamera/internal/camera.h"
> +#include "libcamera/internal/framebuffer.h"
>   #include "libcamera/internal/device_enumerator.h"
>   #include "libcamera/internal/media_device.h"
>   #include "libcamera/internal/request.h"
> @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request)
>   {
>   	LIBCAMERA_TRACEPOINT(request_queue, request);
>   
> +	Request::Private *data = request->_d();
> +
>   	{
>   		MutexLocker lock(waitingRequestsMutex_);
>   		waitingRequests_.push_back(request);
>   	}
>   
> +	for (FrameBuffer *buffer : request->pending_) {
> +		if (buffer->fence() == -1)
> +			continue;
> +
> +		/*
> +		 * Move the Fence into the Request::Private list of
> +		 * fences. This resets the file descriptor fence in the
> +		 * FrameBuffer to -1.
> +		 */
> +		data->addFence(std::move(buffer->_d()->fence()));
> +
> +		Fence *fence = &data->fences().back();
> +		fence->complete.connect(this,
> +					[this, request, buffer, fence]() {
> +						fenceCompleted(request, buffer, fence);
> +					});
> +	}
> +
> +	/* If no fences to wait on, we can queue the request immediately. */
> +	if (!data->pendingFences()) {
> +		doQueueRequest();
> +
> +		return;
> +	}
> +
> +	/*
> +	 * Now that we have added all fences, enable them one by one.
> +	 * Enabling fences while adding them to the Request would race on the
> +	 * number of pending fences.
> +	 */
> +	for (Fence &fence : data->fences())
> +		fence.enable();
> +}
> +
> +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer,
> +				     Fence *fence)
> +{
> +	Request::Private *data = request->_d();
> +
> +	if (fence->expired()) {
> +		/*
> +		 * Move the fence back to the framebuffer, so that it doesn't
> +		 * get closed and the file descriptor number is made
> +		 * available again to applications.
> +		 */
> +		FrameBuffer::Private *bufferData = buffer->_d();
> +		bufferData->fence() = std::move(*fence);
> +
> +		data->fenceExpired();
> +	} else {
> +		data->fenceCompleted();
> +	}
> +
> +	if (data->pendingFences())
> +		return;
> +
> +	data->clearFences();
>   	doQueueRequest();
>   }
>   
> @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest()
>   			return;
>   
>   		request = waitingRequests_.front();
> +		if (request->_d()->pendingFences())
> +			return;
> +
>   		waitingRequests_.pop_front();
>   	}
>   
> -	/* Queue Request to the pipeline handler. */
>   	Camera *camera = request->_d()->camera();
>   	Camera::Private *camData = camera->_d();
>   
>   	request->sequence_ = camData->requestSequence_++;
> -	camData->queuedRequests_.push_back(request);
>   
> +	/* Cancel the request if one of the fences has failed. */
> +	if (request->_d()->expiredFences()) {
> +		request->cancel();
> +		completeRequest(request);
> +
> +		doQueueRequest();


A bit-off to read doQueueRequest() in this codepath here, but okay I guess

> +
> +		return;
> +	}
> +
> +	/* Queue Request to the pipeline handler. */
> +	camData->queuedRequests_.push_back(request);
>   	int ret = queueRequestDevice(camera, request);
>   	if (ret) {
>   		request->cancel();
>   		completeRequest(request);
>   	}
>   
> -	/* Try to queue the next Request in the queue, if ready. */
>   	doQueueRequest();
>   }
>