| Message ID | 20201123221234.485933-7-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Mon, Nov 23, 2020 at 11:12:32PM +0100, Niklas Söderlund wrote: > Instead of setting controls using the RkISP1 local Timeline helper use > the DelayedControls interface provided by the CameraSensor. The result > are the same, the controls are applied with a delay. s/are/is > > The values of the delays are however different between the two methods. > The values used in the Timeline solution was chosen after some s/was/were/ > experimentation and the values used in DelayedControls are taken from a > generic sensor. None of the two are a perfect match as the delays can be > different for different sensors used with the pipeline. How are we going to quantify those dealays when filling in the sensor database ? > > However using the interface provided by CameraSensor we prepare for the > future where sensor specific delays will provided by the CameraSensor > and used without any change in the pipeline. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++--------------- > 1 file changed, 13 insertions(+), 22 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 6e74a49abfda1b55..c3c4b5a65e3d9afe 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -121,8 +121,9 @@ class RkISP1CameraData : public CameraData > public: > RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath, > RkISP1SelfPath *selfPath) > - : CameraData(pipe), sensor_(nullptr), frame_(0), > - frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath) > + : CameraData(pipe), sensor_(nullptr), delayedCtrls_(nullptr), > + frame_(0), frameInfo_(pipe), mainPath_(mainPath), > + selfPath_(selfPath) > { > } > > @@ -136,6 +137,7 @@ public: > Stream mainPathStream_; > Stream selfPathStream_; > CameraSensor *sensor_; > + DelayedControls *delayedCtrls_; > unsigned int frame_; > std::vector<IPABuffer> ipaBuffers_; > RkISP1Frames frameInfo_; > @@ -346,23 +348,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request) > return nullptr; > } > > -class RkISP1ActionSetSensor : public FrameAction > -{ > -public: > - RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, const ControlList &controls) > - : FrameAction(frame, SetSensor), sensor_(sensor), controls_(controls) {} > - > -protected: > - void run() override > - { > - sensor_->setControls(&controls_); > - } > - > -private: > - CameraSensor *sensor_; > - ControlList controls_; > -}; > - > class RkISP1ActionQueueBuffers : public FrameAction > { > public: > @@ -430,9 +415,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame, > switch (action.operation) { > case RKISP1_IPA_ACTION_V4L2_SET: { > const ControlList &controls = action.controls[0]; > - timeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame, > - sensor_, > - controls)); > + delayedCtrls_->push(controls); I'm not sure I like this. As I've said I think this sits half-way between a 1-to-1 replacement of StaggeredCtrl and an half-backed enhancement. What I mean is: 1) Delays and which controls are delayed is a decision of the CameraSensor. The pipeline and the IPA has no say on this with the current interface. 2) Here we unconditionally push controls received from the IPA, which implies it has to know which one are delayed (otherwise the push() here fails). As suggested in the previous patch I would backtrack and mimic what StaggeredCtrls did, so let the pipeline initialize delays and which controls are delayed, as in the end the IPA has to know that anyway. Going forward I agree CameraSensor will have to handle this, but we need to find a better way to do so to decouple IPA and sensor specific requirements (or decide we can't and make the IPA sensor-aware, but I think nobody wants this). > break; > } > case RKISP1_IPA_ACTION_PARAM_FILLED: { > @@ -906,6 +889,8 @@ int PipelineHandlerRkISP1::start(Camera *camera) > }; > } > > + isp_->setFrameStartEnabled(true); > + > activeCamera_ = camera; > > /* Inform IPA of stream configuration and sensor controls. */ > @@ -933,6 +918,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > RkISP1CameraData *data = cameraData(camera); > int ret; > > + isp_->setFrameStartEnabled(false); > + > selfPath_.stop(); > mainPath_.stop(); > > @@ -1051,6 +1038,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > /* Initialize the camera properties. */ > data->properties_ = data->sensor_->properties(); > > + data->delayedCtrls_ = data->sensor_->delayedContols(); > + isp_->frameStart.connect(data->delayedCtrls_, > + &DelayedControls::frameStart); > + > ret = data->loadIPA(); > if (ret) > return ret; > -- > 2.29.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Fri, Nov 27, 2020 at 11:25:18AM +0100, Jacopo Mondi wrote: > On Mon, Nov 23, 2020 at 11:12:32PM +0100, Niklas Söderlund wrote: > > Instead of setting controls using the RkISP1 local Timeline helper use > > the DelayedControls interface provided by the CameraSensor. The result > > are the same, the controls are applied with a delay. > > s/are/is > > > The values of the delays are however different between the two methods. > > The values used in the Timeline solution was chosen after some > > s/was/were/ > > > experimentation and the values used in DelayedControls are taken from a > > generic sensor. None of the two are a perfect match as the delays can be > > different for different sensors used with the pipeline. > > How are we going to quantify those dealays when filling in the sensor > database ? This is information that the sensor documentation should provide. It can also be measured experimentally if needed, by changing controls drastically and checking after how many frames the new value takes effect. > > However using the interface provided by CameraSensor we prepare for the > > future where sensor specific delays will provided by the CameraSensor > > and used without any change in the pipeline. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++--------------- > > 1 file changed, 13 insertions(+), 22 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 6e74a49abfda1b55..c3c4b5a65e3d9afe 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -121,8 +121,9 @@ class RkISP1CameraData : public CameraData > > public: > > RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath, > > RkISP1SelfPath *selfPath) > > - : CameraData(pipe), sensor_(nullptr), frame_(0), > > - frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath) > > + : CameraData(pipe), sensor_(nullptr), delayedCtrls_(nullptr), > > + frame_(0), frameInfo_(pipe), mainPath_(mainPath), > > + selfPath_(selfPath) > > { > > } > > > > @@ -136,6 +137,7 @@ public: > > Stream mainPathStream_; > > Stream selfPathStream_; > > CameraSensor *sensor_; > > + DelayedControls *delayedCtrls_; > > unsigned int frame_; > > std::vector<IPABuffer> ipaBuffers_; > > RkISP1Frames frameInfo_; > > @@ -346,23 +348,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request) > > return nullptr; > > } > > > > -class RkISP1ActionSetSensor : public FrameAction > > -{ > > -public: > > - RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, const ControlList &controls) > > - : FrameAction(frame, SetSensor), sensor_(sensor), controls_(controls) {} > > - > > -protected: > > - void run() override > > - { > > - sensor_->setControls(&controls_); > > - } > > - > > -private: > > - CameraSensor *sensor_; > > - ControlList controls_; > > -}; > > - > > class RkISP1ActionQueueBuffers : public FrameAction > > { > > public: > > @@ -430,9 +415,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame, > > switch (action.operation) { > > case RKISP1_IPA_ACTION_V4L2_SET: { > > const ControlList &controls = action.controls[0]; > > - timeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame, > > - sensor_, > > - controls)); > > + delayedCtrls_->push(controls); There's an undocumented assumption in this series, that IPA will only include controls whose value changes in the sensor control list. The DelayedControls class doesn't skip setting controls whose values hasn't changed. I think that's fine, as setting a control to the same value may have side effects, so it's probably best to let the IPA decide about what controls to set. And now that I've written this, I wonder if there's really a use case for not optimizing this in the DelayedControls class. In any case, we should document the assumption, regardless of whether we avoid unneeded writes in DelayedControls, or expect the IPA to do so (in which case the IPA may need to return empty control lists when no value changes). > I'm not sure I like this. As I've said I think this sits half-way > between a 1-to-1 replacement of StaggeredCtrl and an half-backed > enhancement. > > What I mean is: > 1) Delays and which controls are delayed is a decision of the > CameraSensor. The pipeline and the IPA has no say on this with the > current interface. > 2) Here we unconditionally push controls received from the IPA, which > implies it has to know which one are delayed (otherwise the push() > here fails). > > As suggested in the previous patch I would backtrack and mimic what > StaggeredCtrls did, so let the pipeline initialize delays and which > controls are delayed, as in the end the IPA has to know that > anyway. > > Going forward I agree CameraSensor will have to handle this, but we > need to find a better way to do so to decouple IPA and sensor specific > requirements (or decide we can't and make the IPA sensor-aware, but I > think nobody wants this). I agree, as commented in the review of 5/8. I think we can move this to the CameraSensor class without too much trouble, but that will still require a bit of work, and I don't want to delay this series more than required. > > break; > > } > > case RKISP1_IPA_ACTION_PARAM_FILLED: { > > @@ -906,6 +889,8 @@ int PipelineHandlerRkISP1::start(Camera *camera) > > }; > > } > > > > + isp_->setFrameStartEnabled(true); > > + > > activeCamera_ = camera; > > > > /* Inform IPA of stream configuration and sensor controls. */ > > @@ -933,6 +918,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > > RkISP1CameraData *data = cameraData(camera); > > int ret; > > > > + isp_->setFrameStartEnabled(false); > > + > > selfPath_.stop(); > > mainPath_.stop(); > > > > @@ -1051,6 +1038,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > > /* Initialize the camera properties. */ > > data->properties_ = data->sensor_->properties(); > > > > + data->delayedCtrls_ = data->sensor_->delayedContols(); > > + isp_->frameStart.connect(data->delayedCtrls_, > > + &DelayedControls::frameStart); > > + > > ret = data->loadIPA(); > > if (ret) > > return ret;
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 6e74a49abfda1b55..c3c4b5a65e3d9afe 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -121,8 +121,9 @@ class RkISP1CameraData : public CameraData public: RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath, RkISP1SelfPath *selfPath) - : CameraData(pipe), sensor_(nullptr), frame_(0), - frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath) + : CameraData(pipe), sensor_(nullptr), delayedCtrls_(nullptr), + frame_(0), frameInfo_(pipe), mainPath_(mainPath), + selfPath_(selfPath) { } @@ -136,6 +137,7 @@ public: Stream mainPathStream_; Stream selfPathStream_; CameraSensor *sensor_; + DelayedControls *delayedCtrls_; unsigned int frame_; std::vector<IPABuffer> ipaBuffers_; RkISP1Frames frameInfo_; @@ -346,23 +348,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request) return nullptr; } -class RkISP1ActionSetSensor : public FrameAction -{ -public: - RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, const ControlList &controls) - : FrameAction(frame, SetSensor), sensor_(sensor), controls_(controls) {} - -protected: - void run() override - { - sensor_->setControls(&controls_); - } - -private: - CameraSensor *sensor_; - ControlList controls_; -}; - class RkISP1ActionQueueBuffers : public FrameAction { public: @@ -430,9 +415,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame, switch (action.operation) { case RKISP1_IPA_ACTION_V4L2_SET: { const ControlList &controls = action.controls[0]; - timeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame, - sensor_, - controls)); + delayedCtrls_->push(controls); break; } case RKISP1_IPA_ACTION_PARAM_FILLED: { @@ -906,6 +889,8 @@ int PipelineHandlerRkISP1::start(Camera *camera) }; } + isp_->setFrameStartEnabled(true); + activeCamera_ = camera; /* Inform IPA of stream configuration and sensor controls. */ @@ -933,6 +918,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera) RkISP1CameraData *data = cameraData(camera); int ret; + isp_->setFrameStartEnabled(false); + selfPath_.stop(); mainPath_.stop(); @@ -1051,6 +1038,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) /* Initialize the camera properties. */ data->properties_ = data->sensor_->properties(); + data->delayedCtrls_ = data->sensor_->delayedContols(); + isp_->frameStart.connect(data->delayedCtrls_, + &DelayedControls::frameStart); + ret = data->loadIPA(); if (ret) return ret;
Instead of setting controls using the RkISP1 local Timeline helper use the DelayedControls interface provided by the CameraSensor. The result are the same, the controls are applied with a delay. The values of the delays are however different between the two methods. The values used in the Timeline solution was chosen after some experimentation and the values used in DelayedControls are taken from a generic sensor. None of the two are a perfect match as the delays can be different for different sensors used with the pipeline. However using the interface provided by CameraSensor we prepare for the future where sensor specific delays will provided by the CameraSensor and used without any change in the pipeline. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++--------------- 1 file changed, 13 insertions(+), 22 deletions(-)