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

13958 diff mbox series

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@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(-)

Hi Jacopo, thank you for the patch

On Tue, Sep 28, 2021 at 6:36 AM Jacopo Mondi <jacopo@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@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@chromium.org>

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

Hi Jacopo,

Thank you for the patch.

On Mon, Sep 27, 2021 at 11:37:00PM +0200, Jacopo Mondi wrote:
> Acquisition fences for streams of type Mapped generated by

s/Acquisition/Acquire/

> post-processing, are not correctly handled and are currently

s/processing,/processing/

> 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.
> 
> The CameraStream::waitFence() implementation is copied from the
> CameraWorker::Worker::waitFence() one, and both should be replaced by a
> libcamera core mechanism.

I don't think that's correct, the post-processing streams will stay
internal to the HAL, so libcamera won't be able to handle fences for
them. The implementation in this patch is fine, but comments should be
updated to not state it's temporary.

> Signed-off-by: Jacopo Mondi <jacopo@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'd move this just before the switch.

>  
>  		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>

You need string.h for strerror().

>  #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);

I would set

		camera3Buffer.acquire_fence = -1;

here as I think it can simplify the implementation in CameraDevice (and
it's nice in any case to keep the state in sync with that is happening).

> +		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);

Member functions should go above member variables.

>  };
>  
>  #endif /* __ANDROID_CAMERA_STREAM__ */

Hi Laurent,

On Tue, Sep 28, 2021 at 04:03:57AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Sep 27, 2021 at 11:37:00PM +0200, Jacopo Mondi wrote:
> > Acquisition fences for streams of type Mapped generated by
>
> s/Acquisition/Acquire/
>
> > post-processing, are not correctly handled and are currently
>
> s/processing,/processing/
>
> > 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.
> >
> > The CameraStream::waitFence() implementation is copied from the
> > CameraWorker::Worker::waitFence() one, and both should be replaced by a
> > libcamera core mechanism.
>
> I don't think that's correct, the post-processing streams will stay
> internal to the HAL, so libcamera won't be able to handle fences for
> them. The implementation in this patch is fine, but comments should be
> updated to not state it's temporary.
>

Not having clarified yet in my head how the core fence handling
mechanism would look like, I added this more as a todo than anything
else. I'll drop.

> > Signed-off-by: Jacopo Mondi <jacopo@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'd move this just before the switch.
>
> >
> >  		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>
>
> You need string.h for strerror().
>
> >  #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);
>
> I would set
>
> 		camera3Buffer.acquire_fence = -1;
>
> here as I think it can simplify the implementation in CameraDevice (and
> it's nice in any case to keep the state in sync with that is happening).
>

I considered that as well, but I was blocked by the fact the
CameraWorker doesn't do it (yet).

I can record that the Worker should be instrumented to do so with a
todo in the previous patch and move setting the fence to -1 here

> > +		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);
>
> Member functions should go above member variables.

Ups, don't know why I was sure about the opposite.

Thanks
  j

>
> >  };
> >
> >  #endif /* __ANDROID_CAMERA_STREAM__ */
>
> --
> Regards,
>
> Laurent Pinchart

Hi Hiro,

On Tue, Sep 28, 2021 at 09:04:49AM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thank you for the patch
>
> On Tue, Sep 28, 2021 at 6:36 AM Jacopo Mondi <jacopo@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@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?
>

I guess we can have fences set to -1 from the camera service, the
check for -1 here is for this reason, not to validate the stream type.

I can add the assertion, but it's an unrelated change.

> > +               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@chromium.org>
>
> -Hiro
> >  };
> >
> >  #endif /* __ANDROID_CAMERA_STREAM__ */
> > --
> > 2.32.0
> >