[libcamera-devel,v2,03/14] libcamera: ipu3: Get default image sizes from sensor
diff mbox series

Message ID 20190312121242.2253-4-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: ipu3: ImgU support
Related show

Commit Message

Jacopo Mondi March 12, 2019, 12:12 p.m. UTC 
Inspect all image sizes provided by the sensor and select the
biggest of them, associated with an image format code supported by the
CIO2 unit.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 94 ++++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart March 13, 2019, 5:32 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Mar 12, 2019 at 01:12:31PM +0100, Jacopo Mondi wrote:
> Inspect all image sizes provided by the sensor and select the
> biggest of them, associated with an image format code supported by the
> CIO2 unit.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 94 ++++++++++++++++++++++++----
>  1 file changed, 82 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 55489c31df2d..0f18e4692e77 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -6,8 +6,11 @@
>   */
>  
>  #include <memory>
> +#include <iomanip>

Alphabetical order please.

>  #include <vector>
>  
> +#include <linux/media-bus-format.h>
> +
>  #include <libcamera/camera.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
> @@ -72,12 +75,16 @@ private:
>  		Stream stream_;
>  	};
>  
> +	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> +
>  	IPU3CameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<IPU3CameraData *>(
>  			PipelineHandler::cameraData(camera));
>  	}
>  
> +	int mediaBusToCIO2Format(unsigned int code);

s/;/const ;/

