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

[Kieran Bingham]

Quoting Kieran Bingham (2024-02-13 13:24:08)
> 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 

Err ... language failure? GPT overflow? Something else? I'm not sure
what happened here.

I think I was starting to talk here about the handling of the data
sequence number likely being based aroudn the number of *frames*
contrasted with the number of requests on request->sequence() which are
two separate sequence generators.

> 
> > >
> > >       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
> > >