[2/5] libcamera: rkisp1: Standardise frame number tracking
diff mbox series

Message ID 20240212153532.179283-3-dan.scally@ideasonboard.com
State Rejected, archived
Headers show
Series
  • Remove RkISP1FrameInfo class
Related show

Commit Message

Dan Scally Feb. 12, 2024, 3:35 p.m. UTC
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.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++--------------
 1 file changed, 10 insertions(+), 15 deletions(-)

Comments

Jacopo Mondi Feb. 13, 2024, 11:31 a.m. UTC | #1
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)

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>
> Signed-off-by: Daniel Scally <dan.scally@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();
>
>  	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;
> -
> -	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
>
Kieran Bingham Feb. 13, 2024, 1:24 p.m. UTC | #2
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@ideasonboard.com>
> 
> >
> > Signed-off-by: Daniel Scally <dan.scally@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
> >
Kieran Bingham Feb. 13, 2024, 2:28 p.m. UTC | #3
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@ideasonboard.com>
> > 
> > >
> > > Signed-off-by: Daniel Scally <dan.scally@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
> > >
Jacopo Mondi Feb. 13, 2024, 3:09 p.m. UTC | #4
Hi Kieran

On Tue, Feb 13, 2024 at 01:24:08PM +0000, Kieran Bingham wrote:
> 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@ideasonboard.com>
> >
> > >
> > > Signed-off-by: Daniel Scally <dan.scally@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
>

make the ? :)

Anyway, I'm failing to see why this patch changes the model when it
comes to feeding the pipeline from the application.

Before this patch data->frame_ was increased at queueRequestDevice() time
and request->sequence_, which is used now, is similarly incremented by
the pipeline handler base class before calling queueRequestDevice().

The pipeline feeds one parameters buffer to the IPA in
queueRequestDevice() (aka when application queues a request), and once
the IPA has filled in the parameters it calls
RkISP1CameraData::paramFilled() which queues buffers to the capture
video devices.

If the application does not queue any request it is then my
understanding that the IPA doesn't get triggered and doesn't do any
processing.

The only event that happens asynchronously from the request queueing
is the frameStart event, on which we poke at delayed controls but do
not actually trigger any IPA processing. This has not changed as far
as I can tell..

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

It's rather easy to track this condition, just save frames to disk
when capturing with 'cam' this makes the application stall long enough
to have two (or more) stats buffers being dequeued in between two
capture requests being submitted.


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

And that could actually be the real issue here.

We now do

        data->ipa_->processStatsBuffer(request->sequence(), buffer->cookie(),
                                       data->delayedCtrls_->get(request->sequence()));

While before we had

        data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
                                       data->delayedCtrls_->get(buffer->metadata().sequence));

So yes, it seems to me in this new method are not counting 'frames'
anymore, which is something we should indeed be doing mostly to
synchronize the sensor and the ISP control values...

In fact, I see different behaviours between current master and this
series. I'm testing capturing 100 frames to disk (hence triggering
some stalls). With master I see the image being visibile for the first
frames, then getting very dark then stabilizing. With this series the
image starts all black, then stabilizes. I've not yet found out why or
even if it related, just leaving it here for Dan to see if he had
noticed anything similar in his testing..

>
>
> > > -
> > > -     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
> > >
Kieran Bingham Feb. 13, 2024, 3:41 p.m. UTC | #5
Quoting Jacopo Mondi (2024-02-13 15:09:57)
> Hi Kieran
> 
> On Tue, Feb 13, 2024 at 01:24:08PM +0000, Kieran Bingham wrote:
> > 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@ideasonboard.com>
> > >
> > > >
> > > > Signed-off-by: Daniel Scally <dan.scally@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
> >
> 
> make the ? :)
> 
> Anyway, I'm failing to see why this patch changes the model when it
> comes to feeding the pipeline from the application.
> 
> Before this patch data->frame_ was increased at queueRequestDevice() time
> and request->sequence_, which is used now, is similarly incremented by
> the pipeline handler base class before calling queueRequestDevice().

And also increased if there was a gap identified in
PipelineHandlerRkISP1::statReady()


> The pipeline feeds one parameters buffer to the IPA in
> queueRequestDevice() (aka when application queues a request), and once
> the IPA has filled in the parameters it calls
> RkISP1CameraData::paramFilled() which queues buffers to the capture
> video devices.
> 
> If the application does not queue any request it is then my
> understanding that the IPA doesn't get triggered and doesn't do any
> processing.

I expect so. Though at some point, we might actually want to 'fix' that
so that the IPA can run if the stats are produced. Otherwise we're
losing that data, and the stats buffers 'can' likely run without
requests.

(Changing this behaviour is a big rabbit hole though, and likely
requires lots more synchronisations, and maybe isn't even likely to
happen/be possible if the ISP gets stalled when there's no image output
buffers....)



> The only event that happens asynchronously from the request queueing
> is the frameStart event, on which we poke at delayed controls but do
> not actually trigger any IPA processing. This has not changed as far
> as I can tell..
> 
> > > >
> > > >       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.
> >
> 
> It's rather easy to track this condition, just save frames to disk
> when capturing with 'cam' this makes the application stall long enough
> to have two (or more) stats buffers being dequeued in between two
> capture requests being submitted.
> 
> 
> > 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.
> >
> 
> And that could actually be the real issue here.

Yes, that's my concern. Essentially we have two 'timing source'. The
Sensor - generating sensor sequence numbers, and the applications
(implicitly) generating request sequence numbers.


