[libcamera-devel] [PATCH v2 3/5] libcamera: camera_sensor: Rename controls() method

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2020-12-28 17:55:58 +0100, Jacopo Mondi wrote:
> The CameraSensor::controls() methods returns the information relative to
> the V4L2 controls registered by the sensor sub-device driver. Its
> current use is to inform the IPA module of two pipelines (RkISP1 and
> VIMC) about the V4L2 controls limits.
> 
> The CameraSensor class has a controls_ field, which is instead the
> ControlInfoMap of libcamera controls registered by the CameraSensor
> class and meant to be exported as Camera controls.
> 
> To prepare to register libcamera controls in the CameraSensor::controls_
> info map, and remove any ambiguity on the intended usage of
> CameraSensor::controls(), rename the method in
> CameraSensor::subdevControls() and update its users in the code base.
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

The patch in itself is good but I'm not sure I understand the direction 
we are taking with CameraSensor. I understand we should not expose any 
V4L2 controls to applications but from the pipeline to core helpers (and 
IPA) do we really wish to use libcamera controls? I can't see that we 
will have core helpers or IPA that would work with both V4L2 and 
something new. In my view such interfaces should be kept as close to the 
drivers it interacts with, in this case V4L2.

I might be missing some use-case where mixing libcamera and V4L2 
controls in helpers and lowlevel interfaces are desired but I can't 
really see it. At first I thought maybe we would have some controls that 
simply could be passed from application to CameraSensor but for all 
cases I can think of I can see the possibility for an IPA to be involved 
and then I get confused as we mix and match between the two already too 
much in my taste ;-)

Maybe the right solution is to move even further in the direction taken 
here with the goal to not expose V4L2 controls outside the V4L2 classes.  
In either case I would like to understand what we aim for.

For this rename change alone,

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  include/libcamera/internal/camera_sensor.h | 2 +-
>  src/libcamera/camera_sensor.cpp            | 6 +++---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp   | 2 +-
>  src/libcamera/pipeline/vimc/vimc.cpp       | 4 ++--
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 0357b2a630f7..841c7f4bef0f 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -57,7 +57,7 @@ public:
>  				      const Size &size) const;
>  	int setFormat(V4L2SubdeviceFormat *format);
>  
> -	const ControlInfoMap &controls() const;
> +	const ControlInfoMap &subdevControls() const;
>  	ControlList getControls(const std::vector<uint32_t> &ids);
>  	int setControls(ControlList *ctrls);
>  
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 18dc6d723e27..337d73c524bf 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -617,10 +617,10 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
>  }
>  
>  /**
> - * \brief Retrieve the supported V4L2 controls and their information
> - * \return A map of the V4L2 controls supported by the sensor
> + * \brief Retrieve the controls supported by the V4L2 subdev and their information
> + * \return A map of the V4L2 controls supported by the video subdevice
>   */
> -const ControlInfoMap &CameraSensor::controls() const
> +const ControlInfoMap &CameraSensor::subdevControls() const
>  {
>  	return subdev_->controls();
>  }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 5ce37febc1d7..628d1f39bfbd 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -906,7 +906,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>  	}
>  
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> -	entityControls.emplace(0, data->sensor_->controls());
> +	entityControls.emplace(0, data->sensor_->subdevControls());
>  
>  	IPAOperationData ipaConfig;
>  	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 36325ffbbd7d..d5f6e8a453ff 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -338,7 +338,7 @@ void PipelineHandlerVimc::stop(Camera *camera)
>  
>  int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
>  {
> -	ControlList controls(data->sensor_->controls());
> +	ControlList controls(data->sensor_->subdevControls());
>  
>  	for (auto it : request->controls()) {
>  		unsigned int id = it.first;
> @@ -480,7 +480,7 @@ int VimcCameraData::init()
>  		return -ENODEV;
>  
>  	/* Initialise the supported controls. */
> -	const ControlInfoMap &controls = sensor_->controls();
> +	const ControlInfoMap &controls = sensor_->subdevControls();
>  	ControlInfoMap::Map ctrls;
>  
>  	for (const auto &ctrl : controls) {
> -- 
> 2.29.2
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund