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

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

Commit Message

Naushir Patuck July 20, 2020, 9:13 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>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/ipa/raspberrypi.h           |  2 +-
 src/ipa/raspberrypi/raspberrypi.cpp           | 20 +++++------
 .../pipeline/raspberrypi/raspberrypi.cpp      | 36 +++++++++++--------
 3 files changed, 33 insertions(+), 25 deletions(-)

Comments

Kieran Bingham July 21, 2020, 10:59 a.m. UTC | #1 
Hi Naush,

On 20/07/2020 10:13, 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.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Looks good to me

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  include/libcamera/ipa/raspberrypi.h           |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 20 +++++------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 +++++++++++--------
>  3 files changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index a4937769..0f31b6ea 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -14,6 +14,7 @@ enum RPiConfigParameters {
>  	RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
>  	RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
>  	RPI_IPA_CONFIG_SENSOR = (1 << 2),
> +	RPI_IPA_CONFIG_DROP_FRAMES = (1 << 3),
>  };
>  
>  enum RPiOperations {
> @@ -21,7 +22,6 @@ 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_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 7bd04880..2d9fb9d8 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -65,8 +65,8 @@ class IPARPi : public IPAInterface
>  public:
>  	IPARPi()
>  		: lastMode_({}), controller_(), controllerInit_(false),
> -		  frame_count_(0), check_count_(0), hide_count_(0),
> -		  mistrust_count_(0), lsTable_(nullptr)
> +		  frame_count_(0), check_count_(0), mistrust_count_(0),
> +		  lsTableHandle_(0), lsTable_(nullptr)
>  	{
>  	}
>  
> @@ -137,8 +137,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. */
> @@ -242,14 +240,18 @@ 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();
>  	}
>  
> +	result->data.push_back(drop_frame);
> +	result->operation |= RPI_IPA_CONFIG_DROP_FRAMES;
> +
>  	struct AgcStatus agcStatus;
>  	/* These zero values mean not program anything (unless overwritten). */
>  	agcStatus.shutter_time = 0.0;
> @@ -366,13 +368,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;

The bit field flag here looks better to me indeed!

>  		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 6630ef57..d3639ce1 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -133,7 +133,7 @@ class RPiCameraData : public CameraData
>  public:
>  	RPiCameraData(PipelineHandler *pipe)
>  		: CameraData(pipe), sensor_(nullptr), state_(State::Stopped),
> -		  dropFrame_(false), ispOutputCount_(0)
> +		  dropFrameCount_(0), ispOutputCount_(0)
>  	{
>  	}
>  
> @@ -181,14 +181,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
> @@ -999,6 +1000,7 @@ int RPiCameraData::configureIPA()
>  	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
>  			&result);
>  
> +	unsigned int resultIdx = 0;
>  	if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
>  		/*
>  		 * Setup our staggered control writer with the sensor default
> @@ -1006,9 +1008,9 @@ int RPiCameraData::configureIPA()
>  		 */
>  		if (!staggeredCtrl_) {
>  			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> -					    { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> -					      { V4L2_CID_EXPOSURE, result.data[1] } });
> -			sensorMetadata_ = result.data[2];
> +					    { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> +					      { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
> +			sensorMetadata_ = result.data[resultIdx++];
>  		}
>  
>  		/* Configure the H/V flip controls based on the sensor rotation. */
> @@ -1025,6 +1027,11 @@ int RPiCameraData::configureIPA()
>  			LOG(RPI, Error) << "V4L2 staggered set failed";
>  	}
>  
> +	if (result.operation & RPI_IPA_CONFIG_DROP_FRAMES) {
> +		/* Configure the number of dropped frames required on startup. */
> +		dropFrameCount_ = result.data[resultIdx++];

I assume given this is about dropping frames on startup, it only occurs
once, and dropFrameCount_ is already zero at this point, so we're not
losing a previous request to drop frames for instance.




> +	}
> +
>  	return 0;
>  }
>  
> @@ -1075,7 +1082,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();
> @@ -1084,7 +1090,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;
>  	}
> @@ -1250,7 +1255,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);
>  		}
> @@ -1262,7 +1267,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));
> @@ -1307,7 +1312,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;
> @@ -1326,10 +1331,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)";
> +		}
>  	}
>  }
>  
>
Naushir Patuck July 22, 2020, 7:53 a.m. UTC | #2 
Hi Kieran,

Thank you for your review.