> +
>  	void registerCameras();
>  
>  	std::shared_ptr<MediaDevice> cio2_;
> @@ -102,26 +109,45 @@ std::map<Stream *, StreamConfiguration>
>  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  					 std::set<Stream *> &streams)
>  {
> -	IPU3CameraData *data = cameraData(camera);
>  	std::map<Stream *, StreamConfiguration> configs;
> -	V4L2SubdeviceFormat format = {};
> +	IPU3CameraData *data = cameraData(camera);
> +	V4L2Subdevice *sensor = data->sensor_;
> +	StreamConfiguration *config = &configs[&data->stream_];
>  
>  	/*
> -	 * FIXME: As of now, return the image format reported by the sensor.
> -	 * In future good defaults should be provided for each stream.
> +	 * Make sure the sensor produces a raw format compatible with the
> +	 * CIO2 and use the largest image size the sensor provides.
>  	 */
> -	if (data->sensor_->getFormat(0, &format)) {
> -		LOG(IPU3, Error) << "Failed to create stream configurations";
> +	const SubdevFormatEnum formats = sensor->formats(0);
> +	for (auto it = formats.begin(); it != formats.end(); ++it) {
> +		int cio2Code = mediaBusToCIO2Format(it->first);
> +		if (cio2Code == -EINVAL)
> +			continue;
> +
> +		for (const SizeRange &range : it->second) {
> +			if (range.maxWidth <= config->width ||
> +			    range.maxHeight <= config->height)
> +				continue;
> +
> +			config->width = range.maxWidth;
> +			config->height = range.maxHeight;
> +			config->pixelFormat = cio2Code;
> +		}

Didn't we decide to use the area instead of the width and height ?

> +	}
> +
> +	/* If not suitable format has been found, return an empty config. */
> +	if (!config->pixelFormat) {

This relies on config being zeroed when constructed, which isn't
guaranteed as far as I can tell.

> +		LOG(IPU3, Error) << "Sensor image format not supported";
> +
>  		return configs;
>  	}

Shouldn't this be done at match time and cached ? We shouldn't create a
camera whose sensor can't provide a supported format.

>  
> -	StreamConfiguration config = {};
> -	config.width = format.width;
> -	config.height = format.height;
> -	config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> -	config.bufferCount = 4;
> +	config->bufferCount = IPU3_BUFFER_COUNT;
>  
> -	configs[&data->stream_] = config;
> +	LOG(IPU3, Debug)
> +		<< "Stream format set to = (" << config->width << "x"
> +		<< config->height << ") - 0x" << std::hex << std::setfill('0')
> +		<< std::setw(8) << config->pixelFormat;
>  
>  	return configs;
>  }
> @@ -327,6 +353,50 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  	return true;
>  }
>  
> +int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)
> +{
> +	switch(code) {
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE:
> +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE:
> +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE:
> +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE:
> +	case MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> +	case MEDIA_BUS_FMT_SBGGR14_1X14:
> +	case MEDIA_BUS_FMT_SBGGR16_1X16:
> +		return V4L2_PIX_FMT_IPU3_SBGGR10;
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> +	case MEDIA_BUS_FMT_SGBRG14_1X14:
> +	case MEDIA_BUS_FMT_SGBRG16_1X16:
> +		return V4L2_PIX_FMT_IPU3_SGBRG10;
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> +	case MEDIA_BUS_FMT_SGRBG14_1X14:
> +	case MEDIA_BUS_FMT_SGRBG16_1X16:
> +		return V4L2_PIX_FMT_IPU3_SGRBG10;
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> +	case MEDIA_BUS_FMT_SRGGB14_1X14:
> +	case MEDIA_BUS_FMT_SRGGB16_1X16:
> +		return V4L2_PIX_FMT_IPU3_SRGGB10;

Does the CIO2 really support all these formats, especially the ALAW8,
DPCM8 and > 10bpp formats ?

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  /*
>   * Cameras are created associating an image sensor (represented by a
>   * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
Jacopo Mondi March 14, 2019, 9:52 a.m. UTC | #2 
Hi Laurent,

On Wed, Mar 13, 2019 at 07:32:24PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Mar 12, 2019 at 01:12:31PM +0100, Jacopo Mondi wrote:
> > Inspect all image sizes provided by the sensor and select the
> > biggest of them, associated with an image format code supported by the
> > CIO2 unit.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 94 ++++++++++++++++++++++++----
> >  1 file changed, 82 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 55489c31df2d..0f18e4692e77 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -6,8 +6,11 @@
> >   */
> >
> >  #include <memory>
> > +#include <iomanip>
>
> Alphabetical order please.
>

Trivial, sorry

> >  #include <vector>
> >
> > +#include <linux/media-bus-format.h>
> > +
> >  #include <libcamera/camera.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> > @@ -72,12 +75,16 @@ private:
> >  		Stream stream_;
> >  	};
> >
> > +	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > +
> >  	IPU3CameraData *cameraData(const Camera *camera)
> >  	{
> >  		return static_cast<IPU3CameraData *>(
> >  			PipelineHandler::cameraData(camera));
> >  	}
> >
> > +	int mediaBusToCIO2Format(unsigned int code);
>
> s/;/const ;/

Ack

>
> > +
> >  	void registerCameras();
> >
> >  	std::shared_ptr<MediaDevice> cio2_;
> > @@ -102,26 +109,45 @@ std::map<Stream *, StreamConfiguration>
> >  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> >  					 std::set<Stream *> &streams)
> >  {
> > -	IPU3CameraData *data = cameraData(camera);
> >  	std::map<Stream *, StreamConfiguration> configs;
> > -	V4L2SubdeviceFormat format = {};
> > +	IPU3CameraData *data = cameraData(camera);
> > +	V4L2Subdevice *sensor = data->sensor_;
> > +	StreamConfiguration *config = &configs[&data->stream_];
> >
> >  	/*
> > -	 * FIXME: As of now, return the image format reported by the sensor.
> > -	 * In future good defaults should be provided for each stream.
> > +	 * Make sure the sensor produces a raw format compatible with the
> > +	 * CIO2 and use the largest image size the sensor provides.
> >  	 */
> > -	if (data->sensor_->getFormat(0, &format)) {
> > -		LOG(IPU3, Error) << "Failed to create stream configurations";
> > +	const SubdevFormatEnum formats = sensor->formats(0);
> > +	for (auto it = formats.begin(); it != formats.end(); ++it) {
> > +		int cio2Code = mediaBusToCIO2Format(it->first);
> > +		if (cio2Code == -EINVAL)
> > +			continue;
> > +
> > +		for (const SizeRange &range : it->second) {
> > +			if (range.maxWidth <= config->width ||
> > +			    range.maxHeight <= config->height)
> > +				continue;
> > +
> > +			config->width = range.maxWidth;
> > +			config->height = range.maxHeight;
> > +			config->pixelFormat = cio2Code;
> > +		}
>
> Didn't we decide to use the area instead of the width and height ?
>

I'm using the area when looking for the best size approximations, but
here I'm looking for the largest available resolution, and thus I
would like to have both width and height larger than the default one.

I understand comparing area would work the same and catch some corner
cases I would miss here. I'll fix.

> > +	}
> > +
> > +	/* If not suitable format has been found, return an empty config. */
> > +	if (!config->pixelFormat) {
>
> This relies on config being zeroed when constructed, which isn't
> guaranteed as far as I can tell.
>

I assume it got zeroed by Camera, but that seems not the be the case.
I'll fix this too.

> > +		LOG(IPU3, Error) << "Sensor image format not supported";
> > +
> >  		return configs;
> >  	}
>
> Shouldn't this be done at match time and cached ? We shouldn't create a
> camera whose sensor can't provide a supported format.
>

That would make sense at camera registration time, yes, so I could
skip this.

> >
> > -	StreamConfiguration config = {};
> > -	config.width = format.width;
> > -	config.height = format.height;
> > -	config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> > -	config.bufferCount = 4;
> > +	config->bufferCount = IPU3_BUFFER_COUNT;
> >
> > -	configs[&data->stream_] = config;
> > +	LOG(IPU3, Debug)
> > +		<< "Stream format set to = (" << config->width << "x"
> > +		<< config->height << ") - 0x" << std::hex << std::setfill('0')
> > +		<< std::setw(8) << config->pixelFormat;
> >
> >  	return configs;
> >  }
> > @@ -327,6 +353,50 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> >  	return true;
> >  }
> >
> > +int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)
> > +{
> > +	switch(code) {
> > +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE:
> > +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE:
> > +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE:
> > +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE:
> > +	case MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8:
> > +	case MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8:
> > +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> > +	case MEDIA_BUS_FMT_SBGGR14_1X14:
> > +	case MEDIA_BUS_FMT_SBGGR16_1X16:
> > +		return V4L2_PIX_FMT_IPU3_SBGGR10;
> > +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +	case MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8:
> > +	case MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8:
> > +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> > +	case MEDIA_BUS_FMT_SGBRG14_1X14:
> > +	case MEDIA_BUS_FMT_SGBRG16_1X16:
> > +		return V4L2_PIX_FMT_IPU3_SGBRG10;
> > +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +	case MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8:
> > +	case MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8:
> > +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> > +	case MEDIA_BUS_FMT_SGRBG14_1X14:
> > +	case MEDIA_BUS_FMT_SGRBG16_1X16:
> > +		return V4L2_PIX_FMT_IPU3_SGRBG10;
> > +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +	case MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8:
> > +	case MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8:
> > +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +	case MEDIA_BUS_FMT_SRGGB14_1X14:
> > +	case MEDIA_BUS_FMT_SRGGB16_1X16:
> > +		return V4L2_PIX_FMT_IPU3_SRGGB10;
>
> Does the CIO2 really support all these formats, especially the ALAW8,
> DPCM8 and > 10bpp formats ?
>

No idea. I intentionally put all v4l2 defined raw formats here to get
feedback. I'm fine restricting this to the most common 1x8 1x10 and
1x12 formats (or just drop the exotic ALAW and DPCM ones).

Thanks
  j


> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  /*
> >   * Cameras are created associating an image sensor (represented by a
> >   * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 55489c31df2d..0f18e4692e77 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -6,8 +6,11 @@ 
  */
 
 #include <memory>
+#include <iomanip>
 #include <vector>
 
+#include <linux/media-bus-format.h>
+
 #include <libcamera/camera.h>
 #include <libcamera/request.h>
 #include <libcamera/stream.h>
@@ -72,12 +75,16 @@  private:
 		Stream stream_;
 	};
 
+	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
+
 	IPU3CameraData *cameraData(const Camera *camera)
 	{
 		return static_cast<IPU3CameraData *>(
 			PipelineHandler::cameraData(camera));
 	}
 
+	int mediaBusToCIO2Format(unsigned int code);
+
 	void registerCameras();
 
 	std::shared_ptr<MediaDevice> cio2_;
@@ -102,26 +109,45 @@  std::map<Stream *, StreamConfiguration>
 PipelineHandlerIPU3::streamConfiguration(Camera *camera,
 					 std::set<Stream *> &streams)
 {
-	IPU3CameraData *data = cameraData(camera);
 	std::map<Stream *, StreamConfiguration> configs;
-	V4L2SubdeviceFormat format = {};
+	IPU3CameraData *data = cameraData(camera);
+	V4L2Subdevice *sensor = data->sensor_;
+	StreamConfiguration *config = &configs[&data->stream_];
 
 	/*
-	 * FIXME: As of now, return the image format reported by the sensor.
-	 * In future good defaults should be provided for each stream.
+	 * Make sure the sensor produces a raw format compatible with the
+	 * CIO2 and use the largest image size the sensor provides.
 	 */
-	if (data->sensor_->getFormat(0, &format)) {
-		LOG(IPU3, Error) << "Failed to create stream configurations";
+	const SubdevFormatEnum formats = sensor->formats(0);
+	for (auto it = formats.begin(); it != formats.end(); ++it) {
+		int cio2Code = mediaBusToCIO2Format(it->first);
+		if (cio2Code == -EINVAL)
+			continue;
+
+		for (const SizeRange &range : it->second) {
+			if (range.maxWidth <= config->width ||
+			    range.maxHeight <= config->height)
+				continue;
+
+			config->width = range.maxWidth;
+			config->height = range.maxHeight;
+			config->pixelFormat = cio2Code;
+		}
+	}
+
+	/* If not suitable format has been found, return an empty config. */
+	if (!config->pixelFormat) {
+		LOG(IPU3, Error) << "Sensor image format not supported";
+
 		return configs;
 	}
 
-	StreamConfiguration config = {};
-	config.width = format.width;
-	config.height = format.height;
-	config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
-	config.bufferCount = 4;
+	config->bufferCount = IPU3_BUFFER_COUNT;
 
-	configs[&data->stream_] = config;
+	LOG(IPU3, Debug)
+		<< "Stream format set to = (" << config->width << "x"
+		<< config->height << ") - 0x" << std::hex << std::setfill('0')
+		<< std::setw(8) << config->pixelFormat;
 
 	return configs;
 }
@@ -327,6 +353,50 @@  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
 	return true;
 }
 
+int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)
+{
+	switch(code) {
+	case MEDIA_BUS_FMT_SBGGR8_1X8:
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+	case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE:
+	case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE:
+	case MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE:
+	case MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE:
+	case MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8:
+	case MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8:
+	case MEDIA_BUS_FMT_SBGGR12_1X12:
+	case MEDIA_BUS_FMT_SBGGR14_1X14:
+	case MEDIA_BUS_FMT_SBGGR16_1X16:
+		return V4L2_PIX_FMT_IPU3_SBGGR10;
+	case MEDIA_BUS_FMT_SGBRG8_1X8:
+	case MEDIA_BUS_FMT_SGBRG10_1X10:
+	case MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8:
+	case MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8:
+	case MEDIA_BUS_FMT_SGBRG12_1X12:
+	case MEDIA_BUS_FMT_SGBRG14_1X14:
+	case MEDIA_BUS_FMT_SGBRG16_1X16:
+		return V4L2_PIX_FMT_IPU3_SGBRG10;
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+	case MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8:
+	case MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8:
+	case MEDIA_BUS_FMT_SGRBG12_1X12:
+	case MEDIA_BUS_FMT_SGRBG14_1X14:
+	case MEDIA_BUS_FMT_SGRBG16_1X16:
+		return V4L2_PIX_FMT_IPU3_SGRBG10;
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+	case MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8:
+	case MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8:
+	case MEDIA_BUS_FMT_SRGGB12_1X12:
+	case MEDIA_BUS_FMT_SRGGB14_1X14:
+	case MEDIA_BUS_FMT_SRGGB16_1X16:
+		return V4L2_PIX_FMT_IPU3_SRGGB10;
+	default:
+		return -EINVAL;
+	}
+}
+
 /*
  * Cameras are created associating an image sensor (represented by a
  * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four