[libcamera-devel,2/9] libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling
diff mbox series

Message ID 20200713084727.232422-3-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Zero-copy RAW stream work
Related show

Commit Message

Naushir Patuck July 13, 2020, 8:47 a.m. UTC 
The IPA now signals up front how many frames it wants the pipeline
handler to drop. This makes it easier to handle up-coming changes to the
buffer handling for import/export buffers.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.h           |  2 +-
 src/ipa/raspberrypi/raspberrypi.cpp           | 21 ++++++------
 .../pipeline/raspberrypi/raspberrypi.cpp      | 33 +++++++++++--------
 3 files changed, 32 insertions(+), 24 deletions(-)

Comments

Niklas Söderlund July 13, 2020, 11:30 a.m. UTC | #1 
Hi Naushir,

Thanks for your work.

On 2020-07-13 09:47:21 +0100, Naushir Patuck wrote:
> The IPA now signals up front how many frames it wants the pipeline
> handler to drop. This makes it easier to handle up-coming changes to the
> buffer handling for import/export buffers.

I like this patch. I think it makes the IPA interface easier :-) As you 
say however it depends on pending work and may need to be refreshed to 
fit that.

> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h           |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 21 ++++++------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 33 +++++++++++--------
>  3 files changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index a18ce9a8..2dab2b00 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -15,8 +15,8 @@ enum RPiOperations {
>  	RPI_IPA_ACTION_V4L2_SET_ISP,
>  	RPI_IPA_ACTION_STATS_METADATA_COMPLETE,
>  	RPI_IPA_ACTION_RUN_ISP,
> -	RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME,
>  	RPI_IPA_ACTION_SET_SENSOR_CONFIG,
> +	RPI_IPA_ACTION_SET_DROP_FRAME_COUNT,
>  	RPI_IPA_ACTION_EMBEDDED_COMPLETE,
>  	RPI_IPA_EVENT_SIGNAL_STAT_READY,
>  	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index b1f27861..f3568882 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -64,8 +64,8 @@ class IPARPi : public IPAInterface
>  public:
>  	IPARPi()
>  		: lastMode_({}), controller_(), controllerInit_(false),
> -		  frame_count_(0), check_count_(0), hide_count_(0),
> -		  mistrust_count_(0), lsTableHandle_(0), lsTable_(nullptr)
> +		  frame_count_(0), check_count_(0), mistrust_count_(0),
> +		  lsTableHandle_(0), lsTable_(nullptr)
>  	{
>  	}
>  
> @@ -132,8 +132,6 @@ private:
>  	uint64_t frame_count_;
>  	/* For checking the sequencing of Prepare/Process calls. */
>  	uint64_t check_count_;
> -	/* How many frames the pipeline handler should hide, or "drop". */
> -	unsigned int hide_count_;
>  	/* How many frames we should avoid running control algos on. */
>  	unsigned int mistrust_count_;
>  	/* LS table allocation passed in from the pipeline handler. */
> @@ -241,14 +239,19 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	 */
>  	frame_count_ = 0;
>  	check_count_ = 0;
> +	int drop_frame = 0;
>  	if (controllerInit_) {
> -		hide_count_ = helper_->HideFramesModeSwitch();
> +		drop_frame = helper_->HideFramesModeSwitch();
>  		mistrust_count_ = helper_->MistrustFramesModeSwitch();
>  	} else {
> -		hide_count_ = helper_->HideFramesStartup();
> +		drop_frame = helper_->HideFramesStartup();
>  		mistrust_count_ = helper_->MistrustFramesStartup();
>  	}
>  
> +	IPAOperationData op;
> +	op.operation = RPI_IPA_ACTION_SET_DROP_FRAME_COUNT;
> +	op.data.push_back(drop_frame);
> +
>  	struct AgcStatus agcStatus;
>  	/* These zero values mean not program anything (unless overwritten). */
>  	agcStatus.shutter_time = 0.0;
> @@ -339,13 +342,11 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  		 * they are "unreliable".
>  		 */
>  		prepareISP(embeddedbufferId);
> +		frame_count_++;
>  
>  		/* Ready to push the input buffer into the ISP. */
>  		IPAOperationData op;
> -		if (++frame_count_ > hide_count_)
> -			op.operation = RPI_IPA_ACTION_RUN_ISP;
> -		else
> -			op.operation = RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME;
> +		op.operation = RPI_IPA_ACTION_RUN_ISP;
>  		op.data = { bayerbufferId & RPiIpaMask::ID };
>  		queueFrameAction.emit(0, op);
>  		break;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 7bae72f9..8b6f578f 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -131,7 +131,7 @@ class RPiCameraData : public CameraData
>  public:
>  	RPiCameraData(PipelineHandler *pipe)
>  		: CameraData(pipe), sensor_(nullptr), lsTable_(nullptr),
> -		  state_(State::Stopped), dropFrame_(false), ispOutputCount_(0)
> +		  state_(State::Stopped), dropFrameCount_(0), ispOutputCount_(0)
>  	{
>  	}
>  
> @@ -168,8 +168,8 @@ public:
>  
>  	CameraSensor *sensor_;
>  	/* Array of Unicam and ISP device streams and associated buffers/streams. */
> -	RPiDevice<Unicam, 2> unicam_;
> -	RPiDevice<Isp, 4> isp_;
> +	RPi::RPiDevice<Unicam, 2> unicam_;
> +	RPi::RPiDevice<Isp, 4> isp_;
>  	/* The vector below is just for convenience when iterating over all streams. */
>  	std::vector<RPi::RPiStream *> streams_;
>  	/* Buffers passed to the IPA. */
> @@ -194,14 +194,15 @@ public:
>  	std::queue<FrameBuffer *> embeddedQueue_;
>  	std::deque<Request *> requestQueue_;
>  
> +	unsigned int dropFrameCount_;
> +
>  private:
>  	void checkRequestCompleted();
>  	void tryRunPipeline();
>  	void tryFlushQueues();
>  	FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
>  
> -	bool dropFrame_;
> -	int ispOutputCount_;
> +	unsigned int ispOutputCount_;
>  };
>  
>  class RPiCameraConfiguration : public CameraConfiguration
> @@ -1058,6 +1059,11 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>  		goto done;
>  	}
>  
> +	case RPI_IPA_ACTION_SET_DROP_FRAME_COUNT: {
> +		dropFrameCount_ = action.data[0];
> +		goto done;
> +	}
> +
>  	case RPI_IPA_ACTION_V4L2_SET_ISP: {
>  		ControlList controls = action.controls[0];
>  		isp_[Isp::Input].dev()->setControls(&controls);
> @@ -1091,7 +1097,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>  		break;
>  	}
>  
> -	case RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME:
>  	case RPI_IPA_ACTION_RUN_ISP: {
>  		unsigned int bufferId = action.data[0];
>  		FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> @@ -1100,7 +1105,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>  				<< ", timestamp: " << buffer->metadata().timestamp;
>  
>  		isp_[Isp::Input].dev()->queueBuffer(buffer);
> -		dropFrame_ = (action.operation == RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME) ? true : false;
>  		ispOutputCount_ = 0;
>  		break;
>  	}
> @@ -1266,7 +1270,7 @@ void RPiCameraData::clearIncompleteRequests()
>  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream)
>  {
>  	if (stream->isExternal()) {
> -		if (!dropFrame_) {
> +		if (!dropFrameCount_) {
>  			Request *request = buffer->request();
>  			pipe_->completeBuffer(camera_, request, buffer);
>  		}
> @@ -1278,7 +1282,7 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream
>  		 * simply memcpy to the Request buffer and requeue back to the
>  		 * device.
>  		 */
> -		if (stream == &unicam_[Unicam::Image] && !dropFrame_) {
> +		if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
>  			const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
>  			Request *request = requestQueue_.front();
>  			FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> @@ -1323,7 +1327,7 @@ void RPiCameraData::checkRequestCompleted()
>  	 * If we are dropping this frame, do not touch the request, simply
>  	 * change the state to IDLE when ready.
>  	 */
> -	if (!dropFrame_) {
> +	if (!dropFrameCount_) {
>  		Request *request = requestQueue_.front();
>  		if (request->hasPendingBuffers())
>  			return;
> @@ -1342,10 +1346,13 @@ void RPiCameraData::checkRequestCompleted()
>  	 * frame.
>  	 */
>  	if (state_ == State::IpaComplete &&
> -	    ((ispOutputCount_ == 3 && dropFrame_) || requestCompleted)) {
> +	    ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) {
>  		state_ = State::Idle;
> -		if (dropFrame_)
> -			LOG(RPI, Info) << "Dropping frame at the request of the IPA";
> +		if (dropFrameCount_) {
> +			dropFrameCount_--;
> +			LOG(RPI, Info) << "Dropping frame at the request of the IPA ( "
> +				       << dropFrameCount_ << " left)";
> +		}
>  	}
>  }
>  
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index a18ce9a8..2dab2b00 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -15,8 +15,8 @@  enum RPiOperations {
 	RPI_IPA_ACTION_V4L2_SET_ISP,
 	RPI_IPA_ACTION_STATS_METADATA_COMPLETE,
 	RPI_IPA_ACTION_RUN_ISP,
-	RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME,
 	RPI_IPA_ACTION_SET_SENSOR_CONFIG,
+	RPI_IPA_ACTION_SET_DROP_FRAME_COUNT,
 	RPI_IPA_ACTION_EMBEDDED_COMPLETE,
 	RPI_IPA_EVENT_SIGNAL_STAT_READY,
 	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index b1f27861..f3568882 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -64,8 +64,8 @@  class IPARPi : public IPAInterface
 public:
 	IPARPi()
 		: lastMode_({}), controller_(), controllerInit_(false),
-		  frame_count_(0), check_count_(0), hide_count_(0),
-		  mistrust_count_(0), lsTableHandle_(0), lsTable_(nullptr)
+		  frame_count_(0), check_count_(0), mistrust_count_(0),
+		  lsTableHandle_(0), lsTable_(nullptr)
 	{
 	}
 
@@ -132,8 +132,6 @@  private:
 	uint64_t frame_count_;
 	/* For checking the sequencing of Prepare/Process calls. */
 	uint64_t check_count_;
-	/* How many frames the pipeline handler should hide, or "drop". */
-	unsigned int hide_count_;
 	/* How many frames we should avoid running control algos on. */
 	unsigned int mistrust_count_;
 	/* LS table allocation passed in from the pipeline handler. */
@@ -241,14 +239,19 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 	 */
 	frame_count_ = 0;
 	check_count_ = 0;
+	int drop_frame = 0;
 	if (controllerInit_) {
-		hide_count_ = helper_->HideFramesModeSwitch();
+		drop_frame = helper_->HideFramesModeSwitch();
 		mistrust_count_ = helper_->MistrustFramesModeSwitch();
 	} else {
-		hide_count_ = helper_->HideFramesStartup();
+		drop_frame = helper_->HideFramesStartup();
 		mistrust_count_ = helper_->MistrustFramesStartup();
 	}
 
+	IPAOperationData op;
+	op.operation = RPI_IPA_ACTION_SET_DROP_FRAME_COUNT;
+	op.data.push_back(drop_frame);
+
 	struct AgcStatus agcStatus;
 	/* These zero values mean not program anything (unless overwritten). */
 	agcStatus.shutter_time = 0.0;
@@ -339,13 +342,11 @@  void IPARPi::processEvent(const IPAOperationData &event)
 		 * they are "unreliable".
 		 */
 		prepareISP(embeddedbufferId);
+		frame_count_++;
 
 		/* Ready to push the input buffer into the ISP. */
 		IPAOperationData op;
-		if (++frame_count_ > hide_count_)
-			op.operation = RPI_IPA_ACTION_RUN_ISP;
-		else
-			op.operation = RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME;
+		op.operation = RPI_IPA_ACTION_RUN_ISP;
 		op.data = { bayerbufferId & RPiIpaMask::ID };
 		queueFrameAction.emit(0, op);
 		break;
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 7bae72f9..8b6f578f 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -131,7 +131,7 @@  class RPiCameraData : public CameraData
 public:
 	RPiCameraData(PipelineHandler *pipe)
 		: CameraData(pipe), sensor_(nullptr), lsTable_(nullptr),
-		  state_(State::Stopped), dropFrame_(false), ispOutputCount_(0)
+		  state_(State::Stopped), dropFrameCount_(0), ispOutputCount_(0)
 	{
 	}
 
@@ -168,8 +168,8 @@  public:
 
 	CameraSensor *sensor_;
 	/* Array of Unicam and ISP device streams and associated buffers/streams. */
-	RPiDevice<Unicam, 2> unicam_;
-	RPiDevice<Isp, 4> isp_;
+	RPi::RPiDevice<Unicam, 2> unicam_;
+	RPi::RPiDevice<Isp, 4> isp_;
 	/* The vector below is just for convenience when iterating over all streams. */
 	std::vector<RPi::RPiStream *> streams_;
 	/* Buffers passed to the IPA. */
@@ -194,14 +194,15 @@  public:
 	std::queue<FrameBuffer *> embeddedQueue_;
 	std::deque<Request *> requestQueue_;
 
+	unsigned int dropFrameCount_;
+
 private:
 	void checkRequestCompleted();
 	void tryRunPipeline();
 	void tryFlushQueues();
 	FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
 
-	bool dropFrame_;
-	int ispOutputCount_;
+	unsigned int ispOutputCount_;
 };
 
 class RPiCameraConfiguration : public CameraConfiguration
@@ -1058,6 +1059,11 @@  void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
 		goto done;
 	}
 
+	case RPI_IPA_ACTION_SET_DROP_FRAME_COUNT: {
+		dropFrameCount_ = action.data[0];
+		goto done;
+	}
+
 	case RPI_IPA_ACTION_V4L2_SET_ISP: {
 		ControlList controls = action.controls[0];
 		isp_[Isp::Input].dev()->setControls(&controls);
@@ -1091,7 +1097,6 @@  void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
 		break;
 	}
 
-	case RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME:
 	case RPI_IPA_ACTION_RUN_ISP: {
 		unsigned int bufferId = action.data[0];
 		FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
@@ -1100,7 +1105,6 @@  void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
 				<< ", timestamp: " << buffer->metadata().timestamp;
 
 		isp_[Isp::Input].dev()->queueBuffer(buffer);
-		dropFrame_ = (action.operation == RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME) ? true : false;
 		ispOutputCount_ = 0;
 		break;
 	}
@@ -1266,7 +1270,7 @@  void RPiCameraData::clearIncompleteRequests()
 void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream)
 {
 	if (stream->isExternal()) {
-		if (!dropFrame_) {
+		if (!dropFrameCount_) {
 			Request *request = buffer->request();
 			pipe_->completeBuffer(camera_, request, buffer);
 		}
@@ -1278,7 +1282,7 @@  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream
 		 * simply memcpy to the Request buffer and requeue back to the
 		 * device.
 		 */
-		if (stream == &unicam_[Unicam::Image] && !dropFrame_) {
+		if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
 			const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
 			Request *request = requestQueue_.front();
 			FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
@@ -1323,7 +1327,7 @@  void RPiCameraData::checkRequestCompleted()
 	 * If we are dropping this frame, do not touch the request, simply
 	 * change the state to IDLE when ready.
 	 */
-	if (!dropFrame_) {
+	if (!dropFrameCount_) {
 		Request *request = requestQueue_.front();
 		if (request->hasPendingBuffers())
 			return;
@@ -1342,10 +1346,13 @@  void RPiCameraData::checkRequestCompleted()
 	 * frame.
 	 */
 	if (state_ == State::IpaComplete &&
-	    ((ispOutputCount_ == 3 && dropFrame_) || requestCompleted)) {
+	    ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) {
 		state_ = State::Idle;
-		if (dropFrame_)
-			LOG(RPI, Info) << "Dropping frame at the request of the IPA";
+		if (dropFrameCount_) {
+			dropFrameCount_--;
+			LOG(RPI, Info) << "Dropping frame at the request of the IPA ( "
+				       << dropFrameCount_ << " left)";
+		}
 	}
 }