[libcamera-devel] [PATCH 10/11] libcamera: pipeline: ipu3: Use the new CameraSensor class

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2019-04-15 19:56:59 +0300, Laurent Pinchart wrote:
> Replace the manual handling of sensor formats with usage of the new
> CameraSensor class.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

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

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 78 +++++-----------------------
>  1 file changed, 14 insertions(+), 64 deletions(-)

I just love to see code melt away from pipeline handler implementations 
to helpers.

> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index e3c79f93963e..1326a1be105f 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -15,6 +15,7 @@
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> +#include "camera_sensor.h"
>  #include "device_enumerator.h"
>  #include "log.h"
>  #include "media_device.h"
> @@ -124,11 +125,7 @@ public:
>  
>  	V4L2Device *output_;
>  	V4L2Subdevice *csi2_;
> -	V4L2Subdevice *sensor_;
> -
> -	/* Maximum sizes and the mbus code used to produce them. */
> -	unsigned int mbusCode_;
> -	Size maxSize_;
> +	CameraSensor *sensor_;
>  
>  	BufferPool pool_;
>  };
> @@ -255,8 +252,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  		return -EINVAL;
>  	}
>  
> -	if (cfg.width > cio2->maxSize_.width ||
> -	    cfg.height > cio2->maxSize_.height) {
> +	const Size &resolution = cio2->sensor_->resolution();
> +	if (cfg.width > resolution.width ||
> +	    cfg.height > resolution.height) {
>  		LOG(IPU3, Error)
>  			<< "Invalid stream size: larger than sensor resolution";
>  		return -EINVAL;
> @@ -1036,31 +1034,11 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  	 * \todo Define when to open and close video device nodes, as they
>  	 * might impact on power consumption.
>  	 */
> -	sensor_ = new V4L2Subdevice(sensorEntity);
> -	ret = sensor_->open();
> +	sensor_ = new CameraSensor(sensorEntity);
> +	ret = sensor_->init();
>  	if (ret)
>  		return ret;
>  
> -	for (auto it : sensor_->formats(0)) {
> -		int mbusCode = mediaBusToFormat(it.first);
> -		if (mbusCode < 0)
> -			continue;
> -
> -		for (const SizeRange &size : it.second) {
> -			if (maxSize_.width < size.max.width &&
> -			    maxSize_.height < size.max.height) {
> -				maxSize_ = size.max;
> -				mbusCode_ = mbusCode;
> -			}
> -		}
> -	}
> -	if (maxSize_.width == 0) {
> -		LOG(IPU3, Info) << "Sensor '" << sensor_->entity()->name()
> -				<< "' detected, but no supported image format "
> -				<< " found: skip camera creation";
> -		return -ENODEV;
> -	}
> -
>  	csi2_ = new V4L2Subdevice(csi2Entity);
>  	ret = csi2_->open();
>  	if (ret)
> @@ -1085,47 +1063,19 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  int CIO2Device::configure(const StreamConfiguration &config,
>  			  V4L2DeviceFormat *outputFormat)
>  {
> -	unsigned int imageSize = config.width * config.height;
> -	V4L2SubdeviceFormat sensorFormat = {};
> -	unsigned int best = ~0;
> +	V4L2SubdeviceFormat sensorFormat;
>  	int ret;
>  
> -	for (auto it : sensor_->formats(0)) {
> -		/* Only consider formats consumable by the CIO2 unit. */
> -		if (mediaBusToFormat(it.first) < 0)
> -			continue;
> -
> -		for (const SizeRange &size : it.second) {
> -			/*
> -			 * Only select formats bigger than the requested sizes
> -			 * as the IPU3 cannot up-scale.
> -			 *
> -			 * \todo: Unconditionally scale on the sensor as much
> -			 * as possible. This will need to be revisited when
> -			 * implementing the scaling policy.
> -			 */
> -			if (size.max.width < config.width ||
> -			    size.max.height < config.height)
> -				continue;
> -
> -			unsigned int diff = size.max.width * size.max.height
> -					  - imageSize;
> -			if (diff >= best)
> -				continue;
> -
> -			best = diff;
> -
> -			sensorFormat.width = size.max.width;
> -			sensorFormat.height = size.max.height;
> -			sensorFormat.mbus_code = it.first;
> -		}
> -	}
> -
>  	/*
>  	 * Apply the selected format to the sensor, the CSI-2 receiver and
>  	 * the CIO2 output device.
>  	 */
> -	ret = sensor_->setFormat(0, &sensorFormat);
> +	sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> +					    MEDIA_BUS_FMT_SGBRG10_1X10,
> +					    MEDIA_BUS_FMT_SGRBG10_1X10,
> +					    MEDIA_BUS_FMT_SRGGB10_1X10 },
> +					  Size(config.width, config.height));
> +	ret = sensor_->setFormat(&sensorFormat);
>  	if (ret)
>  		return ret;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund