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

[Laurent Pinchart]

Hi Jacopo,

On Tue, Apr 16, 2019 at 06:40:21PM +0200, Jacopo Mondi wrote:
> On Mon, Apr 15, 2019 at 07:56:59PM +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>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 78 +++++-----------------------
> >  1 file changed, 14 insertions(+), 64 deletions(-)
> >
> > 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));
> 
> over-paranoid me wonders if this has to be checked...

In practice it would be very weird to find a system with a sensor
connected to the IPU3 where the two would have no common format. I
however think it would make sense to check this, not here, but after
calling CameraSensor::init(). I'll do so.

> Very nice removing code from pipeline handlers
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +	ret = sensor_->setFormat(&sensorFormat);
> >  	if (ret)
> >  		return ret;
> >

-- 
Regards,

Laurent Pinchart