[libcamera-devel,1/2] android: wait on fences in CameraStream::process()

13929 diff mbox series

Acquisition fences for streams generated by post-processing are not
correctly handled and are currently ignored by the camera HAL.

Add a waitFence() function to the CameraStream class to be executed before
starting the post-processing in CameraStream::process().

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@jmondi.org>
---
 src/android/camera_device.cpp |  2 +-
 src/android/camera_stream.cpp | 47 ++++++++++++++++++++++++++++++++++-
 src/android/camera_stream.h   |  4 ++-
 3 files changed, 50 insertions(+), 3 deletions(-)

Hi Jacopo, thank you for the patch.

On Sat, Sep 25, 2021 at 2:01 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Acquisition fences for streams generated by post-processing are not
> correctly handled and are currently ignored by the camera HAL.
>

If I understand correctly, the problem happens with the stream whose
type() is CameraStream::Type::Mapped?
The acquire_fence is added to descriptor_.request_, so they are waited
and closed CameraWorker.
Am I wrong?
Then, it seems to be wrong to wait acquire_fence in process() because
it waits and closes the fence for Type::Internal.

-Hiro


> Add a waitFence() function to the CameraStream class to be executed before
> starting the post-processing in CameraStream::process().
>
> 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@jmondi.org>
> ---
>  src/android/camera_device.cpp |  2 +-
>  src/android/camera_stream.cpp | 47 ++++++++++++++++++++++++++++++++++-
>  src/android/camera_stream.h   |  4 ++-
>  3 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 21844e5114a9..db35947afc2f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1179,7 +1179,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..c723da2c6407 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) {
> +               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,6 +166,7 @@ int CameraStream::process(const FrameBuffer &source,
>          * separate thread.
>          */
>         const StreamConfiguration &output = configuration();
> +       buffer_handle_t &camera3Dest = *camera3Buffer.buffer;
>         CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
>                           PROT_READ | PROT_WRITE);
>         if (!dest.isValid()) {
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 2dab6c3a37d1..1566e5e4009c 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();
> @@ -132,6 +132,8 @@ private:
>         camera3_stream_t *camera3Stream_;
>         const unsigned int index_;
>
> +       int waitFence(int fence);
> +
>         std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
>         std::vector<libcamera::FrameBuffer *> buffers_;
>         /*
> --
> 2.32.0
>

Hi Hiro,

On Mon, Sep 27, 2021 at 02:19:29PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thank you for the patch.
>
> On Sat, Sep 25, 2021 at 2:01 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Acquisition fences for streams generated by post-processing are not
> > correctly handled and are currently ignored by the camera HAL.
> >
>
> If I understand correctly, the problem happens with the stream whose
> type() is CameraStream::Type::Mapped?
> The acquire_fence is added to descriptor_.request_, so they are waited
> and closed CameraWorker.
> Am I wrong?

Acquire fences for streams of type ::Mapped are not added to the
descriptor_ and are not handled by the CameraWorker

	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {

                ....

		switch (cameraStream->type()) {
		case CameraStream::Type::Mapped:
			/*
			 * Mapped streams don't need buffers added to the
			 * Request.
			 */
			LOG(HAL, Debug) << ss.str() << " (mapped)";
			continue; <------------------- THIS

		case CameraStream::Type::Direct:
                        ...
		}

                ...


		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
						camera3Buffer.acquire_fence);
	}



> Then, it seems to be wrong to wait acquire_fence in process() because
> it waits and closes the fence for Type::Internal.

If I got your comment right, we have a problem for Type::Internal, as
their acquisition fence are waited on by the CameraWorker, but the
post-processing phase waits on them too.

It doesn't seems to create issues as I've tested it with multiple CTS
runs but it indeed it's not correct. I wonder why it is not
problematic, giving poll() a closed file descriptor should trigger the
PULLHUP event flag. As we don't check for that flag, we might miss it
and happly proceed maybe.

So yes, we should make sure ::Internal are waited on once only, and I
would do so at post-processing time before actually writing data to
the destination buffer. I think to do so, it woul be enough to pass -1
as acquire fence to descriptor.request_->addBuffer() for ::Internal
streams.

Did I get your comment right ?

