| Message ID | 20240212153532.179283-3-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 >
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 > >
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 > > >
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 > > >
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 > > > >
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 >>>>
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)
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(-)