| Message ID | 20201123221234.485933-6-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:31PM +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'm debated. As I see this, the current interface will have to evolve, on top of this series. I think pipeline handler will not have to get a 'DelayedControls' object and interface with it, but rather the CameraSensor will use it internally for either: - all controls marked as delayed in the sensor database - controls initialized as delayed by the pipeline handler (which won't be sensor-agnostic anymore) The current implementation aims to be a drop-in replacement for StaggeredCtrl so I'm more than fine with the proposed enanchement being performed on top. At the same time as you know this specific bits will break ov5647 capture on RPi, and I don't think that's right. As a possible plan I would propose: - replace staggered ctrls with this series and allow pipeline handler to pass in delays to maintain 1-to-1 compatibility with the existing - grow the sensor database and the CameraSensor interface to remove direct pipeline->DelayedControl interactions on top Am I missing a valid use case for pipeline handler having to deal with DelayedControls directly ? > + > + 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 and Niklas, On Fri, Nov 27, 2020 at 10:56:08AM +0100, Jacopo Mondi wrote: > On Mon, Nov 23, 2020 at 11:12:31PM +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(); s/delayedContols/delayedControls/ > > + > > 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'm debated. > > As I see this, the current interface will have to evolve, on top of > this series. I think pipeline handler will not have to get a > 'DelayedControls' object and interface with it, but rather the > CameraSensor will use it internally for either: > - all controls marked as delayed in the sensor database > - controls initialized as delayed by the pipeline handler (which won't > be sensor-agnostic anymore) > > The current implementation aims to be a drop-in replacement for > StaggeredCtrl so I'm more than fine with the proposed enanchement > being performed on top. At the same time as you know this specific > bits will break ov5647 capture on RPi, and I don't think that's right. > > As a possible plan I would propose: > - replace staggered ctrls with this series and allow pipeline handler > to pass in delays to maintain 1-to-1 compatibility with the existing > - grow the sensor database and the CameraSensor interface to remove > direct pipeline->DelayedControl interactions on top > > Am I missing a valid use case for pipeline handler having to deal with > DelayedControls directly ? I'm also wondering about the same. The main use case is indeed the CameraSensor class. I wouldn't rule out other potential users, which would be nicely served by the fact that the DelayedControls class can be instantiated separately from CameraSensor, but I'm not sure what those would be. In order to make the pipeline handler's life as easy as possible, I'd like to completely hide the DelayedControl instance in the CameraSensor class. This assumes that there would be no reason for a pipeline handler to deal with internals of delayed controls, ever, which may not always be true (maybe we'll need hacks in some cases, or maybe there are some clever use cases I can't think about now, but I can't see how those would be pipeline handler-specific, so implementing them in CameraSensor would make most sense to me at this point). How about leaving this patch out for now, and moving delayed controls handling in CameraSensor in a second step ? Nobody uses this function in this series :-) > > + > > + 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
On Sat, Dec 05, 2020 at 03:39:12AM +0200, Laurent Pinchart wrote: > Hi Jacopo and Niklas, > > On Fri, Nov 27, 2020 at 10:56:08AM +0100, Jacopo Mondi wrote: > > On Mon, Nov 23, 2020 at 11:12:31PM +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(); > > s/delayedContols/delayedControls/ > > > > + > > > 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'm debated. > > > > As I see this, the current interface will have to evolve, on top of > > this series. I think pipeline handler will not have to get a > > 'DelayedControls' object and interface with it, but rather the > > CameraSensor will use it internally for either: > > - all controls marked as delayed in the sensor database > > - controls initialized as delayed by the pipeline handler (which won't > > be sensor-agnostic anymore) > > > > The current implementation aims to be a drop-in replacement for > > StaggeredCtrl so I'm more than fine with the proposed enanchement > > being performed on top. At the same time as you know this specific > > bits will break ov5647 capture on RPi, and I don't think that's right. > > > > As a possible plan I would propose: > > - replace staggered ctrls with this series and allow pipeline handler > > to pass in delays to maintain 1-to-1 compatibility with the existing > > - grow the sensor database and the CameraSensor interface to remove > > direct pipeline->DelayedControl interactions on top > > > > Am I missing a valid use case for pipeline handler having to deal with > > DelayedControls directly ? > > I'm also wondering about the same. The main use case is indeed the > CameraSensor class. I wouldn't rule out other potential users, which > would be nicely served by the fact that the DelayedControls class can be > instantiated separately from CameraSensor, but I'm not sure what those > would be. > > In order to make the pipeline handler's life as easy as possible, I'd > like to completely hide the DelayedControl instance in the CameraSensor > class. This assumes that there would be no reason for a pipeline handler > to deal with internals of delayed controls, ever, which may not always > be true (maybe we'll need hacks in some cases, or maybe there are some > clever use cases I can't think about now, but I can't see how those > would be pipeline handler-specific, so implementing them in CameraSensor > would make most sense to me at this point). > > How about leaving this patch out for now, and moving delayed controls > handling in CameraSensor in a second step ? Nobody uses this function in > this series :-) I was wrong about that, it's used in the rkisp1 pipeline handler (and I assume by the IPU3 pipeline handler, on top of this series). Still, I think we could duplicate this code in those two pipeline handlers, and then move it to the CameraSensor class. > > > + > > > + 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
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(+)