| Message ID | 20201215004811.602429-7-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Tue, Dec 15, 2020 at 01:48:09AM +0100, Niklas Söderlund wrote: > Instead of setting controls using the RkISP1 local Timeline helper use > the DelayedControls. The result is 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 were 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. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > * Changes since v3 > - Rewrite to not use CameraSensor. > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 43 +++++++++++++----------- > 1 file changed, 23 insertions(+), 20 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 4d98c9027f42c759..513a60b04e5f2e21 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -23,6 +23,7 @@ > #include <libcamera/stream.h> > > #include "libcamera/internal/camera_sensor.h" > +#include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/ipa_manager.h" > #include "libcamera/internal/log.h" > @@ -136,6 +137,7 @@ public: > Stream mainPathStream_; > Stream selfPathStream_; > CameraSensor *sensor_; > + std::unique_ptr<DelayedControls> delayedCtrls_; > unsigned int frame_; > std::vector<IPABuffer> ipaBuffers_; > RkISP1Frames frameInfo_; > @@ -345,23 +347,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: > @@ -429,9 +414,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: { > @@ -898,6 +881,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > }; > } > > + isp_->setFrameStartEnabled(true); > + > activeCamera_ = camera; > > /* Inform IPA of stream configuration and sensor controls. */ > @@ -925,6 +910,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > RkISP1CameraData *data = cameraData(camera); > int ret; > > + isp_->setFrameStartEnabled(false); > + > selfPath_.stop(); > mainPath_.stop(); > > @@ -1043,6 +1030,22 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > /* Initialize the camera properties. */ > data->properties_ = data->sensor_->properties(); > > + /* > + * \todo Read dealy values from the sensor itself or from a > + * a sensor database. For now use generic values taken from > + * the Raspberry Pi and listed as generic values. > + */ > + std::unordered_map<uint32_t, unsigned int> delays = { > + { V4L2_CID_ANALOGUE_GAIN, 1 }, > + { V4L2_CID_EXPOSURE, 2 }, > + }; > + > + data->delayedCtrls_ = > + std::make_unique<DelayedControls>(data->sensor_->device(), > + delays); > + isp_->frameStart.connect(data->delayedCtrls_.get(), > + &DelayedControls::applyControls); > + > 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 Tue, Dec 15, 2020 at 01:48:09AM +0100, Niklas Söderlund wrote: > Instead of setting controls using the RkISP1 local Timeline helper use > the DelayedControls. The result is 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 were 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. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > * Changes since v3 > - Rewrite to not use CameraSensor. > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 43 +++++++++++++----------- > 1 file changed, 23 insertions(+), 20 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 4d98c9027f42c759..513a60b04e5f2e21 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -23,6 +23,7 @@ > #include <libcamera/stream.h> > > #include "libcamera/internal/camera_sensor.h" > +#include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/ipa_manager.h" > #include "libcamera/internal/log.h" > @@ -136,6 +137,7 @@ public: > Stream mainPathStream_; > Stream selfPathStream_; > CameraSensor *sensor_; > + std::unique_ptr<DelayedControls> delayedCtrls_; > unsigned int frame_; > std::vector<IPABuffer> ipaBuffers_; > RkISP1Frames frameInfo_; > @@ -345,23 +347,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: > @@ -429,9 +414,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: { > @@ -898,6 +881,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > }; > } > > + isp_->setFrameStartEnabled(true); > + > activeCamera_ = camera; > > /* Inform IPA of stream configuration and sensor controls. */ > @@ -925,6 +910,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > RkISP1CameraData *data = cameraData(camera); > int ret; > > + isp_->setFrameStartEnabled(false); > + > selfPath_.stop(); > mainPath_.stop(); > > @@ -1043,6 +1030,22 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > /* Initialize the camera properties. */ > data->properties_ = data->sensor_->properties(); > > + /* > + * \todo Read dealy values from the sensor itself or from a > + * a sensor database. For now use generic values taken from > + * the Raspberry Pi and listed as generic values. > + */ > + std::unordered_map<uint32_t, unsigned int> delays = { > + { V4L2_CID_ANALOGUE_GAIN, 1 }, > + { V4L2_CID_EXPOSURE, 2 }, > + }; > + > + data->delayedCtrls_ = > + std::make_unique<DelayedControls>(data->sensor_->device(), > + delays); > + isp_->frameStart.connect(data->delayedCtrls_.get(), > + &DelayedControls::applyControls); > + > ret = data->loadIPA(); > if (ret) > return ret;
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 4d98c9027f42c759..513a60b04e5f2e21 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -23,6 +23,7 @@ #include <libcamera/stream.h> #include "libcamera/internal/camera_sensor.h" +#include "libcamera/internal/delayed_controls.h" #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/ipa_manager.h" #include "libcamera/internal/log.h" @@ -136,6 +137,7 @@ public: Stream mainPathStream_; Stream selfPathStream_; CameraSensor *sensor_; + std::unique_ptr<DelayedControls> delayedCtrls_; unsigned int frame_; std::vector<IPABuffer> ipaBuffers_; RkISP1Frames frameInfo_; @@ -345,23 +347,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: @@ -429,9 +414,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: { @@ -898,6 +881,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c }; } + isp_->setFrameStartEnabled(true); + activeCamera_ = camera; /* Inform IPA of stream configuration and sensor controls. */ @@ -925,6 +910,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera) RkISP1CameraData *data = cameraData(camera); int ret; + isp_->setFrameStartEnabled(false); + selfPath_.stop(); mainPath_.stop(); @@ -1043,6 +1030,22 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) /* Initialize the camera properties. */ data->properties_ = data->sensor_->properties(); + /* + * \todo Read dealy values from the sensor itself or from a + * a sensor database. For now use generic values taken from + * the Raspberry Pi and listed as generic values. + */ + std::unordered_map<uint32_t, unsigned int> delays = { + { V4L2_CID_ANALOGUE_GAIN, 1 }, + { V4L2_CID_EXPOSURE, 2 }, + }; + + data->delayedCtrls_ = + std::make_unique<DelayedControls>(data->sensor_->device(), + delays); + isp_->frameStart.connect(data->delayedCtrls_.get(), + &DelayedControls::applyControls); + ret = data->loadIPA(); if (ret) return ret;
Instead of setting controls using the RkISP1 local Timeline helper use the DelayedControls. The result is 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 were 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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- * Changes since v3 - Rewrite to not use CameraSensor. --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 43 +++++++++++++----------- 1 file changed, 23 insertions(+), 20 deletions(-)