[libcamera-devel,v3,12/13] libcamera: pipeline: ipu3: Use the new CameraSensor class
diff mbox series

Message ID 20190418141437.14014-13-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Rockchip ISP pipeline handler
Related show

Commit Message

Laurent Pinchart April 18, 2019, 2:14 p.m. UTC 
Replace the manual handling of sensor formats with usage of the new
CameraSensor class.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
Changes since v2:

- Properly sort the CIO2 supported media bus codes numerically

Changes since v1:

- Move sensor entity function check to CameraSensor class
- Verify sensor media bus codes at init time
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 107 +++++++++------------------
 1 file changed, 36 insertions(+), 71 deletions(-)

Comments

Jacopo Mondi April 18, 2019, 2:40 p.m. UTC | #1 
Hi Laurent,

On Thu, Apr 18, 2019 at 05:14:36PM +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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> Changes since v2:
>
> - Properly sort the CIO2 supported media bus codes numerically
>
> Changes since v1:
>
> - Move sensor entity function check to CameraSensor class
> - Verify sensor media bus codes at init time
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 107 +++++++++------------------
>  1 file changed, 36 insertions(+), 71 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f43969099b48..c677ee9b51da 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -5,6 +5,7 @@
>   * ipu3.cpp - Pipeline handler for Intel IPU3
>   */
>
> +#include <algorithm>

What do you need this for?

>  #include <iomanip>
>  #include <memory>
>  #include <vector>
> @@ -15,6 +16,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 +126,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_;
>  };
> @@ -257,8 +255,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;
> @@ -1032,45 +1031,39 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>
>  	MediaLink *link = links[0];
>  	MediaEntity *sensorEntity = link->source()->entity();
> -	if (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)
> -		return -ENODEV;
> +	sensor_ = new CameraSensor(sensorEntity);
> +	ret = sensor_->init();
> +	if (ret)
> +		return ret;
>
>  	ret = link->setEnabled(true);
>  	if (ret)
>  		return ret;
>
>  	/*
> -	 * Now that we're sure a sensor subdevice is connected, make sure it
> -	 * produces at least one image format compatible with CIO2 requirements
> -	 * and cache the camera maximum size.
> +	 * Make sure the sensor produces at least one format compatible with
> +	 * the CIO2 requirements.
>  	 *
> +	 * utils::set_overlap requires the ranges to be sorted, keep the
> +	 * cio2Codes vector sorted in ascending order.
> +	 */
> +	const std::vector<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> +						   MEDIA_BUS_FMT_SGRBG10_1X10,
> +						   MEDIA_BUS_FMT_SGBRG10_1X10,

As you mentioned you ordered them, shouldn't SGBR come before SGRB ?

Minors apart:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

I'll rebase my multiple stream support series on top of this patches!

Thanks
  j

> +						   MEDIA_BUS_FMT_SRGGB10_1X10 };
> +	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
> +	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> +				cio2Codes.begin(), cio2Codes.end())) {
> +		LOG(IPU3, Error)
> +			<< "Sensor " << sensor_->entity()->name()
> +			<< " has not format compatible with the IPU3";
> +		return -EINVAL;
> +	}
> +
> +	/*
>  	 * \todo Define when to open and close video device nodes, as they
>  	 * might impact on power consumption.
>  	 */
> -	sensor_ = new V4L2Subdevice(sensorEntity);
> -	ret = sensor_->open();
> -	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();
> @@ -1096,47 +1089,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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 18, 2019, 3:08 p.m. UTC | #2 
Hi Jacopo,

On Thu, Apr 18, 2019 at 04:40:52PM +0200, Jacopo Mondi wrote:
> On Thu, Apr 18, 2019 at 05:14:36PM +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@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> > Changes since v2:
> >
> > - Properly sort the CIO2 supported media bus codes numerically
> >
> > Changes since v1:
> >
> > - Move sensor entity function check to CameraSensor class
> > - Verify sensor media bus codes at init time
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 107 +++++++++------------------
> >  1 file changed, 36 insertions(+), 71 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index f43969099b48..c677ee9b51da 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -5,6 +5,7 @@
> >   * ipu3.cpp - Pipeline handler for Intel IPU3
> >   */
> >
> > +#include <algorithm>
> 
> What do you need this for?

We don't, I've removed it. It was a leftover.

