[libcamera-devel] [PATCH v2 4/7] libcamera: camera_lens: Add function to fetch subdev controls

[Laurent Pinchart]

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 at 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 at 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 */

-- 
Regards,

Laurent Pinchart