<div dir="ltr"><div dir="ltr">Hi Paul,<div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jun 4, 2021 at 6:27 PM <<a href="mailto:paul.elder@ideasonboard.com">paul.elder@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Fri, Jun 04, 2021 at 05:48:45PM +0900, Hirokazu Honda wrote:<br>
> Hi Paul and Laurent,<br>
> <br>
> On Wed, Jun 2, 2021 at 1:38 PM <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>> wrote:<br>
> <br>
> Hi Jacopo,<br>
> <br>
> On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:<br>
> > Hi Jacopo,<br>
> ><br>
> > On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:<br>
> > > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:<br>
> > > > On Fri, May 28, 2021 at 12:03:59AM +0200, Jacopo Mondi wrote:<br>
> > > > > Implement the flush() camera operation in the CameraDevice class<br>
> > > > > and make it available to the camera framework by implementing the<br>
> > > > > operation wrapper in camera_ops.cpp.<br>
> > > > ><br>
> > > > > Introduce a new camera state State::Flushing to handle concurrent<br>
> > > > > flush() and process_capture_request() calls.<br>
> > > > ><br>
> > > > > As flush() can race with processCaptureRequest() protect it<br>
> > > > > by introducing a new State::Flushing state that<br>
> > > > > processCaptureRequest() inspects before queuing the Request to the<br>
> > > > > Camera. If flush() is in progress while processCaptureRequest() is<br>
> > > > > called, return the current Request immediately in error state. If<br>
> > > > > flush() has completed and a new call to processCaptureRequest() is<br>
> > > > > made just after, start the camera again before queuing the request.<br>
> > > > ><br>
> > > > > Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> > > > > ---<br>
> > > > > src/android/camera_device.cpp | 74<br>
> ++++++++++++++++++++++++++++++++++-<br>
> > > > > src/android/camera_device.h | 3 ++<br>
> > > > > src/android/camera_ops.cpp | 8 +++-<br>
> > > > > 3 files changed, 83 insertions(+), 2 deletions(-)<br>
> > > > ><br>
> > > > > diff --git a/src/android/camera_device.cpp b/src/android/<br>
> camera_device.cpp<br>
> > > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644<br>
> > > > > --- a/src/android/camera_device.cpp<br>
> > > > > +++ b/src/android/camera_device.cpp<br>
> > > > > @@ -797,6 +797,23 @@ void CameraDevice::close()<br>
> > > > > camera_->release();<br>
> > > > > }<br>
> > > > > <br>
> > > > > +void CameraDevice::flush()<br>
> > > > > +{<br>
> > > > > + {<br>
> > > > > + MutexLocker stateLock(stateMutex_);<br>
> > > > > + if (state_ != State::Running)<br>
> > > > > + return;<br>
> > > > > +<br>
> > > > > + state_ = State::Flushing;<br>
> > > > > + }<br>
> > > > > +<br>
> > > > > + worker_.stop();<br>
> > > > > + camera_->stop();<br>
> > > > > +<br>
> > > > > + MutexLocker stateLock(stateMutex_);<br>
> > > > > + state_ = State::Stopped;<br>
> > > > > +}<br>
> > > > > +<br>
> > > > > void CameraDevice::stop()<br>
> > > > > {<br>
> > > > > MutexLocker stateLock(stateMutex_);<br>
> > > > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls<br>
> (Camera3RequestDescriptor *descriptor)<br>
> > > > > return 0;<br>
> > > > > }<br>
> > > > > <br>
> > > > > +void CameraDevice::abortRequest(camera3_capture_request_t<br>
> *request)<br>
> > > > > +{<br>
> > > > > + notifyError(request->frame_number, nullptr,<br>
> CAMERA3_MSG_ERROR_REQUEST);<br>
> > > > > +<br>
> > > > > + camera3_capture_result_t result = {};<br>
> > > > > + result.num_output_buffers = request->num_output_buffers;<br>
> > > > > + result.frame_number = request->frame_number;<br>
> > > > > + result.partial_result = 0;<br>
> > > > > +<br>
> > > > > + std::vector<camera3_stream_buffer_t> resultBuffers<br>
> (result.num_output_buffers);<br>
> > > > > + for (auto [i, buffer] : utils::enumerate(resultBuffers)) {<br>
> > > > > + buffer = request->output_buffers[i];<br>
> > > > > + buffer.release_fence = request->output_buffers<br>
> [i].acquire_fence;<br>
> > > > > + buffer.acquire_fence = -1;<br>
> > > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;<br>
> > > > > + }<br>
> > > > > + result.output_buffers = resultBuffers.data();<br>
> > > > > +<br>
> > > > > + callbacks_->process_capture_result(callbacks_, &result);<br>
> > > > > +}<br>
> > > > > +<br>
> > > > > int CameraDevice::processCaptureRequest(camera3_capture_request_t<br>
> *camera3Request)<br>
> > > > > {<br>
> > > > > if (!isValidRequest(camera3Request))<br>
> > > > > return -EINVAL;<br>
> > > > > <br>
> > > > > {<br>
> > > > > + /*<br>
> > > > > + * Start the camera if that's the first request we<br>
> handle after<br>
> > > > > + * a configuration or after a flush.<br>
> > > > > + *<br>
> > > > > + * If flush is in progress, return the pending<br>
> request<br>
> > > > > + * immediately in error state.<br>
> > > > > + */<br>
> > > > > MutexLocker stateLock(stateMutex_);<br>
> > > > > + if (state_ == State::Flushing) {<br>
> > > > > + abortRequest(camera3Request);<br>
> > > > > + return 0;<br>
> > > > > + }<br>
> > > > > <br>
> > > > > - /* Start the camera if that's the first request we<br>
> handle. */<br>
> > > > > if (state_ == State::Stopped) {<br>
> > > > > worker_.start();<br>
> > > > > <br>
> > > > > @@ -2004,6 +2052,30 @@ int CameraDevice::processCaptureRequest<br>
> (camera3_capture_request_t *camera3Reques<br>
> > > > > if (ret)<br>
> > > > > return ret;<br>
> > > > > <br>
> > > > > + /*<br>
> > > > > + * Just before queuing the request, make sure flush() has<br>
> not<br>
> > > > > + * been called while this function was running. If flush is<br>
> in progress<br>
> > > > > + * abort the request. If flush has completed and has<br>
> stopped the camera<br>
> > > > > + * we have to re-start it to be able to process the<br>
> request.<br>
> > > > > + */<br>
> > > > > + MutexLocker stateLock(stateMutex_);<br>
> > > > > + if (state_ == State::Flushing) {<br>
> > > > > + abortRequest(camera3Request);<br>
> > > > > + return 0;<br>
> > > > > + }<br>
> > > ><br>
> > > > It seems possibly overkill to do this check twice, but it shouldn't<br>
> > > > hurt. I suspect we'll rework this code later, possibly by adding a<br>
> > > > Camera::flush() in the libcamera native API, although I'm not<br>
> entirely<br>
> > > > sure what benefit it would bring compared to a stop/start. For now<br>
> this<br>
> > > > should be fine.<br>
> > > ><br>
> > > > > +<br>
> > > > > + if (state_ == State::Stopped) {<br>
> > > > > + worker_.start();<br>
> > > > > +<br>
> > > > > + ret = camera_->start();<br>
> > > > > + if (ret) {<br>
> > > > > + LOG(HAL, Error) << "Failed to start<br>
> camera";<br>
> > > > > + return ret;<br>
> > > > > + }<br>
> > > > > +<br>
> > > > > + state_ = State::Running;<br>
> > > > > + }<br>
> > > ><br>
> > > > This, however, bothers me a bit. Why do we need to start the camera<br>
> in<br>
> > > > two different locations ? Could we drop the first start above ? And<br>
> if<br>
> > > > we do so, given that preparing the request should be a short<br>
> operation,<br>
> > > > I wonder if we shouldn't also drop the first Flushing check at the<br>
> top<br>
> > > > of this function.<br>
> > ><br>
> > > This shouldn't be a blocker though, so I'll merge the series after<br>
> > > running tests. We can address the issue on top.<br>
> ><br>
> > I'm afraid this series causes CTS regressions :-(<br>
> ><br>
> > I'm running the full camera tests with<br>
> ><br>
> > run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases<br>
> ><br>
> > With the current master branch, I get 22 or 23 failures (some are a bit<br>
> > random), and with this series, it increases to 25. Here's the diff:<br>
> ><br>
> > @@ -3,14 +3,16 @@<br>
> > android.hardware.camera2.cts.ImageReaderTest#testRawPrivate<br>
> > android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate<br>
> > android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes<br>
> > -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot<br>
> > android.hardware.camera2.cts.RobustnessTest#<br>
> testMandatoryOutputCombinations<br>
> > +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths<br>
> > +android.hardware.camera2.cts.StillCaptureTest#testJpegExif<br>
> > android.hardware.camera2.cts.StillCaptureTest#<br>
> testStillPreviewCombination<br>
> > android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces<br>
> > android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks<br>
> > android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback<br>
> > android.hardware.cts.CameraTest#testFocusDistances<br>
> > android.hardware.cts.CameraTest#testImmediateZoom<br>
> > +android.hardware.cts.CameraTest#testJpegExif<br>
> > android.hardware.cts.CameraTest#testPreviewCallback<br>
> > android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer<br>
> > android.hardware.cts.CameraTest#testPreviewCallbackWithPicture<br>
> <br>
> With the subplan, all of the existing ones are excluded. The<br>
> RecordingTests are failing at random (I'll file a bug report for this).<br>
> <br>
> I've managed to reproduce the three extra failures though, and they<br>
> consistently fail with a segfault.<br>
> <br>
> <br>
> <br>
> I ran android.hardware.camera2.cts.StillCaptureTest and the failure tests pass.<br>
> Could you tell me on top of what commit you applied the patch series?<br>
<br>
On... master. I think 59de56f4 "qcam: Add libatomic dependency".<br>
<br></blockquote><div><br></div><div>I couldn't reproduce on the commit with the flush patch.</div><div>Would you mind sharing the libcamera log with me while running android.hardware.cts.CameraTest#testJpegExif?</div><div> </div><div>Thanks,</div><div>-Hiro</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Paul<br>
<br>
> <br>
> <br>
> <br>
> ><br>
> > There's some randomness in the RecordingTest, so that may not be<br>
> > significant. The other three tests seem to pass consistently in master,<br>
> > and fail consistently with the series applied. They also fail when run<br>
> > in isolation.<br>
> ><br>
> > > > The rest of the patch looks good to me.<br>
> > > ><br>
> > > > > +<br>
> > > > > worker_.queueRequest(descriptor.request_.get());<br>
> > > > > <br>
> > > > > {<br>
> > > > > diff --git a/src/android/camera_device.h b/src/android/<br>
> camera_device.h<br>
> > > > > index c949fa509ca4..4aadb27c562c 100644<br>
> > > > > --- a/src/android/camera_device.h<br>
> > > > > +++ b/src/android/camera_device.h<br>
> > > > > @@ -43,6 +43,7 @@ public:<br>
> > > > > <br>
> > > > > int open(const hw_module_t *hardwareModule);<br>
> > > > > void close();<br>
> > > > > + void flush();<br>
> > > > > <br>
> > > > > unsigned int id() const { return id_; }<br>
> > > > > camera3_device_t *camera3Device() { return &camera3Device_;<br>
> }<br>
> > > > > @@ -92,6 +93,7 @@ private:<br>
> > > > > <br>
> > > > > enum class State {<br>
> > > > > Stopped,<br>
> > > > > + Flushing,<br>
> > > > > Running,<br>
> > > > > };<br>
> > > > > <br>
> > > > > @@ -106,6 +108,7 @@ private:<br>
> > > > > getRawResolutions(const libcamera::PixelFormat &<br>
> pixelFormat);<br>
> > > > > <br>
> > > > > libcamera::FrameBuffer *createFrameBuffer(const<br>
> buffer_handle_t camera3buffer);<br>
> > > > > + void abortRequest(camera3_capture_request_t *request);<br>
> > > > > void notifyShutter(uint32_t frameNumber, uint64_t<br>
> timestamp);<br>
> > > > > void notifyError(uint32_t frameNumber, camera3_stream_t<br>
> *stream,<br>
> > > > > camera3_error_msg_code code);<br>
> > > > > diff --git a/src/android/camera_ops.cpp b/src/android/<br>
> camera_ops.cpp<br>
> > > > > index 696e80436821..8a3cfa175ff5 100644<br>
> > > > > --- a/src/android/camera_ops.cpp<br>
> > > > > +++ b/src/android/camera_ops.cpp<br>
> > > > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const<br>
> struct camera3_device *dev,<br>
> > > > > {<br>
> > > > > }<br>
> > > > > <br>
> > > > > -static int hal_dev_flush([[maybe_unused]] const struct<br>
> camera3_device *dev)<br>
> > > > > +static int hal_dev_flush(const struct camera3_device *dev)<br>
> > > > > {<br>
> > > > > + if (!dev)<br>
> > > > > + return -EINVAL;<br>
> > > > > +<br>
> > > > > + CameraDevice *camera = reinterpret_cast<CameraDevice *><br>
> (dev->priv);<br>
> > > > > + camera->flush();<br>
> > > > > +<br>
> > > > > return 0;<br>
> > > > > }<br>
> > > > > <br>
> ><br>
> > --<br>
> > Regards,<br>
> ><br>
> > Laurent Pinchart<br>
> <br>
</blockquote></div></div>