> >  #include <iomanip>
> >  #include <memory>
> >  #include <vector>
> > @@ -15,6 +16,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 +126,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_;
> >  };
> > @@ -257,8 +255,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;
> > @@ -1032,45 +1031,39 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> >
> >  	MediaLink *link = links[0];
> >  	MediaEntity *sensorEntity = link->source()->entity();
> > -	if (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)
> > -		return -ENODEV;
> > +	sensor_ = new CameraSensor(sensorEntity);
> > +	ret = sensor_->init();
> > +	if (ret)
> > +		return ret;
> >
> >  	ret = link->setEnabled(true);
> >  	if (ret)
> >  		return ret;
> >
> >  	/*
> > -	 * Now that we're sure a sensor subdevice is connected, make sure it
> > -	 * produces at least one image format compatible with CIO2 requirements
> > -	 * and cache the camera maximum size.
> > +	 * Make sure the sensor produces at least one format compatible with
> > +	 * the CIO2 requirements.
> >  	 *
> > +	 * utils::set_overlap requires the ranges to be sorted, keep the
> > +	 * cio2Codes vector sorted in ascending order.
> > +	 */
> > +	const std::vector<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> > +						   MEDIA_BUS_FMT_SGRBG10_1X10,
> > +						   MEDIA_BUS_FMT_SGBRG10_1X10,
> 
> As you mentioned you ordered them, shouldn't SGBR come before SGRB ?

In alphabetical order, yes, in numerical order, no.
> 
> Minors apart:
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> I'll rebase my multiple stream support series on top of this patches!

Looking forward to that :-)

> > +						   MEDIA_BUS_FMT_SRGGB10_1X10 };
> > +	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > +	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> > +				cio2Codes.begin(), cio2Codes.end())) {
> > +		LOG(IPU3, Error)
> > +			<< "Sensor " << sensor_->entity()->name()
> > +			<< " has not format compatible with the IPU3";
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> >  	 * \todo Define when to open and close video device nodes, as they
> >  	 * might impact on power consumption.
> >  	 */
> > -	sensor_ = new V4L2Subdevice(sensorEntity);
> > -	ret = sensor_->open();
> > -	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();
> > @@ -1096,47 +1089,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;
> >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index f43969099b48..c677ee9b51da 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -5,6 +5,7 @@ 
  * ipu3.cpp - Pipeline handler for Intel IPU3
  */
 
+#include <algorithm>
 #include <iomanip>
 #include <memory>
 #include <vector>
@@ -15,6 +16,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 +126,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_;
 };
@@ -257,8 +255,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;
@@ -1032,45 +1031,39 @@  int CIO2Device::init(const MediaDevice *media, unsigned int index)
 
 	MediaLink *link = links[0];
 	MediaEntity *sensorEntity = link->source()->entity();
-	if (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)
-		return -ENODEV;
+	sensor_ = new CameraSensor(sensorEntity);
+	ret = sensor_->init();
+	if (ret)
+		return ret;
 
 	ret = link->setEnabled(true);
 	if (ret)
 		return ret;
 
 	/*
-	 * Now that we're sure a sensor subdevice is connected, make sure it
-	 * produces at least one image format compatible with CIO2 requirements
-	 * and cache the camera maximum size.
+	 * Make sure the sensor produces at least one format compatible with
+	 * the CIO2 requirements.
 	 *
+	 * utils::set_overlap requires the ranges to be sorted, keep the
+	 * cio2Codes vector sorted in ascending order.
+	 */
+	const std::vector<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
+						   MEDIA_BUS_FMT_SGRBG10_1X10,
+						   MEDIA_BUS_FMT_SGBRG10_1X10,
+						   MEDIA_BUS_FMT_SRGGB10_1X10 };
+	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
+	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
+				cio2Codes.begin(), cio2Codes.end())) {
+		LOG(IPU3, Error)
+			<< "Sensor " << sensor_->entity()->name()
+			<< " has not format compatible with the IPU3";
+		return -EINVAL;
+	}
+
+	/*
 	 * \todo Define when to open and close video device nodes, as they
 	 * might impact on power consumption.
 	 */
-	sensor_ = new V4L2Subdevice(sensorEntity);
-	ret = sensor_->open();
-	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();
@@ -1096,47 +1089,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;