>
> -Hiro
>
>
> > Add a waitFence() function to the CameraStream class to be executed before
> > starting the post-processing in CameraStream::process().
> >
> > 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@jmondi.org>
> > ---
> >  src/android/camera_device.cpp |  2 +-
> >  src/android/camera_stream.cpp | 47 ++++++++++++++++++++++++++++++++++-
> >  src/android/camera_stream.h   |  4 ++-
> >  3 files changed, 50 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 21844e5114a9..db35947afc2f 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1179,7 +1179,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..c723da2c6407 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) {
> > +               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,6 +166,7 @@ int CameraStream::process(const FrameBuffer &source,
> >          * separate thread.
> >          */
> >         const StreamConfiguration &output = configuration();
> > +       buffer_handle_t &camera3Dest = *camera3Buffer.buffer;
> >         CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
> >                           PROT_READ | PROT_WRITE);
> >         if (!dest.isValid()) {
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index 2dab6c3a37d1..1566e5e4009c 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();
> > @@ -132,6 +132,8 @@ private:
> >         camera3_stream_t *camera3Stream_;
> >         const unsigned int index_;
> >
> > +       int waitFence(int fence);
> > +
> >         std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> >         std::vector<libcamera::FrameBuffer *> buffers_;
> >         /*
> > --
> > 2.32.0
> >

Hi Jacopo,

On Mon, Sep 27, 2021 at 7:27 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Hiro,
>
> On Mon, Sep 27, 2021 at 02:19:29PM +0900, Hirokazu Honda wrote:
> > Hi Jacopo, thank you for the patch.
> >
> > On Sat, Sep 25, 2021 at 2:01 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Acquisition fences for streams generated by post-processing are not
> > > correctly handled and are currently ignored by the camera HAL.
> > >
> >
> > If I understand correctly, the problem happens with the stream whose
> > type() is CameraStream::Type::Mapped?
> > The acquire_fence is added to descriptor_.request_, so they are waited
> > and closed CameraWorker.
> > Am I wrong?
>
> Acquire fences for streams of type ::Mapped are not added to the
> descriptor_ and are not handled by the CameraWorker
>
>         for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
>
>                 ....
>
>                 switch (cameraStream->type()) {
>                 case CameraStream::Type::Mapped:
>                         /*
>                          * Mapped streams don't need buffers added to the
>                          * Request.
>                          */
>                         LOG(HAL, Debug) << ss.str() << " (mapped)";
>                         continue; <------------------- THIS
>
>                 case CameraStream::Type::Direct:
>                         ...
>                 }
>
>                 ...
>
>
>                 descriptor.request_->addBuffer(cameraStream->stream(), buffer,
>                                                 camera3Buffer.acquire_fence);
>         }
>
>
>
> > Then, it seems to be wrong to wait acquire_fence in process() because
> > it waits and closes the fence for Type::Internal.
>
> If I got your comment right, we have a problem for Type::Internal, as
> their acquisition fence are waited on by the CameraWorker, but the
> post-processing phase waits on them too.
>

Thanks. Sorry I wrongly wrote Mapped in the first sentence thinking of Internal.

> It doesn't seems to create issues as I've tested it with multiple CTS
> runs but it indeed it's not correct. I wonder why it is not
> problematic, giving poll() a closed file descriptor should trigger the
> PULLHUP event flag. As we don't check for that flag, we might miss it
> and happly proceed maybe.
>
> So yes, we should make sure ::Internal are waited on once only, and I
> would do so at post-processing time before actually writing data to
> the destination buffer. I think to do so, it woul be enough to pass -1
> as acquire fence to descriptor.request_->addBuffer() for ::Internal
> streams.
>
> Did I get your comment right ?
>

It sounds good to me.

-Hiro

> >
> > -Hiro
> >
> >
> > > Add a waitFence() function to the CameraStream class to be executed before
> > > starting the post-processing in CameraStream::process().
> > >
> > > 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@jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp |  2 +-
> > >  src/android/camera_stream.cpp | 47 ++++++++++++++++++++++++++++++++++-
> > >  src/android/camera_stream.h   |  4 ++-
> > >  3 files changed, 50 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 21844e5114a9..db35947afc2f 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -1179,7 +1179,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..c723da2c6407 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) {
> > > +               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,6 +166,7 @@ int CameraStream::process(const FrameBuffer &source,
> > >          * separate thread.
> > >          */
> > >         const StreamConfiguration &output = configuration();
> > > +       buffer_handle_t &camera3Dest = *camera3Buffer.buffer;
> > >         CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
> > >                           PROT_READ | PROT_WRITE);
> > >         if (!dest.isValid()) {
> > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > index 2dab6c3a37d1..1566e5e4009c 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();
> > > @@ -132,6 +132,8 @@ private:
> > >         camera3_stream_t *camera3Stream_;
> > >         const unsigned int index_;
> > >
> > > +       int waitFence(int fence);
> > > +
> > >         std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> > >         std::vector<libcamera::FrameBuffer *> buffers_;
> > >         /*
> > > --
> > > 2.32.0
> > >