[libcamera-devel] [PATCH v2 2/2] android: Wait on fences in CameraStream::process()

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Sep 27, 2021 at 11:37:00PM +0200, Jacopo Mondi wrote:
> Acquisition fences for streams of type Mapped generated by

s/Acquisition/Acquire/

> post-processing, are not correctly handled and are currently

s/processing,/processing/

> ignored by the camera HAL.
> 
> Fix this by adding CameraStream::waitFence(), executed before
> starting the post-processing in CameraStream::process().
> 
> The change applies to all streams generated by post-processing (Mapped
> and Internal) but currently acquire fences of Internal streams are
> handled by the camera worker. Post-pone that to post-processing time by

s/Post-pone/Postpone/

> passing -1 to the worker for Internal streams.
> 
> The CameraStream::waitFence() implementation is copied from the
> CameraWorker::Worker::waitFence() one, and both should be replaced by a
> libcamera core mechanism.

I don't think that's correct, the post-processing streams will stay
internal to the HAL, so libcamera won't be able to handle fences for
them. The implementation in this patch is fine, but comments should be
updated to not state it's temporary.

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp |  6 +++--
>  src/android/camera_stream.cpp | 48 +++++++++++++++++++++++++++++++++--
>  src/android/camera_stream.h   |  4 ++-
>  3 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 3c9609d74402..8b0caf2bda7d 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -973,6 +973,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
>  		camera3_stream *camera3Stream = camera3Buffer.stream;
>  		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> +		int acquireFence = -1;

I'd move this just before the switch.

>  
>  		std::stringstream ss;
>  		ss << i << " - (" << camera3Stream->width << "x"
> @@ -1008,6 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  						  cameraStream->configuration().size));
>  
>  			buffer = descriptor.frameBuffers_.back().get();
> +			acquireFence = camera3Buffer.acquire_fence;
>  			LOG(HAL, Debug) << ss.str() << " (direct)";
>  			break;
>  
> @@ -1030,7 +1032,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		}
>  
>  		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
> -						camera3Buffer.acquire_fence);
> +					       acquireFence);
>  	}
>  
>  	/*
> @@ -1186,7 +1188,7 @@ void CameraDevice::requestComplete(Request *request)
>  			continue;
>  		}
>  
> -		int ret = cameraStream->process(*src, *buffer.buffer,
> +		int ret = cameraStream->process(*src, buffer,
>  						descriptor.settings_,
>  						resultMetadata.get());
>  		/*
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index e30c7ee4fcb3..40dcb361749f 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -7,7 +7,10 @@
>  
>  #include "camera_stream.h"
>  
> +#include <errno.h>

You need string.h for strerror().

>  #include <sys/mman.h>
> +#include <sys/poll.h>
> +#include <unistd.h>
>  
>  #include <libcamera/formats.h>
>  
> @@ -109,11 +112,52 @@ int CameraStream::configure()
>  	return 0;
>  }
>  
> +int CameraStream::waitFence(int fence)
> +{
> +	/*
> +	 * \todo The implementation here is copied from camera_worker.cpp
> +	 * and both should be removed once libcamera is instrumented to handle
> +	 * fences waiting in the core.
> +	 *
> +	 * \todo Better characterize the timeout. Currently equal to the one
> +	 * used by the Rockchip Camera HAL on ChromeOS.
> +	 */
> +	constexpr unsigned int timeoutMs = 300;
> +	struct pollfd fds = { fence, POLLIN, 0 };
> +
> +	do {
> +		int ret = poll(&fds, 1, timeoutMs);
> +		if (ret == 0)
> +			return -ETIME;
> +
> +		if (ret > 0) {
> +			if (fds.revents & (POLLERR | POLLNVAL))
> +				return -EINVAL;
> +
> +			return 0;
> +		}
> +	} while (errno == EINTR || errno == EAGAIN);
> +
> +	return -errno;
> +}
> +
>  int CameraStream::process(const FrameBuffer &source,
> -			  buffer_handle_t camera3Dest,
> +			  camera3_stream_buffer_t &camera3Buffer,
>  			  const CameraMetadata &requestMetadata,
>  			  CameraMetadata *resultMetadata)
>  {
> +	/* Handle waiting on fences on the destination buffer. */
> +	int fence = camera3Buffer.acquire_fence;
> +	if (fence != -1) {
> +		int ret = waitFence(fence);
> +		::close(fence);

I would set

		camera3Buffer.acquire_fence = -1;

here as I think it can simplify the implementation in CameraDevice (and
it's nice in any case to keep the state in sync with that is happening).

> +		if (ret < 0) {
> +			LOG(HAL, Error) << "Failed waiting for fence: "
> +					<< fence << ": " << strerror(-ret);
> +			return ret;
> +		}
> +	}
> +
>  	if (!postProcessor_)
>  		return 0;
>  
> @@ -122,7 +166,7 @@ int CameraStream::process(const FrameBuffer &source,
>  	 * separate thread.
>  	 */
>  	const StreamConfiguration &output = configuration();
> -	CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
> +	CameraBuffer dest(*camera3Buffer.buffer, formats::MJPEG, output.size,
>  			  PROT_READ | PROT_WRITE);
>  	if (!dest.isValid()) {
>  		LOG(HAL, Error) << "Failed to map android blob buffer";
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 2dab6c3a37d1..454a33dcdaaa 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -119,7 +119,7 @@ public:
>  
>  	int configure();
>  	int process(const libcamera::FrameBuffer &source,
> -		    buffer_handle_t camera3Dest,
> +		    camera3_stream_buffer_t &camera3Buffer,
>  		    const CameraMetadata &requestMetadata,
>  		    CameraMetadata *resultMetadata);
>  	libcamera::FrameBuffer *getBuffer();
> @@ -140,6 +140,8 @@ private:
>  	 */
>  	std::unique_ptr<std::mutex> mutex_;
>  	std::unique_ptr<PostProcessor> postProcessor_;
> +
> +	int waitFence(int fence);

Member functions should go above member variables.

>  };
>  
>  #endif /* __ANDROID_CAMERA_STREAM__ */

-- 
Regards,

Laurent Pinchart