[libcamera-devel] [PATCH v3 1/1] android: Wait on fences in CameraStream::process()
Umang Jain
umang.jain at ideasonboard.com
Tue Sep 28 18:49:37 CEST 2021
Hi Jacopo,
Thank you for the patch.
On 9/28/21 5:55 PM, Jacopo Mondi wrote:
> Acquire fences for streams of type Mapped generated by
> post-processing are not correctly handled and are currently
> 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
> passing -1 to the Worker for Internal streams.
>
> Also correct the release_fence handling for failed captures, as the
> framework requires the release fences to be set to the acquire fence
> value if the acquire fence has not been waited on.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> ---
> src/android/camera_device.cpp | 39 +++++++++++++++++++++++----
> src/android/camera_stream.cpp | 50 +++++++++++++++++++++++++++++++++--
> src/android/camera_stream.h | 4 ++-
> 3 files changed, 85 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ab38116800cd..0fe11fe833b0 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -983,9 +983,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>
> /*
> * Inspect the camera stream type, create buffers opportunely
> - * and add them to the Request if required.
> + * and add them to the Request if required. Only acquire fences
> + * for streams of type Direct are handled by the CameraWorker,
> + * while fences for streams of type Internal and Mapped are
> + * handled at post-processing time.
+1 for the comment, makes things clear
> */
> FrameBuffer *buffer = nullptr;
> + int acquireFence = -1;
> switch (cameraStream->type()) {
> case CameraStream::Type::Mapped:
> /*
> @@ -1008,6 +1012,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 +1035,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> }
>
> descriptor.request_->addBuffer(cameraStream->stream(), buffer,
> - camera3Buffer.acquire_fence);
> + acquireFence);
> }
Ok, so by this worker will wait on these fences, but only for Type::Direct
>
> /*
> @@ -1110,7 +1115,22 @@ void CameraDevice::requestComplete(Request *request)
> captureResult.frame_number = descriptor.frameNumber_;
> captureResult.num_output_buffers = descriptor.buffers_.size();
> for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> - buffer.acquire_fence = -1;
> + CameraStream *cameraStream =
> + static_cast<CameraStream *>(buffer.stream->priv);
> +
> + /*
> + * Streams of type Direct have been queued to the
> + * libcamera::Camera and their acquisition fences have
> + * already been waited on by the CameraWorker.
> + *
> + * Acquire fences of streams of type Internal and Mapped
> + * will be handled during post-processing.
> + *
> + * \todo Instrument the CameraWorker to set the acquire
> + * fence to -1 once it has handled it and remove this check.
> + */
> + if (cameraStream->type() == CameraStream::Type::Direct)
> + buffer.acquire_fence = -1;
Yep, for Type::Direct they have waited upon by worker already, so we
can set them to -1
And release_fence for all buffers is to -1 as right below:
> buffer.release_fence = -1;
> buffer.status = CAMERA3_BUFFER_STATUS_OK;
> }
> @@ -1130,8 +1150,17 @@ void CameraDevice::requestComplete(Request *request)
> CAMERA3_MSG_ERROR_REQUEST);
>
> captureResult.partial_result = 0;
> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> + /*
> + * Signal to the framework it has to handle fences that
> + * have not been waited on by setting the release fence
> + * to the acquire fence value.
> + */
> + buffer.release_fence = buffer.acquire_fence;
> + buffer.acquire_fence = -1;
> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> + }
> +
Yep, that's what should be happening on error as per docs
> callbacks_->process_capture_result(callbacks_, &captureResult);
>
> return;
> @@ -1181,7 +1210,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..2363985398fb 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -7,7 +7,11 @@
>
> #include "camera_stream.h"
>
> +#include <errno.h>
> +#include <string.h>
> #include <sys/mman.h>
> +#include <sys/poll.h>
> +#include <unistd.h>
>
> #include <libcamera/formats.h>
>
> @@ -109,11 +113,53 @@ 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);
Ah these are not Type::Direct, but Type::Mapped probably, so we need to
check the fences and wait upon here
> + ::close(fence);
> + camera3Buffer.acquire_fence = -1;
fence is closed (fixing the leak!) and acquire_fence = -1. Make sense.
This is looking quite logical already but I have a question
If I am seeing things correctly, I don't see a
buffer.release_fence = buffer.acquire_fence;
in the post-processing error handling block. Should it be the case on
post-processing failure or I am missing something? My reason is we do
mark buffer as CAMERA3_BUFFER_STATUS_ERROR on post-processing failure,
so maybe we should do the right thing with fences too ? But not sure, I
have limited understanding in this aspect.
I have been testing this already with cts so,
Tested-by: Umang Jain <umang.jain at ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> + if (ret < 0) {
> + LOG(HAL, Error) << "Failed waiting for fence: "
> + << fence << ": " << strerror(-ret);
> + return ret;
> + }
> + }
> +
> if (!postProcessor_)
> return 0;
>
> @@ -122,7 +168,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..03ecfa94fc8c 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -119,13 +119,15 @@ 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();
> void putBuffer(libcamera::FrameBuffer *buffer);
>
> private:
> + int waitFence(int fence);
> +
> CameraDevice *const cameraDevice_;
> const libcamera::CameraConfiguration *config_;
> const Type type_;
More information about the libcamera-devel
mailing list