[libcamera-devel,v5,8/8] android: Implement flush() camera operation
diff mbox series

Message ID 20210608151633.73465-9-jacopo@jmondi.org
State Superseded
Delegated to: Jacopo Mondi
Headers show
Series
  • Implement flush() camera operation
Related show

Commit Message

Jacopo Mondi June 8, 2021, 3:16 p.m. UTC 
Implement the flush() camera operation in the CameraDevice class
and make it available to the camera framework by implementing the
operation wrapper in camera_ops.cpp.

Introduce a new camera state State::Flushing to handle concurrent
flush() and process_capture_request() calls.

As flush() can race with processCaptureRequest() protect it
by introducing a new State::Flushing state that
processCaptureRequest() inspects before queuing the Request to the
Camera. If flush() is in progress while processCaptureRequest() is
called, return the current Request immediately in error state. If
flush() has completed and a new call to processCaptureRequest() is
made just after, start the camera again before queuing the request.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/android/camera_device.cpp | 77 +++++++++++++++++++++++++++--------
 src/android/camera_device.h   |  3 ++
 src/android/camera_ops.cpp    |  8 +++-
 3 files changed, 71 insertions(+), 17 deletions(-)

Comments

Laurent Pinchart June 8, 2021, 8:42 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Jun 08, 2021 at 05:16:33PM +0200, Jacopo Mondi wrote:
> Implement the flush() camera operation in the CameraDevice class
> and make it available to the camera framework by implementing the
> operation wrapper in camera_ops.cpp.
> 
> Introduce a new camera state State::Flushing to handle concurrent
> flush() and process_capture_request() calls.
> 
> As flush() can race with processCaptureRequest() protect it
> by introducing a new State::Flushing state that
> processCaptureRequest() inspects before queuing the Request to the
> Camera. If flush() is in progress while processCaptureRequest() is
> called, return the current Request immediately in error state. If
> flush() has completed and a new call to processCaptureRequest() is
> made just after, start the camera again before queuing the request.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/android/camera_device.cpp | 77 +++++++++++++++++++++++++++--------
>  src/android/camera_device.h   |  3 ++
>  src/android/camera_ops.cpp    |  8 +++-
>  3 files changed, 71 insertions(+), 17 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b29c1edc9970..3adb657bfeca 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -798,6 +798,23 @@ void CameraDevice::close()
>  	camera_->release();
>  }
>  
> +void CameraDevice::flush()
> +{
> +	{
> +		MutexLocker stateLock(stateMutex_);
> +		if (state_ != State::Running)
> +			return;
> +
> +		state_ = State::Flushing;
> +	}
> +
> +	worker_.stop();
> +	camera_->stop();
> +
> +	MutexLocker stateLock(stateMutex_);
> +	state_ = State::Stopped;
> +}
> +
>  void CameraDevice::stop()
>  {
>  	MutexLocker stateLock(stateMutex_);
> @@ -1896,27 +1913,31 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>  	return 0;
>  }
>  
> -int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> +void CameraDevice::abortRequest(camera3_capture_request_t *request)
>  {
> -	if (!isValidRequest(camera3Request))
> -		return -EINVAL;
> +	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>  
> -	{
> -		MutexLocker stateLock(stateMutex_);
> +	camera3_capture_result_t result = {};
> +	result.num_output_buffers = request->num_output_buffers;
> +	result.frame_number = request->frame_number;
> +	result.partial_result = 0;
>  
> -		/* Start the camera if that's the first request we handle. */
> -		if (state_ == State::Stopped) {
> -			worker_.start();
> +	std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers);
> +	for (auto [i, buffer] : utils::enumerate(resultBuffers)) {
> +		buffer = request->output_buffers[i];
> +		buffer.release_fence = request->output_buffers[i].acquire_fence;
> +		buffer.acquire_fence = -1;
> +		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +	}
> +	result.output_buffers = resultBuffers.data();
>  
> -			int ret = camera_->start();
> -			if (ret) {
> -				LOG(HAL, Error) << "Failed to start camera";
> -				return ret;
> -			}
> +	callbacks_->process_capture_result(callbacks_, &result);
> +}
>  
> -			state_ = State::Running;
> -		}
> -	}
> +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> +{
> +	if (!isValidRequest(camera3Request))
> +		return -EINVAL;
>  
>  	/*
>  	 * Save the request descriptors for use at completion time.
> @@ -2006,6 +2027,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Just before queuing the request, make sure flush() has not
> +	 * been called while this function was running. If flush is in progress
> +	 * abort the request. If flush has completed and has stopped the camera
> +	 * we have to re-start it to be able to process the request.
> +	 */
> +	MutexLocker stateLock(stateMutex_);

I'd add a blank line here.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	if (state_ == State::Flushing) {
> +		abortRequest(camera3Request);
> +		return 0;
> +	}
> +
> +	if (state_ == State::Stopped) {
> +		worker_.start();
> +
> +		ret = camera_->start();
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to start camera";
> +			return ret;
> +		}
> +
> +		state_ = State::Running;
> +	}
> +
>  	worker_.queueRequest(descriptor.request_.get());
>  
>  	{
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index c949fa509ca4..4aadb27c562c 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -43,6 +43,7 @@ public:
>  
>  	int open(const hw_module_t *hardwareModule);
>  	void close();
> +	void flush();
>  
>  	unsigned int id() const { return id_; }
>  	camera3_device_t *camera3Device() { return &camera3Device_; }
> @@ -92,6 +93,7 @@ private:
>  
>  	enum class State {
>  		Stopped,
> +		Flushing,
>  		Running,
>  	};
>  
> @@ -106,6 +108,7 @@ private:
>  	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
>  
>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> +	void abortRequest(camera3_capture_request_t *request);
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>  			 camera3_error_msg_code code);
> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
> index 696e80436821..8a3cfa175ff5 100644
> --- a/src/android/camera_ops.cpp
> +++ b/src/android/camera_ops.cpp
> @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,
>  {
>  }
>  
> -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)
> +static int hal_dev_flush(const struct camera3_device *dev)
>  {
> +	if (!dev)
> +		return -EINVAL;
> +
> +	CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);
> +	camera->flush();
> +
>  	return 0;
>  }
>
Jacopo Mondi June 10, 2021, 4:21 p.m. UTC | #2 
Hi Me and Laurent