On Tue, 21 Jul 2020 at 11:59, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On 20/07/2020 10:13, 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.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
> Looks good to me
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > ---
> >  include/libcamera/ipa/raspberrypi.h           |  2 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 20 +++++------
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 +++++++++++--------
> >  3 files changed, 33 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index a4937769..0f31b6ea 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -14,6 +14,7 @@ enum RPiConfigParameters {
> >       RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> >       RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> >       RPI_IPA_CONFIG_SENSOR = (1 << 2),
> > +     RPI_IPA_CONFIG_DROP_FRAMES = (1 << 3),
> >  };
> >
> >  enum RPiOperations {
> > @@ -21,7 +22,6 @@ 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_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 7bd04880..2d9fb9d8 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -65,8 +65,8 @@ class IPARPi : public IPAInterface
> >  public:
> >       IPARPi()
> >               : lastMode_({}), controller_(), controllerInit_(false),
> > -               frame_count_(0), check_count_(0), hide_count_(0),
> > -               mistrust_count_(0), lsTable_(nullptr)
> > +               frame_count_(0), check_count_(0), mistrust_count_(0),
> > +               lsTableHandle_(0), lsTable_(nullptr)
> >       {
> >       }
> >
> > @@ -137,8 +137,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. */
> > @@ -242,14 +240,18 @@ 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();
> >       }
> >
> > +     result->data.push_back(drop_frame);
> > +     result->operation |= RPI_IPA_CONFIG_DROP_FRAMES;
> > +
> >       struct AgcStatus agcStatus;
> >       /* These zero values mean not program anything (unless overwritten). */
> >       agcStatus.shutter_time = 0.0;
> > @@ -366,13 +368,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;
>
> The bit field flag here looks better to me indeed!
>
> >               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 6630ef57..d3639ce1 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -133,7 +133,7 @@ class RPiCameraData : public CameraData
> >  public:
> >       RPiCameraData(PipelineHandler *pipe)
> >               : CameraData(pipe), sensor_(nullptr), state_(State::Stopped),
> > -               dropFrame_(false), ispOutputCount_(0)
> > +               dropFrameCount_(0), ispOutputCount_(0)
> >       {
> >       }
> >
> > @@ -181,14 +181,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
> > @@ -999,6 +1000,7 @@ int RPiCameraData::configureIPA()
> >       ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> >                       &result);
> >
> > +     unsigned int resultIdx = 0;
> >       if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
> >               /*
> >                * Setup our staggered control writer with the sensor default
> > @@ -1006,9 +1008,9 @@ int RPiCameraData::configureIPA()
> >                */
> >               if (!staggeredCtrl_) {
> >                       staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> > -                                         { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> > -                                           { V4L2_CID_EXPOSURE, result.data[1] } });
> > -                     sensorMetadata_ = result.data[2];
> > +                                         { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> > +                                           { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
> > +                     sensorMetadata_ = result.data[resultIdx++];
> >               }
> >
> >               /* Configure the H/V flip controls based on the sensor rotation. */
> > @@ -1025,6 +1027,11 @@ int RPiCameraData::configureIPA()
> >                       LOG(RPI, Error) << "V4L2 staggered set failed";
> >       }
> >
> > +     if (result.operation & RPI_IPA_CONFIG_DROP_FRAMES) {
> > +             /* Configure the number of dropped frames required on startup. */
> > +             dropFrameCount_ = result.data[resultIdx++];
>
> I assume given this is about dropping frames on startup, it only occurs
> once, and dropFrameCount_ is already zero at this point, so we're not
> losing a previous request to drop frames for instance.

That's correct.  We now only signal how many frames to drop at
startup, and this signal will not make us lose a previous drop frame
request.

>
>
>
>
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -1075,7 +1082,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();
> > @@ -1084,7 +1090,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;
> >       }
> > @@ -1250,7 +1255,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);
> >               }
> > @@ -1262,7 +1267,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));
> > @@ -1307,7 +1312,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;
> > @@ -1326,10 +1331,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)";
> > +             }
> >       }
> >  }
> >
> >
>
> --
> Regards
> --
> Kieran
Laurent Pinchart July 22, 2020, 2:27 p.m. UTC | #3 
Hi Naush,

Thank you for the patch.

