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

[Jacopo Mondi]

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