| Message ID | 20201110002710.3233696-6-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Tue, Nov 10, 2020 at 01:27:07AM +0100, Niklas Söderlund wrote: > Expose the optional DelayedControls interface to pipeline handlers. If > used by a pipeline generic delays are used. In the future the delay > values should be fetched to match the specific sensor module, either > from a new kernel interface or worst case a sensors database. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/internal/camera_sensor.h | 5 ++++ > src/libcamera/camera_sensor.cpp | 31 ++++++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index b9eba89f00fba882..6be1cfd49134c534 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -14,6 +14,7 @@ > #include <libcamera/controls.h> > #include <libcamera/geometry.h> > > +#include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/formats.h" > #include "libcamera/internal/log.h" > #include "libcamera/internal/v4l2_subdevice.h" > @@ -61,6 +62,8 @@ public: > ControlList getControls(const std::vector<uint32_t> &ids); > int setControls(ControlList *ctrls); > > + DelayedControls *delayedContols(); > + > const ControlList &properties() const { return properties_; } > int sensorInfo(CameraSensorInfo *info) const; > > @@ -83,6 +86,8 @@ private: > std::vector<Size> sizes_; > > ControlList properties_; > + > + std::unique_ptr<DelayedControls> delayedControls_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 935de528c4963453..6c92c97f4cc2547a 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls) > return subdev_->setControls(ctrls); > } > > +/** > + * \brief Get the sensors delayed controls interface > + * > + * Access the sensors delayed controls interface. This interface aids pipelines > + * keeping tack of controls that needs time to take effect. The interface is > + * optional to use and does not interact with setControls() and getControls() > + * that operates directly on the sensor device. > + * > + * \sa DelayedControls > + * \return The delayed controls interface > + */ > +DelayedControls *CameraSensor::delayedContols() > +{ > + if (!delayedControls_) { > + /* > + * \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 }, > + }; I would really not hardcode them here. As long as we don't have a sensor database or a place where these values can be fetched from isn't it better to provide something like using DelayMap = std::unordered_map<const ControlId *, unsigned int>; CameraSensor::initControlDelays(DelayMap map) { .... /* * CameraSensor gets the numerical ID from the * DelayMap and construct a new map making sure the * controls are supported by the subdev */ delayedControls_ = std::make_unique<DelayedControls>(subdev_.get(), delays); } So that pipeline handlers receive delays from the IPA (as RPi currently does if I'm not mistaken) and intialize Delayed controls on the CameraSensor passing controls as standard libcamera ControlId ? > + > + delayedControls_ = > + std::make_unique<DelayedControls>(subdev_.get(), delays); > + } > + > + return delayedControls_.get(); > +} > + > /** > * \brief Assemble and return the camera sensor info > * \param[out] info The camera sensor info > -- > 2.29.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2020-11-18 15:23:19 +0100, Jacopo Mondi wrote: > Hi Niklas, > > On Tue, Nov 10, 2020 at 01:27:07AM +0100, Niklas Söderlund wrote: > > Expose the optional DelayedControls interface to pipeline handlers. If > > used by a pipeline generic delays are used. In the future the delay > > values should be fetched to match the specific sensor module, either > > from a new kernel interface or worst case a sensors database. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/internal/camera_sensor.h | 5 ++++ > > src/libcamera/camera_sensor.cpp | 31 ++++++++++++++++++++++ > > 2 files changed, 36 insertions(+) > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > index b9eba89f00fba882..6be1cfd49134c534 100644 > > --- a/include/libcamera/internal/camera_sensor.h > > +++ b/include/libcamera/internal/camera_sensor.h > > @@ -14,6 +14,7 @@ > > #include <libcamera/controls.h> > > #include <libcamera/geometry.h> > > > > +#include "libcamera/internal/delayed_controls.h" > > #include "libcamera/internal/formats.h" > > #include "libcamera/internal/log.h" > > #include "libcamera/internal/v4l2_subdevice.h" > > @@ -61,6 +62,8 @@ public: > > ControlList getControls(const std::vector<uint32_t> &ids); > > int setControls(ControlList *ctrls); > > > > + DelayedControls *delayedContols(); > > + > > const ControlList &properties() const { return properties_; } > > int sensorInfo(CameraSensorInfo *info) const; > > > > @@ -83,6 +86,8 @@ private: > > std::vector<Size> sizes_; > > > > ControlList properties_; > > + > > + std::unique_ptr<DelayedControls> delayedControls_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 935de528c4963453..6c92c97f4cc2547a 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls) > > return subdev_->setControls(ctrls); > > } > > > > +/** > > + * \brief Get the sensors delayed controls interface > > + * > > + * Access the sensors delayed controls interface. This interface aids pipelines > > + * keeping tack of controls that needs time to take effect. The interface is > > + * optional to use and does not interact with setControls() and getControls() > > + * that operates directly on the sensor device. > > + * > > + * \sa DelayedControls > > + * \return The delayed controls interface > > + */ > > +DelayedControls *CameraSensor::delayedContols() > > +{ > > + if (!delayedControls_) { > > + /* > > + * \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 }, > > + }; > > I would really not hardcode them here. > > As long as we don't have a sensor database or a place where these > values can be fetched from isn't it better to provide something like > > using DelayMap = std::unordered_map<const ControlId *, unsigned int>; > > CameraSensor::initControlDelays(DelayMap map) > { > .... > /* > * CameraSensor gets the numerical ID from the > * DelayMap and construct a new map making sure the > * controls are supported by the subdev > */ > delayedControls_ = > std::make_unique<DelayedControls>(subdev_.get(), delays); > > } > > So that pipeline handlers receive delays from the IPA (as RPi > currently does if I'm not mistaken) and intialize Delayed controls on > the CameraSensor passing controls as standard libcamera ControlId ? I'm sorry I don't like this idea and I think the RPi approach is backwards :-) If we allow IPA's or pipelines to set the sensor delays we will IMHO end up with delay values coming from a wide range of sources. Some will come from pipelines or IPAs that link with libcamera that do utilise a sensor database. Some will come from hard coded values in pipeline or inside IPAs (like RPi today) and thus limit the usage of that IPA only to those sensors. It becomes even worse if we think about closed source IPAs where new sensors can not be added or incorrect delays corrected. I think the best solution is to embed the database lookup inside the CameraSensor class and have pipelines read the delays readout delays from the CameraSensor instance . If the IPA needs to be informed of the delays the pipeline should pass the delay values and not the sensor name to the IPA. For this reason I think its good to hardcode the values here as this is the location we should drop in a sensor database lookup once we have it. > > > + > > + delayedControls_ = > > + std::make_unique<DelayedControls>(subdev_.get(), delays); > > + } > > + > > + return delayedControls_.get(); > > +} > > + > > /** > > * \brief Assemble and return the camera sensor info > > * \param[out] info The camera sensor info > > -- > > 2.29.2 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Mon, Nov 23, 2020 at 10:49:39PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your feedback. > > On 2020-11-18 15:23:19 +0100, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Tue, Nov 10, 2020 at 01:27:07AM +0100, Niklas Söderlund wrote: > > > Expose the optional DelayedControls interface to pipeline handlers. If > > > used by a pipeline generic delays are used. In the future the delay > > > values should be fetched to match the specific sensor module, either > > > from a new kernel interface or worst case a sensors database. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > include/libcamera/internal/camera_sensor.h | 5 ++++ > > > src/libcamera/camera_sensor.cpp | 31 ++++++++++++++++++++++ > > > 2 files changed, 36 insertions(+) > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > index b9eba89f00fba882..6be1cfd49134c534 100644 > > > --- a/include/libcamera/internal/camera_sensor.h > > > +++ b/include/libcamera/internal/camera_sensor.h > > > @@ -14,6 +14,7 @@ > > > #include <libcamera/controls.h> > > > #include <libcamera/geometry.h> > > > > > > +#include "libcamera/internal/delayed_controls.h" > > > #include "libcamera/internal/formats.h" > > > #include "libcamera/internal/log.h" > > > #include "libcamera/internal/v4l2_subdevice.h" > > > @@ -61,6 +62,8 @@ public: > > > ControlList getControls(const std::vector<uint32_t> &ids); > > > int setControls(ControlList *ctrls); > > > > > > + DelayedControls *delayedContols(); > > > + > > > const ControlList &properties() const { return properties_; } > > > int sensorInfo(CameraSensorInfo *info) const; > > > > > > @@ -83,6 +86,8 @@ private: > > > std::vector<Size> sizes_; > > > > > > ControlList properties_; > > > + > > > + std::unique_ptr<DelayedControls> delayedControls_; > > > }; > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index 935de528c4963453..6c92c97f4cc2547a 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls) > > > return subdev_->setControls(ctrls); > > > } > > > > > > +/** > > > + * \brief Get the sensors delayed controls interface > > > + * > > > + * Access the sensors delayed controls interface. This interface aids pipelines > > > + * keeping tack of controls that needs time to take effect. The interface is > > > + * optional to use and does not interact with setControls() and getControls() > > > + * that operates directly on the sensor device. > > > + * > > > + * \sa DelayedControls > > > + * \return The delayed controls interface > > > + */ > > > +DelayedControls *CameraSensor::delayedContols() > > > +{ > > > + if (!delayedControls_) { > > > + /* > > > + * \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 }, > > > + }; > > > > I would really not hardcode them here. > > > > As long as we don't have a sensor database or a place where these > > values can be fetched from isn't it better to provide something like > > > > using DelayMap = std::unordered_map<const ControlId *, unsigned int>; > > > > CameraSensor::initControlDelays(DelayMap map) > > { > > .... > > /* > > * CameraSensor gets the numerical ID from the > > * DelayMap and construct a new map making sure the > > * controls are supported by the subdev > > */ > > delayedControls_ = > > std::make_unique<DelayedControls>(subdev_.get(), delays); > > > > } > > > > So that pipeline handlers receive delays from the IPA (as RPi > > currently does if I'm not mistaken) and intialize Delayed controls on > > the CameraSensor passing controls as standard libcamera ControlId ? > > I'm sorry I don't like this idea and I think the RPi approach is > backwards :-) If we allow IPA's or pipelines to set the sensor delays we > will IMHO end up with delay values coming from a wide range of sources. > > Some will come from pipelines or IPAs that link with libcamera that do > utilise a sensor database. Some will come from hard coded values in > pipeline or inside IPAs (like RPi today) and thus limit the usage of > that IPA only to those sensors. It becomes even worse if we think about > closed source IPAs where new sensors can not be added or incorrect > delays corrected. > > I think the best solution is to embed the database lookup inside the > CameraSensor class and have pipelines read the delays readout delays > from the CameraSensor instance . If the IPA needs to be informed of the > delays the pipeline should pass the delay values and not the sensor name > to the IPA. > > For this reason I think its good to hardcode the values here as this is > the location we should drop in a sensor database lookup once we have it. > I understand. Let's try with this approach to encourage us and other developers to grow the sensor database and make it a first class citizen. We could scale back to shortcuts eventually, but I agree let's not start with that in mind. Thanks j > > > > > + > > > + delayedControls_ = > > > + std::make_unique<DelayedControls>(subdev_.get(), delays); > > > + } > > > + > > > + return delayedControls_.get(); > > > +} > > > + > > > /** > > > * \brief Assemble and return the camera sensor info > > > * \param[out] info The camera sensor info > > > -- > > > 2.29.2 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index b9eba89f00fba882..6be1cfd49134c534 100644 --- a/include/libcamera/internal/camera_sensor.h +++ b/include/libcamera/internal/camera_sensor.h @@ -14,6 +14,7 @@ #include <libcamera/controls.h> #include <libcamera/geometry.h> +#include "libcamera/internal/delayed_controls.h" #include "libcamera/internal/formats.h" #include "libcamera/internal/log.h" #include "libcamera/internal/v4l2_subdevice.h" @@ -61,6 +62,8 @@ public: ControlList getControls(const std::vector<uint32_t> &ids); int setControls(ControlList *ctrls); + DelayedControls *delayedContols(); + const ControlList &properties() const { return properties_; } int sensorInfo(CameraSensorInfo *info) const; @@ -83,6 +86,8 @@ private: std::vector<Size> sizes_; ControlList properties_; + + std::unique_ptr<DelayedControls> delayedControls_; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 935de528c4963453..6c92c97f4cc2547a 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls) return subdev_->setControls(ctrls); } +/** + * \brief Get the sensors delayed controls interface + * + * Access the sensors delayed controls interface. This interface aids pipelines + * keeping tack of controls that needs time to take effect. The interface is + * optional to use and does not interact with setControls() and getControls() + * that operates directly on the sensor device. + * + * \sa DelayedControls + * \return The delayed controls interface + */ +DelayedControls *CameraSensor::delayedContols() +{ + if (!delayedControls_) { + /* + * \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 }, + }; + + delayedControls_ = + std::make_unique<DelayedControls>(subdev_.get(), delays); + } + + return delayedControls_.get(); +} + /** * \brief Assemble and return the camera sensor info * \param[out] info The camera sensor info
Expose the optional DelayedControls interface to pipeline handlers. If used by a pipeline generic delays are used. In the future the delay values should be fetched to match the specific sensor module, either from a new kernel interface or worst case a sensors database. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/internal/camera_sensor.h | 5 ++++ src/libcamera/camera_sensor.cpp | 31 ++++++++++++++++++++++ 2 files changed, 36 insertions(+)