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

[Daniel Scally]

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

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