> We now do
> 
>         data->ipa_->processStatsBuffer(request->sequence(), buffer->cookie(),
>                                        data->delayedCtrls_->get(request->sequence()));
> 
> While before we had
> 
>         data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
>                                        data->delayedCtrls_->get(buffer->metadata().sequence));
> 
> So yes, it seems to me in this new method are not counting 'frames'
> anymore, which is something we should indeed be doing mostly to
> synchronize the sensor and the ISP control values...

Yes, at some level we need to make sure we keep maintaining that
relationship.


> In fact, I see different behaviours between current master and this
> series. I'm testing capturing 100 frames to disk (hence triggering
> some stalls). With master I see the image being visibile for the first
> frames, then getting very dark then stabilizing. With this series the
> image starts all black, then stabilizes. I've not yet found out why or
> even if it related, just leaving it here for Dan to see if he had
> noticed anything similar in his testing..
> 
> >
> >
> > > > -
> > > > -     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
> > > >
Dan Scally Feb. 16, 2024, 5:04 p.m. UTC | #6
Hi Kieran, Jacopo

On 13/02/2024 15:09, Jacopo Mondi wrote:
> Hi Kieran
>
> On Tue, Feb 13, 2024 at 01:24:08PM +0000, Kieran Bingham wrote:
>> 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@ideasonboard.com>
>>>
>>>> Signed-off-by: Daniel Scally <dan.scally@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
>>
> make the ? :)
>
> Anyway, I'm failing to see why this patch changes the model when it
> comes to feeding the pipeline from the application.
>
> Before this patch data->frame_ was increased at queueRequestDevice() time
> and request->sequence_, which is used now, is similarly incremented by
> the pipeline handler base class before calling queueRequestDevice().
>
> The pipeline feeds one parameters buffer to the IPA in
> queueRequestDevice() (aka when application queues a request), and once
> the IPA has filled in the parameters it calls
> RkISP1CameraData::paramFilled() which queues buffers to the capture
> video devices.
>
> If the application does not queue any request it is then my
> understanding that the IPA doesn't get triggered and doesn't do any
> processing.
>
> The only event that happens asynchronously from the request queueing
> is the frameStart event, on which we poke at delayed controls but do
> not actually trigger any IPA processing. This has not changed as far
> as I can tell..
>
>>>>        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.
>>
> It's rather easy to track this condition, just save frames to disk
> when capturing with 'cam' this makes the application stall long enough
> to have two (or more) stats buffers being dequeued in between two
> capture requests being submitted.
>
>
>> 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.
>>
> And that could actually be the real issue here.
>
> We now do
>
>          data->ipa_->processStatsBuffer(request->sequence(), buffer->cookie(),
>                                         data->delayedCtrls_->get(request->sequence()));
>
> While before we had
>
>          data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
>                                         data->delayedCtrls_->get(buffer->metadata().sequence));
>
> So yes, it seems to me in this new method are not counting 'frames'
> anymore, which is something we should indeed be doing mostly to
> synchronize the sensor and the ISP control values...
>
> In fact, I see different behaviours between current master and this
> series. I'm testing capturing 100 frames to disk (hence triggering
> some stalls). With master I see the image being visibile for the first
> frames, then getting very dark then stabilizing. With this series the
> image starts all black, then stabilizes. I've not yet found out why or
> even if it related, just leaving it here for Dan to see if he had
> noticed anything similar in his testing..


So I didn't notice any difference between master and my branch in my testing. I'll graph it as 
Kieran suggested to try to have a more reproducible proof of equivalence. I couldn't see any reason 
to rely on the frame number coming from the driver instead of the request sequence, so it seemed ok 
to change. I've been working through it again today to see if I missed something, but as far as I 
can tell I haven't. In queueRequestDevice() the FrameInfo->frame is set to data->frame_, and then 
that value is used to index the IPA's frameContexts every other time they're accessed. Although 
data->frame_ is corrected in PipelineHandlerRkISP1::statReady() to track the sequence from the 
kernel driver it's not actually **used** following that correction until the next queueRequestDevice 
where it's used to set the new FrameInfo's frame member. In that regard then as far as I can tell, 
all that's happening is that it's accessing a different one of the 16 FrameContexts stored in the 
IPA's FCQueue...but given the chosen context is cleared in FCQueue::alloc anyway, that doesn't seem 
like a problem.


As for the delayed controls; it looks to me like the implementation doesn't really take account of 
the passed-in frame number to the IPA module anyway. IPARkISP1::processStatsBuffer() calls 
setControls(frame) - that function fills a ControlList with values from frameContext.agc and emits 
setSensorControls(frame, ctrls) but the RkISP1CameraData::setSensorControls() slot connected to that 
signal doesn't use the frame parameter - it just pushes the controls onto delayedCtrls_ with a 1 or 
2 frame delay depending on whether it's gain or exposure, and then the DelayedControls class 
internally relies on those delay values and its writeCount_ (taken from the kernel driver's 
sequence) and queueCount_ members to decide which controls to set. The values in frameContext.agc 
were filled during the Agc algorithm's ::prepare() with the values from the activeContext, 
calculated during the last processStatsBuffer...in other words as far as I can tell the controls set 
via the DelayedControls always take effect in 1/2 frames regardless of what frame number you use and 
always use the values calculated from whatever the most recently processed statistics buffer is. The 
actual applied control values are directly passed in from the pipeline handler rather than fetched 
with the frame/request number or anything, and they're fetched from DelayedControls without relying 
on either the request->sequence() number or the data->frame_ number...


This has been painful to follow, but I think, **think**, it's safe.


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

Patch
diff mbox series

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();
 
 	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;
-
-	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)