[libcamera-devel] [PATCH v3 1/1] android: Wait on fences in CameraStream::process()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 28 22:25:14 CEST 2021
Hello,
On Tue, Sep 28, 2021 at 10:19:37PM +0530, Umang Jain wrote:
> 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
s/Post-pone/Postpone/
> > 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.
We don't need fences for internal streams, right ? Should this be
* while fences for Mapped streams are handled at
* post-processing time. Internal streams do not use fences at
* all.
> +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
s/acquisition/acquire/
> > + * already been waited on by the CameraWorker.
> > + *
> > + * Acquire fences of streams of type Internal and Mapped
> > + * will be handled during post-processing.
Ditto here,
* Acquire fences of Mapped streams 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,
Could we name the parameter camera3Dest, dest, dst or destination ?
Otherwise it's not clear in the implementation of the function which
buffer "camera3Buffer" refers to.
> > 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.
We only need to do that if we haven't waited on the fence. If we wait
but processing then fails, there's no need to move the acquire_fence to
the release_fence.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 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_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list