On Wed, Jul 22, 2020 at 08:53:06AM +0100, Naushir Patuck wrote:
> On Tue, 21 Jul 2020 at 11:59, Kieran Bingham wrote:
> > On 20/07/2020 10:13, 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.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >
> > Looks good to me
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > > ---
> > >  include/libcamera/ipa/raspberrypi.h           |  2 +-
> > >  src/ipa/raspberrypi/raspberrypi.cpp           | 20 +++++------
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 +++++++++++--------
> > >  3 files changed, 33 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > > index a4937769..0f31b6ea 100644
> > > --- a/include/libcamera/ipa/raspberrypi.h
> > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > @@ -14,6 +14,7 @@ enum RPiConfigParameters {
> > >       RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> > >       RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> > >       RPI_IPA_CONFIG_SENSOR = (1 << 2),
> > > +     RPI_IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > >  };
> > >
> > >  enum RPiOperations {
> > > @@ -21,7 +22,6 @@ 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_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 7bd04880..2d9fb9d8 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -65,8 +65,8 @@ class IPARPi : public IPAInterface
> > >  public:
> > >       IPARPi()
> > >               : lastMode_({}), controller_(), controllerInit_(false),
> > > -               frame_count_(0), check_count_(0), hide_count_(0),
> > > -               mistrust_count_(0), lsTable_(nullptr)
> > > +               frame_count_(0), check_count_(0), mistrust_count_(0),
> > > +               lsTableHandle_(0), lsTable_(nullptr)

Did lsTableHandle creep in during a rebase conflict resolution ?

> > >       {
> > >       }
> > >
> > > @@ -137,8 +137,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. */
> > > @@ -242,14 +240,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > >        */
> > >       frame_count_ = 0;
> > >       check_count_ = 0;
> > > +     int drop_frame = 0;

Maybe unsigned int (as a negative number would be really strange) ?

> > >       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();
> > >       }
> > >
> > > +     result->data.push_back(drop_frame);
> > > +     result->operation |= RPI_IPA_CONFIG_DROP_FRAMES;
> > > +
> > >       struct AgcStatus agcStatus;
> > >       /* These zero values mean not program anything (unless overwritten). */
> > >       agcStatus.shutter_time = 0.0;
> > > @@ -366,13 +368,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;
> >
> > The bit field flag here looks better to me indeed!
> >
> > >               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 6630ef57..d3639ce1 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -133,7 +133,7 @@ class RPiCameraData : public CameraData
> > >  public:
> > >       RPiCameraData(PipelineHandler *pipe)
> > >               : CameraData(pipe), sensor_(nullptr), state_(State::Stopped),
> > > -               dropFrame_(false), ispOutputCount_(0)
> > > +               dropFrameCount_(0), ispOutputCount_(0)
> > >       {
> > >       }
> > >
> > > @@ -181,14 +181,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
> > > @@ -999,6 +1000,7 @@ int RPiCameraData::configureIPA()
> > >       ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > >                       &result);
> > >
> > > +     unsigned int resultIdx = 0;
> > >       if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
> > >               /*
> > >                * Setup our staggered control writer with the sensor default
> > > @@ -1006,9 +1008,9 @@ int RPiCameraData::configureIPA()
> > >                */
> > >               if (!staggeredCtrl_) {
> > >                       staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> > > -                                         { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> > > -                                           { V4L2_CID_EXPOSURE, result.data[1] } });
> > > -                     sensorMetadata_ = result.data[2];
> > > +                                         { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> > > +                                           { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
> > > +                     sensorMetadata_ = result.data[resultIdx++];

Paul is experimenting with options to use custom data types for
communication between the pipeline handler and the IPA, it will
hopefully simplify this kind of use case.

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

> > >               }
> > >
> > >               /* Configure the H/V flip controls based on the sensor rotation. */
> > > @@ -1025,6 +1027,11 @@ int RPiCameraData::configureIPA()
> > >                       LOG(RPI, Error) << "V4L2 staggered set failed";
> > >       }
> > >
> > > +     if (result.operation & RPI_IPA_CONFIG_DROP_FRAMES) {
> > > +             /* Configure the number of dropped frames required on startup. */
> > > +             dropFrameCount_ = result.data[resultIdx++];
> >
> > I assume given this is about dropping frames on startup, it only occurs
> > once, and dropFrameCount_ is already zero at this point, so we're not
> > losing a previous request to drop frames for instance.
> 
> That's correct.  We now only signal how many frames to drop at
> startup, and this signal will not make us lose a previous drop frame
> request.
> 
> > > +     }
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -1075,7 +1082,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();
> > > @@ -1084,7 +1090,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;
> > >       }
> > > @@ -1250,7 +1255,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);
> > >               }
> > > @@ -1262,7 +1267,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));
> > > @@ -1307,7 +1312,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;
> > > @@ -1326,10 +1331,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)";
> > > +             }
> > >       }
> > >  }
> > >
Naushir Patuck July 22, 2020, 2:29 p.m. UTC | #4 
Hi Laurent,

Thank you for your review.

On Wed, 22 Jul 2020 at 15:27, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, Jul 22, 2020 at 08:53:06AM +0100, Naushir Patuck wrote:
> > On Tue, 21 Jul 2020 at 11:59, Kieran Bingham wrote:
> > > On 20/07/2020 10:13, 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.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > >
> > > Looks good to me
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > > ---
> > > >  include/libcamera/ipa/raspberrypi.h           |  2 +-
> > > >  src/ipa/raspberrypi/raspberrypi.cpp           | 20 +++++------
> > > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 +++++++++++--------
> > > >  3 files changed, 33 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > > > index a4937769..0f31b6ea 100644
> > > > --- a/include/libcamera/ipa/raspberrypi.h
> > > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > > @@ -14,6 +14,7 @@ enum RPiConfigParameters {
> > > >       RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> > > >       RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> > > >       RPI_IPA_CONFIG_SENSOR = (1 << 2),
> > > > +     RPI_IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > > >  };
> > > >
> > > >  enum RPiOperations {
> > > > @@ -21,7 +22,6 @@ 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_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 7bd04880..2d9fb9d8 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -65,8 +65,8 @@ class IPARPi : public IPAInterface
> > > >  public:
> > > >       IPARPi()
> > > >               : lastMode_({}), controller_(), controllerInit_(false),
> > > > -               frame_count_(0), check_count_(0), hide_count_(0),
> > > > -               mistrust_count_(0), lsTable_(nullptr)
> > > > +               frame_count_(0), check_count_(0), mistrust_count_(0),
> > > > +               lsTableHandle_(0), lsTable_(nullptr)
>
> Did lsTableHandle creep in during a rebase conflict resolution ?

Oops, good catch!

>
> > > >       {
> > > >       }
> > > >
> > > > @@ -137,8 +137,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. */
> > > > @@ -242,14 +240,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > >        */
> > > >       frame_count_ = 0;
> > > >       check_count_ = 0;
> > > > +     int drop_frame = 0;
>
> Maybe unsigned int (as a negative number would be really strange) ?

Ack.

>
> > > >       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();
> > > >       }
> > > >
> > > > +     result->data.push_back(drop_frame);
> > > > +     result->operation |= RPI_IPA_CONFIG_DROP_FRAMES;
> > > > +
> > > >       struct AgcStatus agcStatus;
> > > >       /* These zero values mean not program anything (unless overwritten). */
> > > >       agcStatus.shutter_time = 0.0;
> > > > @@ -366,13 +368,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;
> > >
> > > The bit field flag here looks better to me indeed!
> > >
> > > >               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 6630ef57..d3639ce1 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -133,7 +133,7 @@ class RPiCameraData : public CameraData
> > > >  public:
> > > >       RPiCameraData(PipelineHandler *pipe)
> > > >               : CameraData(pipe), sensor_(nullptr), state_(State::Stopped),
> > > > -               dropFrame_(false), ispOutputCount_(0)
> > > > +               dropFrameCount_(0), ispOutputCount_(0)
> > > >       {
> > > >       }
> > > >
> > > > @@ -181,14 +181,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
> > > > @@ -999,6 +1000,7 @@ int RPiCameraData::configureIPA()
> > > >       ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > > >                       &result);
> > > >
> > > > +     unsigned int resultIdx = 0;
> > > >       if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
> > > >               /*
> > > >                * Setup our staggered control writer with the sensor default
> > > > @@ -1006,9 +1008,9 @@ int RPiCameraData::configureIPA()
> > > >                */
> > > >               if (!staggeredCtrl_) {
> > > >                       staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> > > > -                                         { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> > > > -                                           { V4L2_CID_EXPOSURE, result.data[1] } });
> > > > -                     sensorMetadata_ = result.data[2];
> > > > +                                         { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> > > > +                                           { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
> > > > +                     sensorMetadata_ = result.data[resultIdx++];
>
> Paul is experimenting with options to use custom data types for
> communication between the pipeline handler and the IPA, it will
> hopefully simplify this kind of use case.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > > >               }
> > > >
> > > >               /* Configure the H/V flip controls based on the sensor rotation. */
> > > > @@ -1025,6 +1027,11 @@ int RPiCameraData::configureIPA()
> > > >                       LOG(RPI, Error) << "V4L2 staggered set failed";
> > > >       }
> > > >
> > > > +     if (result.operation & RPI_IPA_CONFIG_DROP_FRAMES) {
> > > > +             /* Configure the number of dropped frames required on startup. */
> > > > +             dropFrameCount_ = result.data[resultIdx++];
> > >
> > > I assume given this is about dropping frames on startup, it only occurs
> > > once, and dropFrameCount_ is already zero at this point, so we're not
> > > losing a previous request to drop frames for instance.
> >
> > That's correct.  We now only signal how many frames to drop at
> > startup, and this signal will not make us lose a previous drop frame
> > request.
> >
> > > > +     }
> > > > +
> > > >       return 0;
> > > >  }
> > > >
> > > > @@ -1075,7 +1082,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();
> > > > @@ -1084,7 +1090,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;
> > > >       }
> > > > @@ -1250,7 +1255,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);
> > > >               }
> > > > @@ -1262,7 +1267,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));
> > > > @@ -1307,7 +1312,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;
> > > > @@ -1326,10 +1331,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)";
> > > > +             }
> > > >       }
> > > >  }
> > > >
>
> --
> Regards,
>
> Laurent Pinchart

Regards,
Naush

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index a4937769..0f31b6ea 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -14,6 +14,7 @@  enum RPiConfigParameters {
 	RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
 	RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
 	RPI_IPA_CONFIG_SENSOR = (1 << 2),
+	RPI_IPA_CONFIG_DROP_FRAMES = (1 << 3),
 };
 
 enum RPiOperations {
@@ -21,7 +22,6 @@  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_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 7bd04880..2d9fb9d8 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -65,8 +65,8 @@  class IPARPi : public IPAInterface
 public:
 	IPARPi()
 		: lastMode_({}), controller_(), controllerInit_(false),
-		  frame_count_(0), check_count_(0), hide_count_(0),
-		  mistrust_count_(0), lsTable_(nullptr)
+		  frame_count_(0), check_count_(0), mistrust_count_(0),
+		  lsTableHandle_(0), lsTable_(nullptr)
 	{
 	}
 
@@ -137,8 +137,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. */
@@ -242,14 +240,18 @@  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();
 	}
 
+	result->data.push_back(drop_frame);
+	result->operation |= RPI_IPA_CONFIG_DROP_FRAMES;
+
 	struct AgcStatus agcStatus;
 	/* These zero values mean not program anything (unless overwritten). */
 	agcStatus.shutter_time = 0.0;
@@ -366,13 +368,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 6630ef57..d3639ce1 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -133,7 +133,7 @@  class RPiCameraData : public CameraData
 public:
 	RPiCameraData(PipelineHandler *pipe)
 		: CameraData(pipe), sensor_(nullptr), state_(State::Stopped),
-		  dropFrame_(false), ispOutputCount_(0)
+		  dropFrameCount_(0), ispOutputCount_(0)
 	{
 	}
 
@@ -181,14 +181,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
@@ -999,6 +1000,7 @@  int RPiCameraData::configureIPA()
 	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
 			&result);
 
+	unsigned int resultIdx = 0;
 	if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
 		/*
 		 * Setup our staggered control writer with the sensor default
@@ -1006,9 +1008,9 @@  int RPiCameraData::configureIPA()
 		 */
 		if (!staggeredCtrl_) {
 			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
-					    { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
-					      { V4L2_CID_EXPOSURE, result.data[1] } });
-			sensorMetadata_ = result.data[2];
+					    { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
+					      { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
+			sensorMetadata_ = result.data[resultIdx++];
 		}
 
 		/* Configure the H/V flip controls based on the sensor rotation. */
@@ -1025,6 +1027,11 @@  int RPiCameraData::configureIPA()
 			LOG(RPI, Error) << "V4L2 staggered set failed";
 	}
 
+	if (result.operation & RPI_IPA_CONFIG_DROP_FRAMES) {
+		/* Configure the number of dropped frames required on startup. */
+		dropFrameCount_ = result.data[resultIdx++];
+	}
+
 	return 0;
 }
 
@@ -1075,7 +1082,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();
@@ -1084,7 +1090,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;
 	}
@@ -1250,7 +1255,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);
 		}
@@ -1262,7 +1267,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));
@@ -1307,7 +1312,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;
@@ -1326,10 +1331,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)";
+		}
 	}
 }