| Message ID | 20201028010051.3830668-7-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the changes. I will go through all the patches with review comment in due course, but wanted to raise a critical point below: On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund < niklas.soderlund@ragnatech.se> 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 }, > + }; > This will break the OV5647 sensor usage with Raspberry Pi without referring back to a sensor database . OV5647 has a delay of 2 for gain and exposure, so will not work with the above hard-coded settings. In a more general sense, I wonder if initialising the DelayedControl here is the right thing to do? I have some changes waiting to be submitted for review that add framerate control to the Raspberry Pi stack. In this change, I've added a VBLANK control to the staggered writer. There are also some changes to control very long exposures on the imx477 that go through the staggered writer. In these cases, I would have to add values to the map above, but other pipeline handlers would never need to use them. This may be a bit wasteful, I don't know... Would it make more sense for the pipeline handlers to initialise the DelayedControls as they need to? Regards, Naush > + > + 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.1 > >
Hi Naushir, Thanks for your feedback. On 2020-10-28 11:51:13 +0000, Naushir Patuck wrote: > Hi Niklas, > > Thank you for the changes. I will go through all the patches with review > comment in due course, but wanted to raise a critical point below: > > On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund < > niklas.soderlund@ragnatech.se> 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 }, > > + }; > > > > This will break the OV5647 sensor usage with Raspberry Pi without > referring back to a sensor database . OV5647 has a delay of 2 for gain and > exposure, so will not work with the above hard-coded settings. I understand your point, and we really should get started with a solution on how to get sensor specific data into CameraSensor database. But with this series there is no problem for the Raspberry Pi pipeline as it does not deal with controls on the CameraSensor but the V4L2 video device. Therefor the RPi pipeline creates a private instance of DelayedControls witch delays still are initialized by the RPi IPA, just as things worked before using StaggerdCtrls. When developing this I ran DelayedControls and StaggerdCtrls in parallel on the RPi and the output of both matched perfectly frame by frame. > > In a more general sense, I wonder if initialising the DelayedControl here > is the right thing to do? I have some changes waiting to be submitted for > review that add framerate control to the Raspberry Pi stack. In this > change, I've added a VBLANK control to the staggered writer. There are > also some changes to control very long exposures on the imx477 that go > through the staggered writer. In these cases, I would have to add values > to the map above, but other pipeline handlers would never need to use > them. This may be a bit wasteful, I don't know... Would it make more > sense for the pipeline handlers to initialise the DelayedControls as they > need to? First, we are still thinking about how VBLANK and other controls that can modify the frame length are to be modelled in the CameraSensor. If you have any ideas or tips in this area I would greatly appreciate them. I really think CameraSensor is the location to initialising the delays, this separates the sensor from the pipeline. I think this is important as otherwise each pipeline will have have a list of sensors it knows about and will not function popery with different ones. By "forcing" pipelines to learn about the sensor used thru CameraSensor interface we ensure that we can add more sensors in the future without having to update all pipelines and/or IPAs. This is even more important if we consider closed-source IPAs as we can't just add a new sensor with timings recompile. Yet again as the Raspberry Pi pipeline is video device centric it can't really use the CameraSensor to deal with controls and must instead create and configure its local version of DelayedControls. I still think in the future such pipelines can benefit from a sensor database in CameraSensor as it should be possible to query the datbase and use the delays reported while still creating a private local DelayedControls. Does this relive your worries bout this series? > > Regards, > Naush > > > > > + > > + 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.1 > > > >
Hi Niklas, Thank you for the clarification. On Wed, 28 Oct 2020 at 13:26, Niklas Söderlund < niklas.soderlund@ragnatech.se> wrote: > Hi Naushir, > > Thanks for your feedback. > > On 2020-10-28 11:51:13 +0000, Naushir Patuck wrote: > > Hi Niklas, > > > > Thank you for the changes. I will go through all the patches with review > > comment in due course, but wanted to raise a critical point below: > > > > On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund < > > niklas.soderlund@ragnatech.se> 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 }, > > > + }; > > > > > > > This will break the OV5647 sensor usage with Raspberry Pi without > > referring back to a sensor database . OV5647 has a delay of 2 for gain > and > > exposure, so will not work with the above hard-coded settings. > > I understand your point, and we really should get started with a > solution on how to get sensor specific data into CameraSensor database. > > But with this series there is no problem for the Raspberry Pi pipeline > as it does not deal with controls on the CameraSensor but the V4L2 video > device. Therefor the RPi pipeline creates a private instance of > DelayedControls witch delays still are initialized by the RPi IPA, just > as things worked before using StaggerdCtrls. > > When developing this I ran DelayedControls and StaggerdCtrls in parallel > on the RPi and the output of both matched perfectly frame by frame. > Yes, I see that now. So there should be no problems for pipeline handlers to allocate a DelayedControl for its own use? In that case, the above should not be a problem. But you are correct, this does show the need to get a central sensor database available soon. > > > > > In a more general sense, I wonder if initialising the DelayedControl here > > is the right thing to do? I have some changes waiting to be submitted > for > > review that add framerate control to the Raspberry Pi stack. In this > > change, I've added a VBLANK control to the staggered writer. There are > > also some changes to control very long exposures on the imx477 that go > > through the staggered writer. In these cases, I would have to add values > > to the map above, but other pipeline handlers would never need to use > > them. This may be a bit wasteful, I don't know... Would it make more > > sense for the pipeline handlers to initialise the DelayedControls as they > > need to? > > First, we are still thinking about how VBLANK and other controls that > can modify the frame length are to be modelled in the CameraSensor. If > you have any ideas or tips in this area I would greatly appreciate them. > I could possibly share with you (privately for now, to avoid too much noise on this mailing list) my changes to add VBLANK control, to discuss what I have done? > > I really think CameraSensor is the location to initialising the delays, > this separates the sensor from the pipeline. I think this is important > as otherwise each pipeline will have have a list of sensors it knows > about and will not function popery with different ones. By "forcing" > pipelines to learn about the sensor used thru CameraSensor interface we > ensure that we can add more sensors in the future without having to > update all pipelines and/or IPAs. This is even more important if we > consider closed-source IPAs as we can't just add a new sensor with > timings recompile. > > Yet again as the Raspberry Pi pipeline is video device centric it can't > really use the CameraSensor to deal with controls and must instead > create and configure its local version of DelayedControls. I still think > in the future such pipelines can benefit from a sensor database in > CameraSensor as it should be possible to query the datbase and use the > delays reported while still creating a private local DelayedControls. > Yes, I agree with this. Just want to raise the point that some sensors will have controls not available to others, e.g. imx477 long exposure settings. So it would be good to have as much flexibility in the sensor database to allow for these variations. > > Does this relive your worries bout this series? > Indeed it does. I will get to reviewing the full patchset shortly and send over my comments. Best regards, Naush > > > > > Regards, > > Naush > > > > > > > > > + > > > + 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.1 > > > > > > > > -- > Regards, > Niklas Söderlund >
Hi Naushir, On 2020-10-29 10:58:19 +0000, Naushir Patuck wrote: > Hi Niklas, > > Thank you for the clarification. > > On Wed, 28 Oct 2020 at 13:26, Niklas Söderlund < > niklas.soderlund@ragnatech.se> wrote: > > > Hi Naushir, > > > > Thanks for your feedback. > > > > On 2020-10-28 11:51:13 +0000, Naushir Patuck wrote: > > > Hi Niklas, > > > > > > Thank you for the changes. I will go through all the patches with review > > > comment in due course, but wanted to raise a critical point below: > > > > > > On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund < > > > niklas.soderlund@ragnatech.se> 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 }, > > > > + }; > > > > > > > > > > This will break the OV5647 sensor usage with Raspberry Pi without > > > referring back to a sensor database . OV5647 has a delay of 2 for gain > > and > > > exposure, so will not work with the above hard-coded settings. > > > > I understand your point, and we really should get started with a > > solution on how to get sensor specific data into CameraSensor database. > > > > But with this series there is no problem for the Raspberry Pi pipeline > > as it does not deal with controls on the CameraSensor but the V4L2 video > > device. Therefor the RPi pipeline creates a private instance of > > DelayedControls witch delays still are initialized by the RPi IPA, just > > as things worked before using StaggerdCtrls. > > > > When developing this I ran DelayedControls and StaggerdCtrls in parallel > > on the RPi and the output of both matched perfectly frame by frame. > > > > Yes, I see that now. So there should be no problems for pipeline handlers > to allocate a DelayedControl for its own use? In that case, the above > should not be a problem. But you are correct, this does show the need to > get a central sensor database available soon. I don't think there is going to be a problem for pipelines who are not using CameraSensor to manage their sensor no. I do hope we given time can move the Raspberry Pi pipeline to be media device centric so it don't *have* to instantiate its own DelayedControl, but that is a separate discussion ;-P > > > > > > > > > > In a more general sense, I wonder if initialising the DelayedControl here > > > is the right thing to do? I have some changes waiting to be submitted > > for > > > review that add framerate control to the Raspberry Pi stack. In this > > > change, I've added a VBLANK control to the staggered writer. There are > > > also some changes to control very long exposures on the imx477 that go > > > through the staggered writer. In these cases, I would have to add values > > > to the map above, but other pipeline handlers would never need to use > > > them. This may be a bit wasteful, I don't know... Would it make more > > > sense for the pipeline handlers to initialise the DelayedControls as they > > > need to? > > > > First, we are still thinking about how VBLANK and other controls that > > can modify the frame length are to be modelled in the CameraSensor. If > > you have any ideas or tips in this area I would greatly appreciate them. > > > > I could possibly share with you (privately for now, to avoid too much noise > on this mailing list) my changes to add VBLANK control, to discuss what I > have done? That would be interesting if you have something you feel is ready, no rush. I'm just starting to think about this problem so any input and feedback is good for me. > > > > > > I really think CameraSensor is the location to initialising the delays, > > this separates the sensor from the pipeline. I think this is important > > as otherwise each pipeline will have have a list of sensors it knows > > about and will not function popery with different ones. By "forcing" > > pipelines to learn about the sensor used thru CameraSensor interface we > > ensure that we can add more sensors in the future without having to > > update all pipelines and/or IPAs. This is even more important if we > > consider closed-source IPAs as we can't just add a new sensor with > > timings recompile. > > > > Yet again as the Raspberry Pi pipeline is video device centric it can't > > really use the CameraSensor to deal with controls and must instead > > create and configure its local version of DelayedControls. I still think > > in the future such pipelines can benefit from a sensor database in > > CameraSensor as it should be possible to query the datbase and use the > > delays reported while still creating a private local DelayedControls. > > > > Yes, I agree with this. Just want to raise the point that some sensors > will have controls not available to others, e.g. imx477 long exposure > settings. So it would be good to have as much flexibility in the sensor > database to allow for these variations. Agreed. One goal I hope we can achieve is that all such information can be enumerated on the CameraSensor as controls/values/properties/flags. I would if possible try to avoid pipelines and IPAs making decisions based on the sensor model name if it can be described as a property of the sensor. But maybe that will not be possible in all cases. > > > > > > Does this relive your worries bout this series? > > > > Indeed it does. I will get to reviewing the full patchset shortly and send > over my comments. Thanks! > > Best regards, > Naush > > > > > > > > > > Regards, > > > Naush > > > > > > > > > > > > > + > > > > + 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.1 > > > > > > > > > > > > -- > > 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(+)