[libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2: Consolidate information about formats

[Jacopo Mondi]

Hi Niklas,

On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote:
> Instead of spreading the mapping between media bus codes and V4L2 FourCC
> all over the CIO2 code collect it in a single map and extract the data
> from it. This is done in preparation of adding PixelFormat information
> to the mix.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++---------------
>  1 file changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -18,6 +18,17 @@ namespace libcamera {
>
>  LOG_DECLARE_CATEGORY(IPU3)
>
> +namespace {
> +
> +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = {
> +	{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },
> +	{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },
> +	{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },
> +	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },
> +};
> +
> +} /* namespace */
> +

Ok, I see where this is going with the next patch, but I'm not super
excited by having to iterate the map just to extract keys (and anyway,
why doesn't std::map have a keys() function ??).

>  CIO2Device::CIO2Device()
>  	: output_(nullptr), csi2_(nullptr), sensor_(nullptr)
>  {
> @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  	 * Make sure the sensor produces at least one format compatible with
>  	 * the CIO2 requirements.
>  	 */
> -	const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> -						MEDIA_BUS_FMT_SGRBG10_1X10,
> -						MEDIA_BUS_FMT_SGBRG10_1X10,
> -						MEDIA_BUS_FMT_SRGGB10_1X10 };
> +	std::set<unsigned int> cio2Codes;

why a set ?

> +	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> +		std::inserter(cio2Codes, cio2Codes.begin()),
> +		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });

you could
		[](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; });
or since C++14
		[](auto &pair){ return pair.first; });



>  	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
>  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
>  				cio2Codes.begin(), cio2Codes.end())) {
> @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
>  	 * Apply the selected format to the sensor, the CSI-2 receiver and
>  	 * the CIO2 output device.
>  	 */
> -	sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> -					    MEDIA_BUS_FMT_SGBRG10_1X10,
> -					    MEDIA_BUS_FMT_SGRBG10_1X10,
> -					    MEDIA_BUS_FMT_SRGGB10_1X10 },
> -					  size);
> +	std::vector<unsigned int> mbusCodes;

You could reserve space in the vector knowing the map size

> +	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> +		std::back_inserter(mbusCodes),
> +		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> +
> +	sensorFormat = sensor_->getFormat(mbusCodes, size);
>  	ret = sensor_->setFormat(&sensorFormat);
>  	if (ret)
>  		return ret;
> @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
>  	if (ret)
>  		return ret;
>
> -	V4L2PixelFormat v4l2Format;
> -	switch (sensorFormat.mbus_code) {
> -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> -		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> -		break;
> -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> -		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> -		break;
> -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> -		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> -		break;
> -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> -		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> -		break;
> -	default:
> +	const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code);
> +	if (itInfo == mbusCodesToInfo.end())
>  		return -EINVAL;
> -	}
>
> -	outputFormat->fourcc = v4l2Format;
> +	outputFormat->fourcc = itInfo->second;

I would be tempted to suggest you to add helper functions around that
map, not sure it's worth the effort, but those transform to just
retreive keys are not nice imho.

That apart
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j
>  	outputFormat->size = sensorFormat.size;
>  	outputFormat->planesCount = 1;
>
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel