| Message ID | 20210928122515.28034-2-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. 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 > 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@jmondi.org> > Reviewed-by: Hirokazu Honda <hiroh@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. +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 > + * already been waited on by the CameraWorker. > + * > + * Acquire fences of streams of type Internal and Mapped > + * 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, > 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 > + ::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. I have been testing this already with cts so, Tested-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > + 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_;
Hello, 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@jmondi.org> > > Reviewed-by: Hirokazu Honda <hiroh@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. > +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. > > 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 > > > + ::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. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > I have been testing this already with cts so, > > Tested-by: Umang Jain <umang.jain@ideasonboard.com> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > + 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_;
Hi Laurent, Umang, On Tue, Sep 28, 2021 at 11:25:14PM +0300, Laurent Pinchart wrote: > Hello, > > 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@jmondi.org> > > > Reviewed-by: Hirokazu Honda <hiroh@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 ? > > +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@ideasonboard.com> > > > I have been testing this already with cts so, > > > > Tested-by: Umang Jain <umang.jain@ideasonboard.com> > > Reviewed-by: Umang Jain <umang.jain@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
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@jmondi.org> > > > > Reviewed-by: Hirokazu Honda <hiroh@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@ideasonboard.com> > > > > > I have been testing this already with cts so, > > > > > > Tested-by: Umang Jain <umang.jain@ideasonboard.com> > > > Reviewed-by: Umang Jain <umang.jain@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_;
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. */ 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); } /* @@ -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 + * already been waited on by the CameraWorker. + * + * Acquire fences of streams of type Internal and Mapped + * 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; 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; + } + 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, 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); + camera3Buffer.acquire_fence = -1; + 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_;