On Tue, Jun 08, 2021 at 11:42:48PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jun 08, 2021 at 05:16:33PM +0200, Jacopo Mondi wrote:
> > Implement the flush() camera operation in the CameraDevice class
> > and make it available to the camera framework by implementing the
> > operation wrapper in camera_ops.cpp.
> >
> > Introduce a new camera state State::Flushing to handle concurrent
> > flush() and process_capture_request() calls.
> >
> > As flush() can race with processCaptureRequest() protect it
> > by introducing a new State::Flushing state that
> > processCaptureRequest() inspects before queuing the Request to the
> > Camera. If flush() is in progress while processCaptureRequest() is
> > called, return the current Request immediately in error state. If
> > flush() has completed and a new call to processCaptureRequest() is
> > made just after, start the camera again before queuing the request.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/android/camera_device.cpp | 77 +++++++++++++++++++++++++++--------
> >  src/android/camera_device.h   |  3 ++
> >  src/android/camera_ops.cpp    |  8 +++-
> >  3 files changed, 71 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index b29c1edc9970..3adb657bfeca 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -798,6 +798,23 @@ void CameraDevice::close()
> >  	camera_->release();
> >  }
> >
> > +void CameraDevice::flush()
> > +{
> > +	{
> > +		MutexLocker stateLock(stateMutex_);
> > +		if (state_ != State::Running)
> > +			return;
> > +
> > +		state_ = State::Flushing;
> > +	}
> > +
> > +	worker_.stop();
> > +	camera_->stop();
> > +
> > +	MutexLocker stateLock(stateMutex_);
> > +	state_ = State::Stopped;
> > +}
> > +
> >  void CameraDevice::stop()
> >  {
> >  	MutexLocker stateLock(stateMutex_);
> > @@ -1896,27 +1913,31 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> >  	return 0;
> >  }
> >
> > -int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> > +void CameraDevice::abortRequest(camera3_capture_request_t *request)
> >  {
> > -	if (!isValidRequest(camera3Request))
> > -		return -EINVAL;
> > +	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> >
> > -	{
> > -		MutexLocker stateLock(stateMutex_);
> > +	camera3_capture_result_t result = {};
> > +	result.num_output_buffers = request->num_output_buffers;
> > +	result.frame_number = request->frame_number;
> > +	result.partial_result = 0;
> >
> > -		/* Start the camera if that's the first request we handle. */
> > -		if (state_ == State::Stopped) {
> > -			worker_.start();
> > +	std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers);
> > +	for (auto [i, buffer] : utils::enumerate(resultBuffers)) {
> > +		buffer = request->output_buffers[i];
> > +		buffer.release_fence = request->output_buffers[i].acquire_fence;
> > +		buffer.acquire_fence = -1;
> > +		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +	}
> > +	result.output_buffers = resultBuffers.data();
> >
> > -			int ret = camera_->start();
> > -			if (ret) {
> > -				LOG(HAL, Error) << "Failed to start camera";
> > -				return ret;
> > -			}
> > +	callbacks_->process_capture_result(callbacks_, &result);
> > +}
> >
> > -			state_ = State::Running;
> > -		}
> > -	}
> > +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> > +{
> > +	if (!isValidRequest(camera3Request))
> > +		return -EINVAL;
> >
> >  	/*
> >  	 * Save the request descriptors for use at completion time.
> > @@ -2006,6 +2027,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  	if (ret)
> >  		return ret;
> >
> > +	/*
> > +	 * Just before queuing the request, make sure flush() has not
> > +	 * been called while this function was running. If flush is in progress
> > +	 * abort the request. If flush has completed and has stopped the camera
> > +	 * we have to re-start it to be able to process the request.
> > +	 */
> > +	MutexLocker stateLock(stateMutex_);
>
> I'd add a blank line here.

I would also take the occasion to re-phrase the above comment to

+        * If flush is in progress abort the request. If the camera has been
+        * stopped we have to re-start it to be able to process the request.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +	if (state_ == State::Flushing) {
> > +		abortRequest(camera3Request);
> > +		return 0;
> > +	}
> > +
> > +	if (state_ == State::Stopped) {
> > +		worker_.start();
> > +
> > +		ret = camera_->start();
> > +		if (ret) {

And stop the worker_ if we're returning here

With those I presume I can push the series

Thanks
  j

> > +			LOG(HAL, Error) << "Failed to start camera";
> > +			return ret;
> > +		}
> > +
> > +		state_ = State::Running;
> > +	}
> > +
> >  	worker_.queueRequest(descriptor.request_.get());
> >
> >  	{
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index c949fa509ca4..4aadb27c562c 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -43,6 +43,7 @@ public:
> >
> >  	int open(const hw_module_t *hardwareModule);
> >  	void close();
> > +	void flush();
> >
> >  	unsigned int id() const { return id_; }
> >  	camera3_device_t *camera3Device() { return &camera3Device_; }
> > @@ -92,6 +93,7 @@ private:
> >
> >  	enum class State {
> >  		Stopped,
> > +		Flushing,
> >  		Running,
> >  	};
> >
> > @@ -106,6 +108,7 @@ private:
> >  	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
> >
> >  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> > +	void abortRequest(camera3_capture_request_t *request);
> >  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> >  			 camera3_error_msg_code code);
> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
> > index 696e80436821..8a3cfa175ff5 100644
> > --- a/src/android/camera_ops.cpp
> > +++ b/src/android/camera_ops.cpp
> > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,
> >  {
> >  }
> >
> > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)
> > +static int hal_dev_flush(const struct camera3_device *dev)
> >  {
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);
> > +	camera->flush();
> > +
> >  	return 0;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index b29c1edc9970..3adb657bfeca 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -798,6 +798,23 @@  void CameraDevice::close()
 	camera_->release();
 }
 
