[libcamera-devel] [PATCH v3 6/9] pipeline: raspberrypi: Set packing formats for the Unicam image node

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Oct 27, 2021 at 10:28:00AM +0100, Naushir Patuck wrote:
> Default to using CSI2 packed formats when setting up the Unicam image format,
> but use an unpacked format if the user requests one through StreamConfiguration.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 48f561d31a31..1b78b5e74a63 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode)
>  	return BayerFormat{};
>  }
>  
> -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format)
> +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, BayerFormat::Packing packing)
>  {
>  	V4L2DeviceFormat deviceFormat;
>  	BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code);
>  
>  	ASSERT(bayer.isValid());
>  
> -	bayer.packing = BayerFormat::Packing::CSI2Packed;
> +	bayer.packing = packing;
>  	deviceFormat.fourcc = bayer.toV4L2PixelFormat();

Packing is handled explicitly, so going through BayerFormat here is fine
I think. The other calls to mbusCodeToBayerFormat() in 5/9 however
bother me, especially when calling .toPixelFormat() or
.toV4L2PixelFormat() immediately after. It would be good to wrap those
in helper functions that explicitly handle packing, to ensure that it
will be considered everywhere.

Have you tested both packed and unpacked formats by the way ?

>  	deviceFormat.size = format.size;
>  	return deviceFormat;
> @@ -413,7 +413,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  			 * the user request.
>  			 */
>  			V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size);
> -			V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);
> +			V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat,
> +									   BayerFormat::Packing::CSI2Packed);
>  			int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);
>  			if (ret)
>  				return Invalid;
> @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	for (auto const stream : data->streams_)
>  		stream->reset();
>  
> +	BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed;

Could we default to packed formats when generating the configuration
too ?

>  	Size maxSize, sensorSize;
>  	unsigned int maxIndex = 0;
>  	bool rawStream = false;
> @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  			 */
>  			sensorSize = cfg.size;
>  			rawStream = true;
> +			/* Check if the user has explicitly set an unpacked format. */
> +			packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;
>  		} else {
>  			if (cfg.size > maxSize) {
>  				maxSize = config->at(i).size;
> @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	 * Unicam image output format. The ISP input format gets set at start,
>  	 * just in case we have swapped bayer orders due to flips.
>  	 */
> -	V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);
> +	V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing);
>  	ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart