[libcamera-devel] [PATCH v4 2/9] libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling
Naushir Patuck
naush at raspberrypi.com
Wed Jul 22 16:29:07 CEST 2020
Hi Laurent,
Thank you for your review.
On Wed, 22 Jul 2020 at 15:27, Laurent Pinchart
<laurent.pinchart at 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 at raspberrypi.com>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > >
> > > Looks good to me
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at 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 at 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
More information about the libcamera-devel
mailing list