[libcamera-devel] [PATCH v3 1/1] android: Wait on fences in CameraStream::process()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 29 14:18:04 CEST 2021
Hi Jacopo,
On Wed, Sep 29, 2021 at 01:59:54PM +0200, Jacopo Mondi wrote:
> On Tue, Sep 28, 2021 at 11:25:14PM +0300, Laurent Pinchart wrote:
> > 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.
>
> Internal streams are created to support generating through
> post-processing a format the libcamera::Camera cannot produce, the
> most notable (and only?) example is JPEG.
>
> The destination buffer where the post-processor writes to might be
> associated with an acquire_fence as well in my understanding.
>
> Have I missed something ?
As far as I understand, the destination buffers for internal streams are
allocated internally, where would a fence come from ?
> > > +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.
>
> According to the badly enforced naming scheme in use in the HAL,
> variable prefixed with camera3 are meant to represent object provided
> by the camera service to the HAL. I can change this though, also
> because it was the name originally in use.
>
> > > > 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
>
> Mapped or Internal
>
> > > > + ::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.
>
> Yes, and we close the fence unconditionally, so we should not ask the
> camera service to close it for us.
>
> > 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>
>
> Thank you both
>
> > > > + 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