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

[Daniel Scally]

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 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?

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