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

[Hirokazu Honda]

Hi Jacopo, thank you for the patch

On Tue, Sep 28, 2021 at 6:36 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Acquisition 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.
>
> The CameraStream::waitFence() implementation is copied from the
> CameraWorker::Worker::waitFence() one, and both should be replaced by a
> libcamera core mechanism.
>
> 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 would like to have a comment that acquireFence -1 is passed in the
case of Internal and reason.
>
>                 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>
>  #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) {

Perhaps ASSERT(type_ == Internal || type_ == Mapped) is useful here?

> +               int ret = waitFence(fence);
> +               ::close(fence);
> +               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);

A function should be put before member variables.

With these nits,
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>

-Hiro
>  };
>
>  #endif /* __ANDROID_CAMERA_STREAM__ */
> --
> 2.32.0
>