[PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking

[Kieran Bingham]

Quoting Jacopo Mondi (2024-02-13 11:31:31)
> Hi Dan
> 
> On Mon, Feb 12, 2024 at 03:35:29PM +0000, Daniel Scally wrote:
> > The rkisp1 driver uses a couple of different methods of tracking the
> > number of the frame - standardise them on request->sequence() and
> > remove the frame_ member of RkISP1CameraData which was being used to
> > do the job.
> 
> Request::Private::sequence_ gets assigned to
> Camera::Private::requestSequence_ by the PipelineHandler base class.
> 
> Camera::Private::requestSequence_ gets zeroed in
> PipelineHandler::stop()
> 
> Camera::Private::requestSequence_ gets incremented in
> PipelineHandler::doQueueRequest()
> 
> So I think this change is legit (and should be done on IPU3 as well if
> it uses the same scheme that was implemented here)

I agree there, if this series works well, then we should handle both
IPU3 and RKISP1 together to keep things aligned, and do the same
cleanups on both sides.


> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> >
> > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 5460dc11..22e553fe 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -87,7 +87,7 @@ class RkISP1CameraData : public Camera::Private
> >  public:
> >       RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,
> >                        RkISP1SelfPath *selfPath)
> > -             : Camera::Private(pipe), frame_(0), frameInfo_(pipe),
> > +             : Camera::Private(pipe), frameInfo_(pipe),
> >                 mainPath_(mainPath), selfPath_(selfPath)
> >       {
> >       }
> > @@ -99,7 +99,6 @@ public:
> >       Stream selfPathStream_;
> >       std::unique_ptr<CameraSensor> sensor_;
> >       std::unique_ptr<DelayedControls> delayedCtrls_;
> > -     unsigned int frame_;
> >       std::vector<IPABuffer> ipaBuffers_;
> >       RkISP1Frames frameInfo_;
> >
> > @@ -212,7 +211,7 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> >  RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,
> >                                     bool isRaw)
> >  {
> > -     unsigned int frame = data->frame_;
> > +     unsigned int frame = request->sequence();

This worries me a little when we start thinking about what might happen
if frames are dropped/application slows down.

We probably need some tests in lc-compliance to validate what happens if
the applciation *doesn't* keep up with the camera. I think before that
would make the 

> >
> >       FrameBuffer *paramBuffer = nullptr;
> >       FrameBuffer *statBuffer = nullptr;
> > @@ -233,6 +232,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
> >
> >               statBuffer = pipe_->availableStatBuffers_.front();
> >               pipe_->availableStatBuffers_.pop();
> > +             statBuffer->_d()->setRequest(request);
> >       }
> >
> >       FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> > @@ -932,8 +932,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
> >               return ret;
> >       }
> >
> > -     data->frame_ = 0;
> > -
> >       if (!isRaw_) {
> >               ret = param_->streamOn();
> >               if (ret) {
> > @@ -1025,7 +1023,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> >       if (!info)
> >               return -ENOENT;
> >
> > -     data->ipa_->queueRequest(data->frame_, request->controls());
> > +     data->ipa_->queueRequest(request->sequence(), request->controls());
> >       if (isRaw_) {
> >               if (info->mainPathBuffer)
> >                       data->mainPath_->queueBuffer(info->mainPathBuffer);
> > @@ -1033,12 +1031,10 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> >               if (data->selfPath_ && info->selfPathBuffer)
> >                       data->selfPath_->queueBuffer(info->selfPathBuffer);
> >       } else {
> > -             data->ipa_->fillParamsBuffer(data->frame_,
> > +             data->ipa_->fillParamsBuffer(request->sequence(),
> >                                            info->paramBuffer->cookie());
> >       }
> >
> > -     data->frame_++;
> > -
> >       return 0;
> >  }
> >
> > @@ -1269,7 +1265,8 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> >               if (isRaw_) {
> >                       const ControlList &ctrls =
> >                               data->delayedCtrls_->get(metadata.sequence);
> > -                     data->ipa_->processStatsBuffer(info->frame, 0, ctrls);
> > +                     data->ipa_->processStatsBuffer(request->sequence(),
> > +                                                    0, ctrls);
> >               }
> >       } else {
> >               if (isRaw_)
> > @@ -1284,6 +1281,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> >  {
> >       ASSERT(activeCamera_);
> >       RkISP1CameraData *data = cameraData(activeCamera_);
> > +     Request *request = buffer->request();
> >
> >       RkISP1FrameInfo *info = data->frameInfo_.find(buffer);
> >       if (!info)
> > @@ -1295,11 +1293,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> >               return;
> >       }
> >
> > -     if (data->frame_ <= buffer->metadata().sequence)
> > -             data->frame_ = buffer->metadata().sequence + 1;

for instance, it looks like there is some handling here that is tracking
data->frame_ to match the sequence number from the image pipelines.

Now the sequencing is all based around the request sequences queued.
That might be all fine, but I'm weary about how we might need to keep
things aligned somewhere to the real sequence number from the camera.
Especially when it comes to tracking what controls were trully applied
through DelayedControls etc.



> > -
> > -     data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
> > -                                    data->delayedCtrls_->get(buffer->metadata().sequence));
> > +     data->ipa_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),
> > +                                    data->delayedCtrls_->get(request->sequence()));
> >  }
> >
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)
> > --
> > 2.34.1
> >