+void CameraDevice::flush()
+{
+	{
+		MutexLocker stateLock(stateMutex_);
+		if (state_ != State::Running)
+			return;
+
+		state_ = State::Flushing;
+	}
+
+	worker_.stop();
+	camera_->stop();
+
+	MutexLocker stateLock(stateMutex_);
+	state_ = State::Stopped;
+}
+
 void CameraDevice::stop()
 {
 	MutexLocker stateLock(stateMutex_);
@@ -1896,27 +1913,31 @@  int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
 	return 0;
 }
 
-int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
+void CameraDevice::abortRequest(camera3_capture_request_t *request)
 {
-	if (!isValidRequest(camera3Request))
-		return -EINVAL;
+	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
 
-	{
-		MutexLocker stateLock(stateMutex_);
+	camera3_capture_result_t result = {};
+	result.num_output_buffers = request->num_output_buffers;
+	result.frame_number = request->frame_number;
+	result.partial_result = 0;
 
-		/* Start the camera if that's the first request we handle. */
-		if (state_ == State::Stopped) {
-			worker_.start();
+	std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers);
+	for (auto [i, buffer] : utils::enumerate(resultBuffers)) {
+		buffer = request->output_buffers[i];
+		buffer.release_fence = request->output_buffers[i].acquire_fence;
+		buffer.acquire_fence = -1;
+		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
+	}
+	result.output_buffers = resultBuffers.data();
 
-			int ret = camera_->start();
-			if (ret) {
-				LOG(HAL, Error) << "Failed to start camera";
-				return ret;
-			}
+	callbacks_->process_capture_result(callbacks_, &result);
+}
 
-			state_ = State::Running;
-		}
-	}
+int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
+{
+	if (!isValidRequest(camera3Request))
+		return -EINVAL;
 
 	/*
 	 * Save the request descriptors for use at completion time.
@@ -2006,6 +2027,30 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	if (ret)
 		return ret;
 
+	/*
+	 * Just before queuing the request, make sure flush() has not
+	 * been called while this function was running. If flush is in progress
+	 * abort the request. If flush has completed and has stopped the camera
+	 * we have to re-start it to be able to process the request.
+	 */
+	MutexLocker stateLock(stateMutex_);
+	if (state_ == State::Flushing) {
+		abortRequest(camera3Request);
+		return 0;
+	}
+
+	if (state_ == State::Stopped) {
+		worker_.start();
+
+		ret = camera_->start();
+		if (ret) {
+			LOG(HAL, Error) << "Failed to start camera";
+			return ret;
+		}
+
+		state_ = State::Running;
+	}
+
 	worker_.queueRequest(descriptor.request_.get());
 
 	{
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index c949fa509ca4..4aadb27c562c 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -43,6 +43,7 @@  public:
 
 	int open(const hw_module_t *hardwareModule);
 	void close();
+	void flush();
 
 	unsigned int id() const { return id_; }
 	camera3_device_t *camera3Device() { return &camera3Device_; }
@@ -92,6 +93,7 @@  private:
 
 	enum class State {
 		Stopped,
+		Flushing,
 		Running,
 	};
 
@@ -106,6 +108,7 @@  private:
 	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
 
 	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
+	void abortRequest(camera3_capture_request_t *request);
 	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
 	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
 			 camera3_error_msg_code code);
diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
index 696e80436821..8a3cfa175ff5 100644
--- a/src/android/camera_ops.cpp
+++ b/src/android/camera_ops.cpp
@@ -66,8 +66,14 @@  static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,
 {
 }
 
-static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)
+static int hal_dev_flush(const struct camera3_device *dev)
 {
+	if (!dev)
+		return -EINVAL;
+
+	CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);
+	camera->flush();
+
 	return 0;
 }