| Message ID | 20211203224230.38700-5-djrscally@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, Thank you for the patch. On Fri, Dec 03, 2021 at 10:42:27PM +0000, Daniel Scally wrote: > Add a function to the CameraLens class to fetch the V4L2 controls > for its V4L2 subdev > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - New patch > > include/libcamera/internal/camera_lens.h | 4 ++++ > src/libcamera/camera_lens.cpp | 11 +++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h > index 6f2ea1bc..64794294 100644 > --- a/include/libcamera/internal/camera_lens.h > +++ b/include/libcamera/internal/camera_lens.h > @@ -12,6 +12,8 @@ > #include <libcamera/base/class.h> > #include <libcamera/base/log.h> > > +#include <libcamera/controls.h> > + > namespace libcamera { > > class MediaEntity; > @@ -28,6 +30,8 @@ public: > > const std::string &model() const { return model_; } > > + const ControlInfoMap &controls() const; > + > protected: > std::string logPrefix() const override; > > diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp > index 189cb025..7b3b9c24 100644 > --- a/src/libcamera/camera_lens.cpp > +++ b/src/libcamera/camera_lens.cpp > @@ -139,4 +139,15 @@ std::string CameraLens::logPrefix() const > return "'" + entity_->name() + "'"; > } > > +/** > + * \fn CameraLens::controls() > + * \brief Retrieve the V4L2 controls of the lens' subdev > + * > + * \return A map of the V4L2 controls supported by the sensor Not a sensor anymore. With that fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I however would like to take one more step, and was wondering if you'd like to volunteer for it. I think the CameraSensor and CameraLens classes shouldn't expose V4L2 controls, but a custom set of controls specific to libcamera. This way we could remove the dependencies on V4L2 in the IPA modules. The translation between those controls and the V4L2 controls would be local to the CameraSensor and CameraLens classes. We would need three controls at the moment, for the analogue gain, exposure time, and focus position. It could be a good idea to decouple those controls from the libcamera public controls (exposed by cameras to applications), but given that we have libcamera controls for those three already, we could start by using them and see if we need something else later. I'm also wondering if we could, this way, combine the sensor controls and lens controls in a single ControlInfoMap passed to the IPA, and a single ControlList passed back from the IPA. > + */ > +const ControlInfoMap &CameraLens::controls() const > +{ > + return subdev_->controls(); > +} > + > } /* namespace libcamera */
Hi Laurent On 04/12/2021 00:36, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Fri, Dec 03, 2021 at 10:42:27PM +0000, Daniel Scally wrote: >> Add a function to the CameraLens class to fetch the V4L2 controls >> for its V4L2 subdev >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v2: >> >> - New patch >> >> include/libcamera/internal/camera_lens.h | 4 ++++ >> src/libcamera/camera_lens.cpp | 11 +++++++++++ >> 2 files changed, 15 insertions(+) >> >> diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h >> index 6f2ea1bc..64794294 100644 >> --- a/include/libcamera/internal/camera_lens.h >> +++ b/include/libcamera/internal/camera_lens.h >> @@ -12,6 +12,8 @@ >> #include <libcamera/base/class.h> >> #include <libcamera/base/log.h> >> >> +#include <libcamera/controls.h> >> + >> namespace libcamera { >> >> class MediaEntity; >> @@ -28,6 +30,8 @@ public: >> >> const std::string &model() const { return model_; } >> >> + const ControlInfoMap &controls() const; >> + >> protected: >> std::string logPrefix() const override; >> >> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp >> index 189cb025..7b3b9c24 100644 >> --- a/src/libcamera/camera_lens.cpp >> +++ b/src/libcamera/camera_lens.cpp >> @@ -139,4 +139,15 @@ std::string CameraLens::logPrefix() const >> return "'" + entity_->name() + "'"; >> } >> >> +/** >> + * \fn CameraLens::controls() >> + * \brief Retrieve the V4L2 controls of the lens' subdev >> + * >> + * \return A map of the V4L2 controls supported by the sensor > > Not a sensor anymore. With that fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks, and for the others too. > > I however would like to take one more step, and was wondering if you'd > like to volunteer for it. Sure, I'd be happy to. > I think the CameraSensor and CameraLens classes shouldn't expose V4L2 > controls, but a custom set of controls specific to libcamera. This way > we could remove the dependencies on V4L2 in the IPA modules. The > translation between those controls and the V4L2 controls would be local > to the CameraSensor and CameraLens classes. Makes sense; but what's the advantage to removing the dependency? Is there another means of controlling sensor drivers beyond the V4L2 API that could then be supported? > > We would need three controls at the moment, for the analogue gain, > exposure time, and focus position. It could be a good idea to decouple > those controls from the libcamera public controls (exposed by cameras to > applications), but given that we have libcamera controls for those three > already, we could start by using them and see if we need something else > later. > > I'm also wondering if we could, this way, combine the sensor controls > and lens controls in a single ControlInfoMap passed to the IPA, and a > single ControlList passed back from the IPA. Yeah I thought this could do with combining somehow too; I'll take a look. > >> + */ >> +const ControlInfoMap &CameraLens::controls() const >> +{ >> + return subdev_->controls(); >> +} >> + >> } /* namespace libcamera */ >
Hi Daniel, On Sat, Dec 04, 2021 at 09:55:38PM +0000, Daniel Scally wrote: > On 04/12/2021 00:36, Laurent Pinchart wrote: > > On Fri, Dec 03, 2021 at 10:42:27PM +0000, Daniel Scally wrote: > >> Add a function to the CameraLens class to fetch the V4L2 controls > >> for its V4L2 subdev > >> > >> Signed-off-by: Daniel Scally <djrscally@gmail.com> > >> --- > >> Changes in v2: > >> > >> - New patch > >> > >> include/libcamera/internal/camera_lens.h | 4 ++++ > >> src/libcamera/camera_lens.cpp | 11 +++++++++++ > >> 2 files changed, 15 insertions(+) > >> > >> diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h > >> index 6f2ea1bc..64794294 100644 > >> --- a/include/libcamera/internal/camera_lens.h > >> +++ b/include/libcamera/internal/camera_lens.h > >> @@ -12,6 +12,8 @@ > >> #include <libcamera/base/class.h> > >> #include <libcamera/base/log.h> > >> > >> +#include <libcamera/controls.h> > >> + > >> namespace libcamera { > >> > >> class MediaEntity; > >> @@ -28,6 +30,8 @@ public: > >> > >> const std::string &model() const { return model_; } > >> > >> + const ControlInfoMap &controls() const; > >> + > >> protected: > >> std::string logPrefix() const override; > >> > >> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp > >> index 189cb025..7b3b9c24 100644 > >> --- a/src/libcamera/camera_lens.cpp > >> +++ b/src/libcamera/camera_lens.cpp > >> @@ -139,4 +139,15 @@ std::string CameraLens::logPrefix() const > >> return "'" + entity_->name() + "'"; > >> } > >> > >> +/** > >> + * \fn CameraLens::controls() > >> + * \brief Retrieve the V4L2 controls of the lens' subdev > >> + * > >> + * \return A map of the V4L2 controls supported by the sensor > > > > Not a sensor anymore. With that fixed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks, and for the others too. > > > I however would like to take one more step, and was wondering if you'd > > like to volunteer for it. > > Sure, I'd be happy to. > > > I think the CameraSensor and CameraLens classes shouldn't expose V4L2 > > controls, but a custom set of controls specific to libcamera. This way > > we could remove the dependencies on V4L2 in the IPA modules. The > > translation between those controls and the V4L2 controls would be local > > to the CameraSensor and CameraLens classes. > > Makes sense; but what's the advantage to removing the dependency? Is > there another means of controlling sensor drivers beyond the V4L2 API > that could then be supported? At the moment there isn't, but that shouldn't prevent us from getting ready for the future :-) Even with V4L2, I could very well imagine improving the controls dealing with sensor timings (VBLANK is really painful to deal with), with API differences handled in the CameraSensor class. Future predictions aside, another advantage would be to isolate all the code dealing with V4L2 on the pipeline handler side (as opposed to the IPA side). IPA code would have less dependencies on the sensor, and would thus be more generic and reusable. It could also possibly allow us (us noted below) to group sensor and lens controls in a single control list. This is a topic for research, as there may be drawbacks in trying to abstract the sensor interface. I'm not entirely sure what the right balance would be. > > We would need three controls at the moment, for the analogue gain, > > exposure time, and focus position. It could be a good idea to decouple > > those controls from the libcamera public controls (exposed by cameras to > > applications), but given that we have libcamera controls for those three > > already, we could start by using them and see if we need something else > > later. > > > > I'm also wondering if we could, this way, combine the sensor controls > > and lens controls in a single ControlInfoMap passed to the IPA, and a > > single ControlList passed back from the IPA. > > Yeah I thought this could do with combining somehow too; I'll take a look. > > >> + */ > >> +const ControlInfoMap &CameraLens::controls() const > >> +{ > >> + return subdev_->controls(); > >> +} > >> + > >> } /* namespace libcamera */
Hi Laurent On 07/12/2021 18:53, Laurent Pinchart wrote: > Hi Daniel, > > On Sat, Dec 04, 2021 at 09:55:38PM +0000, Daniel Scally wrote: >> On 04/12/2021 00:36, Laurent Pinchart wrote: >>> On Fri, Dec 03, 2021 at 10:42:27PM +0000, Daniel Scally wrote: >>>> Add a function to the CameraLens class to fetch the V4L2 controls >>>> for its V4L2 subdev >>>> >>>> Signed-off-by: Daniel Scally <djrscally@gmail.com> >>>> --- >>>> Changes in v2: >>>> >>>> - New patch >>>> >>>> include/libcamera/internal/camera_lens.h | 4 ++++ >>>> src/libcamera/camera_lens.cpp | 11 +++++++++++ >>>> 2 files changed, 15 insertions(+) >>>> >>>> diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h >>>> index 6f2ea1bc..64794294 100644 >>>> --- a/include/libcamera/internal/camera_lens.h >>>> +++ b/include/libcamera/internal/camera_lens.h >>>> @@ -12,6 +12,8 @@ >>>> #include <libcamera/base/class.h> >>>> #include <libcamera/base/log.h> >>>> >>>> +#include <libcamera/controls.h> >>>> + >>>> namespace libcamera { >>>> >>>> class MediaEntity; >>>> @@ -28,6 +30,8 @@ public: >>>> >>>> const std::string &model() const { return model_; } >>>> >>>> + const ControlInfoMap &controls() const; >>>> + >>>> protected: >>>> std::string logPrefix() const override; >>>> >>>> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp >>>> index 189cb025..7b3b9c24 100644 >>>> --- a/src/libcamera/camera_lens.cpp >>>> +++ b/src/libcamera/camera_lens.cpp >>>> @@ -139,4 +139,15 @@ std::string CameraLens::logPrefix() const >>>> return "'" + entity_->name() + "'"; >>>> } >>>> >>>> +/** >>>> + * \fn CameraLens::controls() >>>> + * \brief Retrieve the V4L2 controls of the lens' subdev >>>> + * >>>> + * \return A map of the V4L2 controls supported by the sensor >>> >>> Not a sensor anymore. With that fixed, >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Thanks, and for the others too. >> >>> I however would like to take one more step, and was wondering if you'd >>> like to volunteer for it. >> >> Sure, I'd be happy to. >> >>> I think the CameraSensor and CameraLens classes shouldn't expose V4L2 >>> controls, but a custom set of controls specific to libcamera. This way >>> we could remove the dependencies on V4L2 in the IPA modules. The >>> translation between those controls and the V4L2 controls would be local >>> to the CameraSensor and CameraLens classes. >> >> Makes sense; but what's the advantage to removing the dependency? Is >> there another means of controlling sensor drivers beyond the V4L2 API >> that could then be supported? > > At the moment there isn't, but that shouldn't prevent us from getting > ready for the future :-) Even with V4L2, I could very well imagine > improving the controls dealing with sensor timings (VBLANK is really > painful to deal with), with API differences handled in the CameraSensor > class. > > Future predictions aside, another advantage would be to isolate all the > code dealing with V4L2 on the pipeline handler side (as opposed to the > IPA side). IPA code would have less dependencies on the sensor, and > would thus be more generic and reusable. It could also possibly allow us > (us noted below) to group sensor and lens controls in a single control > list. > > This is a topic for research, as there may be drawbacks in trying to > abstract the sensor interface. I'm not entirely sure what the right > balance would be. Thanks for the explanation; makes sense to me. I'll start taking a look at that amongst the other stuff I'm working on. >>> We would need three controls at the moment, for the analogue gain, >>> exposure time, and focus position. It could be a good idea to decouple >>> those controls from the libcamera public controls (exposed by cameras to >>> applications), but given that we have libcamera controls for those three >>> already, we could start by using them and see if we need something else >>> later. >>> >>> I'm also wondering if we could, this way, combine the sensor controls >>> and lens controls in a single ControlInfoMap passed to the IPA, and a >>> single ControlList passed back from the IPA. >> >> Yeah I thought this could do with combining somehow too; I'll take a look. >> >>>> + */ >>>> +const ControlInfoMap &CameraLens::controls() const >>>> +{ >>>> + return subdev_->controls(); >>>> +} >>>> + >>>> } /* namespace libcamera */ >
diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h index 6f2ea1bc..64794294 100644 --- a/include/libcamera/internal/camera_lens.h +++ b/include/libcamera/internal/camera_lens.h @@ -12,6 +12,8 @@ #include <libcamera/base/class.h> #include <libcamera/base/log.h> +#include <libcamera/controls.h> + namespace libcamera { class MediaEntity; @@ -28,6 +30,8 @@ public: const std::string &model() const { return model_; } + const ControlInfoMap &controls() const; + protected: std::string logPrefix() const override; diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp index 189cb025..7b3b9c24 100644 --- a/src/libcamera/camera_lens.cpp +++ b/src/libcamera/camera_lens.cpp @@ -139,4 +139,15 @@ std::string CameraLens::logPrefix() const return "'" + entity_->name() + "'"; } +/** + * \fn CameraLens::controls() + * \brief Retrieve the V4L2 controls of the lens' subdev + * + * \return A map of the V4L2 controls supported by the sensor + */ +const ControlInfoMap &CameraLens::controls() const +{ + return subdev_->controls(); +} + } /* namespace libcamera */
Add a function to the CameraLens class to fetch the V4L2 controls for its V4L2 subdev Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - New patch include/libcamera/internal/camera_lens.h | 4 ++++ src/libcamera/camera_lens.cpp | 11 +++++++++++ 2 files changed, 15 insertions(+)