[{"id":20558,"web_url":"https://patchwork.libcamera.org/comment/20558/","msgid":"<163533683068.1184428.15438489259661859505@Monstersaurus>","date":"2021-10-27T12:13:50","subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2021-10-27 10:27:59)\n> Switch the pipeline handler to use the new Unicam media controller based driver.\n> With this change, we directly talk to the sensor device driver to set controls\n> and set/get formats in the pipeline handler.\n> \n> This change requires the accompanying Raspberry Pi linux kernel change at\n> https://github.com/raspberrypi/linux/pull/4645. If this kernel change is not\n> present, the pipeline handler will fail to run with an error message informing\n> the user to update the kernel build.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 185 ++++++++++++------\n> 1 file changed, 127 insertions(+), 58 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 1634ca98f481..48f561d31a31 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -26,6 +26,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \"libcamera/internal/bayer_format.h\"\n> @@ -48,6 +49,66 @@ LOG_DEFINE_CATEGORY(RPI)\n> \n> namespace {\n> \n> +/* Map of mbus codes to supported sizes reported by the sensor. */\n> +using SensorFormats = std::map>;\n> +\n> +SensorFormats populateSensorFormats(std::unique_ptr &sensor)\n> +{\n> + SensorFormats formats;\n> +\n> + for (auto const mbusCode : sensor->mbusCodes())\n> + formats.emplace(mbusCode, sensor->sizes(mbusCode));\n> +\n> + return formats;\n> +}\n> +\n> +BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode)\n> +{\n> + static const std::unordered_map mbusCodeToBayer {\n> + { MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGBRG8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGRBG8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SRGGB8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGBRG10_1X10, { BayerFormat::GBRG, 10, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGRBG10_1X10, { BayerFormat::GRBG, 10, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SRGGB10_1X10, { BayerFormat::RGGB, 10, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SBGGR12_1X12, { BayerFormat::BGGR, 12, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGBRG12_1X12, { BayerFormat::GBRG, 12, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGRBG12_1X12, { BayerFormat::GRBG, 12, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SRGGB12_1X12, { BayerFormat::RGGB, 12, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SBGGR14_1X14, { BayerFormat::BGGR, 14, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGBRG14_1X14, { BayerFormat::GBRG, 14, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGRBG14_1X14, { BayerFormat::GRBG, 14, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SRGGB14_1X14, { BayerFormat::RGGB, 14, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::None } },\n> + { MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::None } },\n> + };\n> +\n> + const auto it = mbusCodeToBayer.find(mbusCode);\n> + if (it != mbusCodeToBayer.end())\n> + return it->second;\n> +\n> + return BayerFormat{};\n> +}\n> +\n> +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format)\n> +{\n> + V4L2DeviceFormat deviceFormat;\n> + BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code);\n> +\n> + ASSERT(bayer.isValid());\n> +\n> + bayer.packing = BayerFormat::Packing::CSI2Packed;\n\nI think this can be BayerFormat::CSI2Packed; It doesn't hurt to\nfully qualify it I suspect, but the packing is used without the\n::Packing:: in the table above.\n\nIf we always set BayerFormat::CSI2Packed though, why not do that in the\ntable above instead of initialising with BayerFormat::None?\n\n(Maybe I'll discover later that we don't alway do this ... ?)\n\n> + deviceFormat.fourcc = bayer.toV4L2PixelFormat();\n> + deviceFormat.size = format.size;\n> + return deviceFormat;\n> +}\n> +\n> bool isRaw(PixelFormat &pixFmt)\n> {\n> /*\n> @@ -74,11 +135,10 @@ double scoreFormat(double desired, double actual)\n> return score;\n> }\n> \n> -V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> - const Size &req)\n> +V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &req)\n> {\n> double bestScore = std::numeric_limits::max(), score;\n> - V4L2DeviceFormat bestMode;\n> + V4L2SubdeviceFormat bestFormat;\n> \n> #define PENALTY_AR 1500.0\n> #define PENALTY_8BIT 2000.0\n> @@ -88,19 +148,18 @@ V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> \n> /* Calculate the closest/best mode from the user requested size. */\n> for (const auto &iter : formatsMap) {\n> - V4L2PixelFormat v4l2Format = iter.first;\n> - const PixelFormatInfo &info = PixelFormatInfo::info(v4l2Format);\n> + const unsigned int mbusCode = iter.first;\n> + const PixelFormat format = mbusCodeToBayerFormat(mbusCode).toPixelFormat();\n\nAha, ok here it is ... so we can't initialise with Packed in the table.\n\n> + const PixelFormatInfo &info = PixelFormatInfo::info(format);\n> \n> - for (const SizeRange &sz : iter.second) {\n> - double modeWidth = sz.contains(req) ? req.width : sz.max.width;\n> - double modeHeight = sz.contains(req) ? req.height : sz.max.height;\n> + for (const Size &size : iter.second) {\n> double reqAr = static_cast(req.width) / req.height;\n> - double modeAr = modeWidth / modeHeight;\n> + double fmtAr = size.width / size.height;\n> \n> /* Score the dimensions for closeness. */\n> - score = scoreFormat(req.width, modeWidth);\n> - score += scoreFormat(req.height, modeHeight);\n> - score += PENALTY_AR * scoreFormat(reqAr, modeAr);\n> + score = scoreFormat(req.width, size.width);\n> + score += scoreFormat(req.height, size.height);\n> + score += PENALTY_AR * scoreFormat(reqAr, fmtAr);\n> \n> /* Add any penalties... this is not an exact science! */\n> if (!info.packed)\n> @@ -115,18 +174,18 @@ V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> \n> if (score <= bestScore) {\n> bestScore = score;\n> - bestMode.fourcc = v4l2Format;\n> - bestMode.size = Size(modeWidth, modeHeight);\n> + bestFormat.mbus_code = mbusCode;\n> + bestFormat.size = size;\n> }\n> \n> - LOG(RPI, Info) << \"Mode: \" << modeWidth << \"x\" << modeHeight\n> - << \" fmt \" << v4l2Format.toString()\n> + LOG(RPI, Info) << \"Format: \" << size.toString()\n> + << \" fmt \" << format.toString()\n> << \" Score: \" << score\n> << \" (best \" << bestScore << \")\";\n> }\n> }\n> \n> - return bestMode;\n> + return bestFormat;\n> }\n> \n> enum class Unicam : unsigned int { Image, Embedded };\n> @@ -170,6 +229,7 @@ public:\n> std::unique_ptr ipa_;\n> \n> std::unique_ptr sensor_;\n> + SensorFormats sensorFormats_;\n> /* Array of Unicam and ISP device streams and associated buffers/streams. */\n> RPi::Device unicam_;\n> RPi::Device isp_;\n> @@ -352,9 +412,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> * Calculate the best sensor mode we can use based on\n> * the user request.\n> */\n> - V4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats();\n> - V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);\n> - int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);\n> + V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size);\n> + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);\n> + int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);\n> if (ret)\n> return Invalid;\n> \n> @@ -366,7 +426,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> * fetch the \"native\" (i.e. untransformed) Bayer order,\n> * because the sensor may currently be flipped!\n> */\n> - V4L2PixelFormat fourcc = sensorFormat.fourcc;\n> + V4L2PixelFormat fourcc = unicamFormat.fourcc;\n> if (data_->flipsAlterBayerOrder_) {\n> BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);\n> bayer.order = data_->nativeBayerOrder_;\n> @@ -375,15 +435,15 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> }\n> \n> PixelFormat sensorPixFormat = fourcc.toPixelFormat();\n> - if (cfg.size != sensorFormat.size ||\n> + if (cfg.size != unicamFormat.size ||\n> cfg.pixelFormat != sensorPixFormat) {\n> - cfg.size = sensorFormat.size;\n> + cfg.size = unicamFormat.size;\n> cfg.pixelFormat = sensorPixFormat;\n> status = Adjusted;\n> }\n> \n> - cfg.stride = sensorFormat.planes[0].bpl;\n> - cfg.frameSize = sensorFormat.planes[0].size;\n> + cfg.stride = unicamFormat.planes[0].bpl;\n> + cfg.frameSize = unicamFormat.planes[0].size;\n> \n> rawCount++;\n> } else {\n> @@ -472,7 +532,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> {\n> RPiCameraData *data = cameraData(camera);\n> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> - V4L2DeviceFormat sensorFormat;\n> + V4L2SubdeviceFormat sensorFormat;\n> unsigned int bufferCount;\n> PixelFormat pixelFormat;\n> V4L2VideoDevice::Formats fmts;\n> @@ -487,9 +547,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> switch (role) {\n> case StreamRole::Raw:\n> size = data->sensor_->resolution();\n> - fmts = data->unicam_[Unicam::Image].dev()->formats();\n> - sensorFormat = findBestMode(fmts, size);\n> - pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> + sensorFormat = findBestFormat(data->sensorFormats_, size);\n> + pixelFormat = mbusCodeToBayerFormat(sensorFormat.mbus_code).toPixelFormat();\n> ASSERT(pixelFormat.isValid());\n> bufferCount = 2;\n> rawCount++;\n> @@ -599,32 +658,29 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> }\n> \n> /* First calculate the best sensor mode we can use based on the user request. */\n> - V4L2VideoDevice::Formats fmts = data->unicam_[Unicam::Image].dev()->formats();\n> - V4L2DeviceFormat sensorFormat = findBestMode(fmts, rawStream ? sensorSize : maxSize);\n> + V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);\n> + ret = data->sensor_->setFormat(&sensorFormat);\n> + if (ret)\n> + return ret;\n> \n> /*\n> * Unicam image output format. The ISP input format gets set at start,\n> * just in case we have swapped bayer orders due to flips.\n> */\n> - ret = data->unicam_[Unicam::Image].dev()->setFormat(&sensorFormat);\n> + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);\n> + ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);\n> if (ret)\n> return ret;\n> \n> - /*\n> - * The control ranges associated with the sensor may need updating\n> - * after a format change.\n> - * \\todo Use the CameraSensor::setFormat API instead.\n> - */\n> - data->sensor_->updateControlInfo();\n> -\n> LOG(RPI, Info) << \"Sensor: \" << camera->id()\n> - << \" - Selected mode: \" << sensorFormat.toString();\n> + << \" - Selected sensor format: \" << sensorFormat.toString()\n> + << \" - Selected unicam format: \" << unicamFormat.toString();\n> \n> /*\n> * This format may be reset on start() if the bayer order has changed\n> * because of flips in the sensor.\n> */\n> - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> + ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat);\n> if (ret)\n> return ret;\n> \n> @@ -746,8 +802,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> data->ispMinCropSize_ = testCrop.size();\n> \n> /* Adjust aspect ratio by providing crops on the input image. */\n> - Size size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> - Rectangle crop = size.centeredTo(Rectangle(sensorFormat.size).center());\n> + Size size = unicamFormat.size.boundedToAspectRatio(maxSize);\n> + Rectangle crop = size.centeredTo(Rectangle(unicamFormat.size).center());\n> data->ispCrop_ = crop;\n> \n> data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> @@ -761,8 +817,11 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> * supports it.\n> */\n> if (data->sensorMetadata_) {\n> - format = {};\n> + V4L2SubdeviceFormat embeddedFormat;\n> +\n> + data->sensor_->device()->getFormat(1, &embeddedFormat);\n> format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);\n> + format.planes[0].size = embeddedFormat.size.width * embeddedFormat.size.height;\n> \n> LOG(RPI, Debug) << \"Setting embedded data format.\";\n> ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);\n> @@ -847,9 +906,14 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> * IPA configure may have changed the sensor flips - hence the bayer\n> * order. Get the sensor format and set the ISP input now.\n> */\n> - V4L2DeviceFormat sensorFormat;\n> - data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n> - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> + V4L2SubdeviceFormat sensorFormat;\n> + data->sensor_->device()->getFormat(0, &sensorFormat);\n> +\n> + V4L2DeviceFormat ispFormat;\n> + ispFormat.fourcc = mbusCodeToBayerFormat(sensorFormat.mbus_code).toV4L2PixelFormat();\n> + ispFormat.size = sensorFormat.size;\n> +\n> + ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat);\n> if (ret) {\n> stop(camera);\n> return ret;\n> @@ -1004,6 +1068,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> if (data->sensor_->init())\n> return false;\n> \n> + data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> +\n> ipa::RPi::SensorConfig sensorConfig;\n> if (data->loadIPA(&sensorConfig)) {\n> LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> @@ -1030,6 +1096,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> return false;\n> }\n> \n> + if (!(data->unicam_[Unicam::Image].dev()->caps().device_caps() & V4L2_CAP_IO_MC)) {\n\nI'd probably add this to v4l2_videodevice.h as hasMediaController() on\nthe V4L2Capabilities.\n\n Done: libcamera: v4l2_videodevice: provide hasMediaController()\n\nNo requirement to change here, unless you want to/the above gets in\nfast, it can be updated later.\n\n\n> + LOG(RPI, Error) << \"Unicam driver did not advertise V4L2_CAP_IO_MC, please update your kernel!\";\n\nI would probably have written this as \n\t\"Unicam driver does not use the MediaController, please update your kernel!\";\n\nbut I'm pleased to see we can check this at run time anyway.\n\n\nNothing else stands out to me in this so:\n\n\nReviewed-by: Kieran Bingham \n\n\n> + return false;\n> + }\n> +\n> /*\n> * Setup our delayed control writer with the sensor default\n> * gain and exposure delays. Mark VBLANK for priority write.\n> @@ -1039,7 +1110,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },\n> { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }\n> };\n> - data->delayedCtrls_ = std::make_unique(data->unicam_[Unicam::Image].dev(), params);\n> + data->delayedCtrls_ = std::make_unique(data->sensor_->device(), params);\n> data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> \n> /* Register the controls that the Raspberry Pi IPA can handle. */\n> @@ -1066,15 +1137,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> * As part of answering the final question, we reset the camera to\n> * no transform at all.\n> */\n> -\n> - V4L2VideoDevice *dev = data->unicam_[Unicam::Image].dev();\n> - const struct v4l2_query_ext_ctrl *hflipCtrl = dev->controlInfo(V4L2_CID_HFLIP);\n> + const V4L2Subdevice *sensor = data->sensor_->device();\n> + const struct v4l2_query_ext_ctrl *hflipCtrl = sensor->controlInfo(V4L2_CID_HFLIP);\n> if (hflipCtrl) {\n> /* We assume it will support vflips too... */\n> data->supportsFlips_ = true;\n> data->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> \n> - ControlList ctrls(dev->controls());\n> + ControlList ctrls(data->sensor_->controls());\n> ctrls.set(V4L2_CID_HFLIP, 0);\n> ctrls.set(V4L2_CID_VFLIP, 0);\n> data->setSensorControls(ctrls);\n> @@ -1082,9 +1152,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \n> /* Look for a valid Bayer format. */\n> BayerFormat bayerFormat;\n> - for (const auto &iter : dev->formats()) {\n> - V4L2PixelFormat v4l2Format = iter.first;\n> - bayerFormat = BayerFormat::fromV4L2PixelFormat(v4l2Format);\n> + for (const auto &iter : data->sensorFormats_) {\n> + bayerFormat = mbusCodeToBayerFormat(iter.first);\n> if (bayerFormat.isValid())\n> break;\n> }\n> @@ -1271,7 +1340,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> }\n> }\n> \n> - entityControls.emplace(0, unicam_[Unicam::Image].dev()->controls());\n> + entityControls.emplace(0, sensor_->controls());\n> entityControls.emplace(1, isp_[Isp::Input].dev()->controls());\n> \n> /* Always send the user transform to the IPA. */\n> @@ -1406,10 +1475,10 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> ControlList vblank_ctrl;\n> \n> vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK));\n> - unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> + sensor_->setControls(&vblank_ctrl);\n> }\n> \n> - unicam_[Unicam::Image].dev()->setControls(&controls);\n> + sensor_->setControls(&controls);\n> }\n> \n> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> -- \n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D76C2BDB1C\n\tfor ;\n\tWed, 27 Oct 2021 12:13:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2AD3164882;\n\tWed, 27 Oct 2021 14:13:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DC6360123\n\tfor ;\n\tWed, 27 Oct 2021 14:13:54 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D4F6D596;\n\tWed, 27 Oct 2021 14:13:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VsouMnti\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635336833;\n\tbh=LM679g8TsBl56EIh/0AHIlFefvOVK5q3ny+ktDdUqyc=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=VsouMnti32aalZTFLZNnMrqt5XiDmB9pCAZOzMItKJvJk2x4iHl4pgc9ypIj5s/mn\n\tqEqsuF9kLHoPAAqIo2/PdS7QuL/Fx1KTEEoVRuGQJZA4cGfzGFI/4Bja65PGD36Mn7\n\ts7vUdItCbCyXfOiOu7eSUwV46fy01epIBsOAj4VM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211027092803.3671096-6-naush@raspberrypi.com>","References":"<20211027092803.3671096-1-naush@raspberrypi.com>\n\t<20211027092803.3671096-6-naush@raspberrypi.com>","From":"Kieran Bingham ","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 27 Oct 2021 13:13:50 +0100","Message-ID":"<163533683068.1184428.15438489259661859505@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20568,"web_url":"https://patchwork.libcamera.org/comment/20568/","msgid":"","date":"2021-10-27T12:36:42","subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for your feedback.\n\nOn Wed, 27 Oct 2021 at 13:13, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck (2021-10-27 10:27:59)\n> > Switch the pipeline handler to use the new Unicam media controller based\n> driver.\n> > With this change, we directly talk to the sensor device driver to set\n> controls\n> > and set/get formats in the pipeline handler.\n> >\n> > This change requires the accompanying Raspberry Pi linux kernel change at\n> > https://github.com/raspberrypi/linux/pull/4645. If this kernel change\n> is not\n> > present, the pipeline handler will fail to run with an error message\n> informing\n> > the user to update the kernel build.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 185 ++++++++++++------\n> > 1 file changed, 127 insertions(+), 58 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 1634ca98f481..48f561d31a31 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -26,6 +26,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> >\n> > #include \"libcamera/internal/bayer_format.h\"\n> > @@ -48,6 +49,66 @@ LOG_DEFINE_CATEGORY(RPI)\n> >\n> > namespace {\n> >\n> > +/* Map of mbus codes to supported sizes reported by the sensor. */\n> > +using SensorFormats = std::map>;\n> > +\n> > +SensorFormats populateSensorFormats(std::unique_ptr\n> &sensor)\n> > +{\n> > + SensorFormats formats;\n> > +\n> > + for (auto const mbusCode : sensor->mbusCodes())\n> > + formats.emplace(mbusCode, sensor->sizes(mbusCode));\n> > +\n> > + return formats;\n> > +}\n> > +\n> > +BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode)\n> > +{\n> > + static const std::unordered_map\n> mbusCodeToBayer {\n> > + { MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG8_1X8, { BayerFormat::GBRG, 8,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG8_1X8, { BayerFormat::GRBG, 8,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB8_1X8, { BayerFormat::RGGB, 8,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG10_1X10, { BayerFormat::GBRG, 10,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG10_1X10, { BayerFormat::GRBG, 10,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB10_1X10, { BayerFormat::RGGB, 10,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SBGGR12_1X12, { BayerFormat::BGGR, 12,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG12_1X12, { BayerFormat::GBRG, 12,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG12_1X12, { BayerFormat::GRBG, 12,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB12_1X12, { BayerFormat::RGGB, 12,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SBGGR14_1X14, { BayerFormat::BGGR, 14,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG14_1X14, { BayerFormat::GBRG, 14,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG14_1X14, { BayerFormat::GRBG, 14,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB14_1X14, { BayerFormat::RGGB, 14,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8,\n> BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10,\n> BayerFormat::None } },\n> > + };\n> > +\n> > + const auto it = mbusCodeToBayer.find(mbusCode);\n> > + if (it != mbusCodeToBayer.end())\n> > + return it->second;\n> > +\n> > + return BayerFormat{};\n> > +}\n> > +\n> > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format)\n> > +{\n> > + V4L2DeviceFormat deviceFormat;\n> > + BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code);\n> > +\n> > + ASSERT(bayer.isValid());\n> > +\n> > + bayer.packing = BayerFormat::Packing::CSI2Packed;\n>\n> I think this can be BayerFormat::CSI2Packed; It doesn't hurt to\n> fully qualify it I suspect, but the packing is used without the\n> ::Packing:: in the table above.\n>\n> If we always set BayerFormat::CSI2Packed though, why not do that in the\n> table above instead of initialising with BayerFormat::None?\n>\n> (Maybe I'll discover later that we don't alway do this ... ?)\n>\n> > + deviceFormat.fourcc = bayer.toV4L2PixelFormat();\n> > + deviceFormat.size = format.size;\n> > + return deviceFormat;\n> > +}\n> > +\n> > bool isRaw(PixelFormat &pixFmt)\n> > {\n> > /*\n> > @@ -74,11 +135,10 @@ double scoreFormat(double desired, double actual)\n> > return score;\n> > }\n> >\n> > -V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > - const Size &req)\n> > +V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap,\n> const Size &req)\n> > {\n> > double bestScore = std::numeric_limits::max(), score;\n> > - V4L2DeviceFormat bestMode;\n> > + V4L2SubdeviceFormat bestFormat;\n> >\n> > #define PENALTY_AR 1500.0\n> > #define PENALTY_8BIT 2000.0\n> > @@ -88,19 +148,18 @@ V4L2DeviceFormat\n> findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> >\n> > /* Calculate the closest/best mode from the user requested size.\n> */\n> > for (const auto &iter : formatsMap) {\n> > - V4L2PixelFormat v4l2Format = iter.first;\n> > - const PixelFormatInfo &info =\n> PixelFormatInfo::info(v4l2Format);\n> > + const unsigned int mbusCode = iter.first;\n> > + const PixelFormat format =\n> mbusCodeToBayerFormat(mbusCode).toPixelFormat();\n>\n> Aha, ok here it is ... so we can't initialise with Packed in the table.\n>\n> > + const PixelFormatInfo &info =\n> PixelFormatInfo::info(format);\n> >\n> > - for (const SizeRange &sz : iter.second) {\n> > - double modeWidth = sz.contains(req) ? req.width\n> : sz.max.width;\n> > - double modeHeight = sz.contains(req) ?\n> req.height : sz.max.height;\n> > + for (const Size &size : iter.second) {\n> > double reqAr = static_cast(req.width) /\n> req.height;\n> > - double modeAr = modeWidth / modeHeight;\n> > + double fmtAr = size.width / size.height;\n> >\n> > /* Score the dimensions for closeness. */\n> > - score = scoreFormat(req.width, modeWidth);\n> > - score += scoreFormat(req.height, modeHeight);\n> > - score += PENALTY_AR * scoreFormat(reqAr, modeAr);\n> > + score = scoreFormat(req.width, size.width);\n> > + score += scoreFormat(req.height, size.height);\n> > + score += PENALTY_AR * scoreFormat(reqAr, fmtAr);\n> >\n> > /* Add any penalties... this is not an exact\n> science! */\n> > if (!info.packed)\n> > @@ -115,18 +174,18 @@ V4L2DeviceFormat\n> findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> >\n> > if (score <= bestScore) {\n> > bestScore = score;\n> > - bestMode.fourcc = v4l2Format;\n> > - bestMode.size = Size(modeWidth,\n> modeHeight);\n> > + bestFormat.mbus_code = mbusCode;\n> > + bestFormat.size = size;\n> > }\n> >\n> > - LOG(RPI, Info) << \"Mode: \" << modeWidth << \"x\"\n> << modeHeight\n> > - << \" fmt \" <<\n> v4l2Format.toString()\n> > + LOG(RPI, Info) << \"Format: \" << size.toString()\n> > + << \" fmt \" << format.toString()\n> > << \" Score: \" << score\n> > << \" (best \" << bestScore << \")\";\n> > }\n> > }\n> >\n> > - return bestMode;\n> > + return bestFormat;\n> > }\n> >\n> > enum class Unicam : unsigned int { Image, Embedded };\n> > @@ -170,6 +229,7 @@ public:\n> > std::unique_ptr ipa_;\n> >\n> > std::unique_ptr sensor_;\n> > + SensorFormats sensorFormats_;\n> > /* Array of Unicam and ISP device streams and associated\n> buffers/streams. */\n> > RPi::Device unicam_;\n> > RPi::Device isp_;\n> > @@ -352,9 +412,9 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n> > * Calculate the best sensor mode we can use\n> based on\n> > * the user request.\n> > */\n> > - V4L2VideoDevice::Formats fmts =\n> data_->unicam_[Unicam::Image].dev()->formats();\n> > - V4L2DeviceFormat sensorFormat =\n> findBestMode(fmts, cfg.size);\n> > - int ret =\n> data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);\n> > + V4L2SubdeviceFormat sensorFormat =\n> findBestFormat(data_->sensorFormats_, cfg.size);\n> > + V4L2DeviceFormat unicamFormat =\n> toV4L2DeviceFormat(sensorFormat);\n> > + int ret =\n> data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);\n> > if (ret)\n> > return Invalid;\n> >\n> > @@ -366,7 +426,7 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n> > * fetch the \"native\" (i.e. untransformed) Bayer\n> order,\n> > * because the sensor may currently be flipped!\n> > */\n> > - V4L2PixelFormat fourcc = sensorFormat.fourcc;\n> > + V4L2PixelFormat fourcc = unicamFormat.fourcc;\n> > if (data_->flipsAlterBayerOrder_) {\n> > BayerFormat bayer =\n> BayerFormat::fromV4L2PixelFormat(fourcc);\n> > bayer.order = data_->nativeBayerOrder_;\n> > @@ -375,15 +435,15 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n> > }\n> >\n> > PixelFormat sensorPixFormat =\n> fourcc.toPixelFormat();\n> > - if (cfg.size != sensorFormat.size ||\n> > + if (cfg.size != unicamFormat.size ||\n> > cfg.pixelFormat != sensorPixFormat) {\n> > - cfg.size = sensorFormat.size;\n> > + cfg.size = unicamFormat.size;\n> > cfg.pixelFormat = sensorPixFormat;\n> > status = Adjusted;\n> > }\n> >\n> > - cfg.stride = sensorFormat.planes[0].bpl;\n> > - cfg.frameSize = sensorFormat.planes[0].size;\n> > + cfg.stride = unicamFormat.planes[0].bpl;\n> > + cfg.frameSize = unicamFormat.planes[0].size;\n> >\n> > rawCount++;\n> > } else {\n> > @@ -472,7 +532,7 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> > CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > - V4L2DeviceFormat sensorFormat;\n> > + V4L2SubdeviceFormat sensorFormat;\n> > unsigned int bufferCount;\n> > PixelFormat pixelFormat;\n> > V4L2VideoDevice::Formats fmts;\n> > @@ -487,9 +547,8 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > switch (role) {\n> > case StreamRole::Raw:\n> > size = data->sensor_->resolution();\n> > - fmts =\n> data->unicam_[Unicam::Image].dev()->formats();\n> > - sensorFormat = findBestMode(fmts, size);\n> > - pixelFormat =\n> sensorFormat.fourcc.toPixelFormat();\n> > + sensorFormat =\n> findBestFormat(data->sensorFormats_, size);\n> > + pixelFormat =\n> mbusCodeToBayerFormat(sensorFormat.mbus_code).toPixelFormat();\n> > ASSERT(pixelFormat.isValid());\n> > bufferCount = 2;\n> > rawCount++;\n> > @@ -599,32 +658,29 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > }\n> >\n> > /* First calculate the best sensor mode we can use based on the\n> user request. */\n> > - V4L2VideoDevice::Formats fmts =\n> data->unicam_[Unicam::Image].dev()->formats();\n> > - V4L2DeviceFormat sensorFormat = findBestMode(fmts, rawStream ?\n> sensorSize : maxSize);\n> > + V4L2SubdeviceFormat sensorFormat =\n> findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);\n> > + ret = data->sensor_->setFormat(&sensorFormat);\n> > + if (ret)\n> > + return ret;\n> >\n> > /*\n> > * Unicam image output format. The ISP input format gets set at\n> start,\n> > * just in case we have swapped bayer orders due to flips.\n> > */\n> > - ret =\n> data->unicam_[Unicam::Image].dev()->setFormat(&sensorFormat);\n> > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);\n> > + ret =\n> data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);\n> > if (ret)\n> > return ret;\n> >\n> > - /*\n> > - * The control ranges associated with the sensor may need\n> updating\n> > - * after a format change.\n> > - * \\todo Use the CameraSensor::setFormat API instead.\n> > - */\n> > - data->sensor_->updateControlInfo();\n> > -\n> > LOG(RPI, Info) << \"Sensor: \" << camera->id()\n> > - << \" - Selected mode: \" <<\n> sensorFormat.toString();\n> > + << \" - Selected sensor format: \" <<\n> sensorFormat.toString()\n> > + << \" - Selected unicam format: \" <<\n> unicamFormat.toString();\n> >\n> > /*\n> > * This format may be reset on start() if the bayer order has\n> changed\n> > * because of flips in the sensor.\n> > */\n> > - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > + ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat);\n> > if (ret)\n> > return ret;\n> >\n> > @@ -746,8 +802,8 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > data->ispMinCropSize_ = testCrop.size();\n> >\n> > /* Adjust aspect ratio by providing crops on the input image. */\n> > - Size size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> > - Rectangle crop =\n> size.centeredTo(Rectangle(sensorFormat.size).center());\n> > + Size size = unicamFormat.size.boundedToAspectRatio(maxSize);\n> > + Rectangle crop =\n> size.centeredTo(Rectangle(unicamFormat.size).center());\n> > data->ispCrop_ = crop;\n> >\n> > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP,\n> &crop);\n> > @@ -761,8 +817,11 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > * supports it.\n> > */\n> > if (data->sensorMetadata_) {\n> > - format = {};\n> > + V4L2SubdeviceFormat embeddedFormat;\n> > +\n> > + data->sensor_->device()->getFormat(1, &embeddedFormat);\n> > format.fourcc =\n> V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);\n> > + format.planes[0].size = embeddedFormat.size.width *\n> embeddedFormat.size.height;\n> >\n> > LOG(RPI, Debug) << \"Setting embedded data format.\";\n> > ret =\n> data->unicam_[Unicam::Embedded].dev()->setFormat(&format);\n> > @@ -847,9 +906,14 @@ int PipelineHandlerRPi::start(Camera *camera, const\n> ControlList *controls)\n> > * IPA configure may have changed the sensor flips - hence the\n> bayer\n> > * order. Get the sensor format and set the ISP input now.\n> > */\n> > - V4L2DeviceFormat sensorFormat;\n> > - data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n> > - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > + V4L2SubdeviceFormat sensorFormat;\n> > + data->sensor_->device()->getFormat(0, &sensorFormat);\n> > +\n> > + V4L2DeviceFormat ispFormat;\n> > + ispFormat.fourcc =\n> mbusCodeToBayerFormat(sensorFormat.mbus_code).toV4L2PixelFormat();\n> > + ispFormat.size = sensorFormat.size;\n> > +\n> > + ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat);\n> > if (ret) {\n> > stop(camera);\n> > return ret;\n> > @@ -1004,6 +1068,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator\n> *enumerator)\n> > if (data->sensor_->init())\n> > return false;\n> >\n> > + data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > +\n> > ipa::RPi::SensorConfig sensorConfig;\n> > if (data->loadIPA(&sensorConfig)) {\n> > LOG(RPI, Error) << \"Failed to load a suitable IPA\n> library\";\n> > @@ -1030,6 +1096,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator\n> *enumerator)\n> > return false;\n> > }\n> >\n> > + if (!(data->unicam_[Unicam::Image].dev()->caps().device_caps() &\n> V4L2_CAP_IO_MC)) {\n>\n> I'd probably add this to v4l2_videodevice.h as hasMediaController() on\n> the V4L2Capabilities.\n>\n> Done: libcamera: v4l2_videodevice: provide hasMediaController()\n>\n> No requirement to change here, unless you want to/the above gets in\n> fast, it can be updated later.\n>\n\nI've reviewed that change, and I see Laurent has as well, so that could go\nin very soon...\nI will change this code to use your helper!\n\n\n>\n>\n> > + LOG(RPI, Error) << \"Unicam driver did not advertise\n> V4L2_CAP_IO_MC, please update your kernel!\";\n>\n> I would probably have written this as\n> \"Unicam driver does not use the MediaController, please update\n> your kernel!\";\n>\n\nAck.\n\n\n> but I'm pleased to see we can check this at run time anyway.\n>\n>\n> Nothing else stands out to me in this so:\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n\nThanks!\nNaush\n\n\n>\n>\n> > + return false;\n> > + }\n> > +\n> > /*\n> > * Setup our delayed control writer with the sensor default\n> > * gain and exposure delays. Mark VBLANK for priority write.\n> > @@ -1039,7 +1110,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator\n> *enumerator)\n> > { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false\n> } },\n> > { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }\n> > };\n> > - data->delayedCtrls_ =\n> std::make_unique(data->unicam_[Unicam::Image].dev(),\n> params);\n> > + data->delayedCtrls_ =\n> std::make_unique(data->sensor_->device(), params);\n> > data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> >\n> > /* Register the controls that the Raspberry Pi IPA can handle. */\n> > @@ -1066,15 +1137,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator\n> *enumerator)\n> > * As part of answering the final question, we reset the camera\n> to\n> > * no transform at all.\n> > */\n> > -\n> > - V4L2VideoDevice *dev = data->unicam_[Unicam::Image].dev();\n> > - const struct v4l2_query_ext_ctrl *hflipCtrl =\n> dev->controlInfo(V4L2_CID_HFLIP);\n> > + const V4L2Subdevice *sensor = data->sensor_->device();\n> > + const struct v4l2_query_ext_ctrl *hflipCtrl =\n> sensor->controlInfo(V4L2_CID_HFLIP);\n> > if (hflipCtrl) {\n> > /* We assume it will support vflips too... */\n> > data->supportsFlips_ = true;\n> > data->flipsAlterBayerOrder_ = hflipCtrl->flags &\n> V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> >\n> > - ControlList ctrls(dev->controls());\n> > + ControlList ctrls(data->sensor_->controls());\n> > ctrls.set(V4L2_CID_HFLIP, 0);\n> > ctrls.set(V4L2_CID_VFLIP, 0);\n> > data->setSensorControls(ctrls);\n> > @@ -1082,9 +1152,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator\n> *enumerator)\n> >\n> > /* Look for a valid Bayer format. */\n> > BayerFormat bayerFormat;\n> > - for (const auto &iter : dev->formats()) {\n> > - V4L2PixelFormat v4l2Format = iter.first;\n> > - bayerFormat =\n> BayerFormat::fromV4L2PixelFormat(v4l2Format);\n> > + for (const auto &iter : data->sensorFormats_) {\n> > + bayerFormat = mbusCodeToBayerFormat(iter.first);\n> > if (bayerFormat.isValid())\n> > break;\n> > }\n> > @@ -1271,7 +1340,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > }\n> > }\n> >\n> > - entityControls.emplace(0,\n> unicam_[Unicam::Image].dev()->controls());\n> > + entityControls.emplace(0, sensor_->controls());\n> > entityControls.emplace(1, isp_[Isp::Input].dev()->controls());\n> >\n> > /* Always send the user transform to the IPA. */\n> > @@ -1406,10 +1475,10 @@ void\n> RPiCameraData::setSensorControls(ControlList &controls)\n> > ControlList vblank_ctrl;\n> >\n> > vblank_ctrl.set(V4L2_CID_VBLANK,\n> controls.get(V4L2_CID_VBLANK));\n> > - unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> > + sensor_->setControls(&vblank_ctrl);\n> > }\n> >\n> > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > + sensor_->setControls(&controls);\n> > }\n> >\n> > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7C0B4BF415\n\tfor ;\n\tWed, 27 Oct 2021 12:37:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 35AE764882;\n\tWed, 27 Oct 2021 14:37:01 +0200 (CEST)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DF3360123\n\tfor ;\n\tWed, 27 Oct 2021 14:36:59 +0200 (CEST)","by mail-lf1-x12a.google.com with SMTP id l13so5859848lfg.6\n\tfor ;\n\tWed, 27 Oct 2021 05:36:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"jpYxrcLK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=+MPGjIDkj/X++kn/NfvnzgvRkoKiZamXW+uXEORGwH0=;\n\tb=jpYxrcLKIh7VdcXB5Mh0n5yuTpnnJsFtMf7feASxW9X3+M8kGVgjD3vajbY5zOjIPJ\n\tZN5p5HkXVQe4ZFCGNsaKytllVoeLbbZ2KCJNhDaIi6nD8+kZj42XeacaM/LET6CVccLH\n\t+tqY+j3QzjUpUGFvxB/hF7LRXRArlJ+d+r4eI54NkZL1FtSgR+4oYZfd+IvSs8LB191l\n\tETsPRdIAdU/1VIHe8Bi4BVk4O+8LfG8rdOwlyUqQ1W8/9AFJ+6PBFP6QDpBuuC30Kx5l\n\t02lS4YF81rCIcM7vRrzW1QI2AeZ3Pv6KNdatmP4izzIOplB90Mb1XyPF9MaTAVhYaKTv\n\tEtCQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=+MPGjIDkj/X++kn/NfvnzgvRkoKiZamXW+uXEORGwH0=;\n\tb=RzWE5xt418zlCOZZIcE8+hZ4bBmmQItXogVFq94ir1GAeGHSWpOmOlB7vj8k/WkxRo\n\t/q2X4xoMIjqNtbWDb/x5q78U1HObeWMaCFQyXfknxuYPWqqST6qYMiTZTgCdB7d1asyh\n\tClDU36RyFm6d2491ASwdFzEgPB5fqNOozbOeGPiYJ/InkceKgqVnnz4k0x+4HnPmEzqb\n\tohhcZj3uv8FEsg120/cmioc0N8zD2Sg9s4TpmM4Nt9pb1RqY2bNFSy1GfwuyCqzl0nsh\n\tLzojOihwC5XDFnGA2ZrtaM8BNUPIUIquMdz8l3qc+HnLInLa6BU1src2TH6bpz1sszcu\n\tEHOQ==","X-Gm-Message-State":"AOAM530XH1KNRfbPdpFnGqAq7hatVfdNym5BFY2yORKpsspWaZTKKn25\n\tc4PyDBiLW7BaNk2P/NW48yRfMYD3aFm2+DDQLM78t7h7qCsg8g==","X-Google-Smtp-Source":"ABdhPJz03IPHZLlYUMrUu/hcpzClBc33Rn6iO0wvk6CezvEDS/LzLUvXjHXn4IQBi1QAjHzU3XuLOjxIVbRinF6DAmE=","X-Received":"by 2002:ac2:5304:: with SMTP id\n\tc4mr20395540lfh.687.1635338218626; \n\tWed, 27 Oct 2021 05:36:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20211027092803.3671096-1-naush@raspberrypi.com>\n\t<20211027092803.3671096-6-naush@raspberrypi.com>\n\t<163533683068.1184428.15438489259661859505@Monstersaurus>","In-Reply-To":"<163533683068.1184428.15438489259661859505@Monstersaurus>","From":"Naushir Patuck ","Date":"Wed, 27 Oct 2021 13:36:42 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"0000000000009bf39205cf54d92e\"","Subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20585,"web_url":"https://patchwork.libcamera.org/comment/20585/","msgid":"","date":"2021-10-27T13:51:31","subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 27, 2021 at 01:13:50PM +0100, Kieran Bingham wrote:\n> Quoting Naushir Patuck (2021-10-27 10:27:59)\n> > Switch the pipeline handler to use the new Unicam media controller based driver.\n> > With this change, we directly talk to the sensor device driver to set controls\n> > and set/get formats in the pipeline handler.\n> > \n> > This change requires the accompanying Raspberry Pi linux kernel change at\n> > https://github.com/raspberrypi/linux/pull/4645. If this kernel change is not\n> > present, the pipeline handler will fail to run with an error message informing\n> > the user to update the kernel build.\n> > \n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 185 ++++++++++++------\n> > 1 file changed, 127 insertions(+), 58 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 1634ca98f481..48f561d31a31 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -26,6 +26,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \"libcamera/internal/bayer_format.h\"\n> > @@ -48,6 +49,66 @@ LOG_DEFINE_CATEGORY(RPI)\n> > \n> > namespace {\n> > \n> > +/* Map of mbus codes to supported sizes reported by the sensor. */\n> > +using SensorFormats = std::map>;\n> > +\n> > +SensorFormats populateSensorFormats(std::unique_ptr &sensor)\n> > +{\n> > + SensorFormats formats;\n> > +\n> > + for (auto const mbusCode : sensor->mbusCodes())\n> > + formats.emplace(mbusCode, sensor->sizes(mbusCode));\n> > +\n> > + return formats;\n> > +}\n> > +\n> > +BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode)\n> > +{\n> > + static const std::unordered_map mbusCodeToBayer {\n> > + { MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG10_1X10, { BayerFormat::GBRG, 10, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG10_1X10, { BayerFormat::GRBG, 10, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB10_1X10, { BayerFormat::RGGB, 10, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SBGGR12_1X12, { BayerFormat::BGGR, 12, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG12_1X12, { BayerFormat::GBRG, 12, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG12_1X12, { BayerFormat::GRBG, 12, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB12_1X12, { BayerFormat::RGGB, 12, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SBGGR14_1X14, { BayerFormat::BGGR, 14, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG14_1X14, { BayerFormat::GBRG, 14, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG14_1X14, { BayerFormat::GRBG, 14, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB14_1X14, { BayerFormat::RGGB, 14, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::None } },\n> > + { MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::None } },\n> > + };\n> > +\n> > + const auto it = mbusCodeToBayer.find(mbusCode);\n> > + if (it != mbusCodeToBayer.end())\n> > + return it->second;\n> > +\n> > + return BayerFormat{};\n\nI'd go the other way around, returning early on errors.\n\n> > +}\n\nI was expecting media bus code <-> (V4L2)PixelFormat conversion here, as\nthat's the part that is device specific. I understand why it's\nconvenient to use the BayerFormat class as an intermediate format in the\nmedia bus code <-> BayerFormat <-> PixelFormat conversion. I'm a bit\nworried that it will cause issues later, especially when adding support\nfor YUV sensors (as BayerFormat won't be able to represent non-raw\nformats), but solving this can be deferred.\n\nIf you prefer going through BayerFormat, I think the map can be dropped,\nas it essentially duplicates the one from the BayerFormat class. This\nfunction should be kept though, given that a packing argument will be\nadded later.\n\n> > +\n> > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format)\n\nThe argument isn't modified, it should be const.\n\n> > +{\n> > + V4L2DeviceFormat deviceFormat;\n> > + BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code);\n> > +\n> > + ASSERT(bayer.isValid());\n> > +\n> > + bayer.packing = BayerFormat::Packing::CSI2Packed;\n> \n> I think this can be BayerFormat::CSI2Packed; It doesn't hurt to\n> fully qualify it I suspect, but the packing is used without the\n> ::Packing:: in the table above.\n> \n> If we always set BayerFormat::CSI2Packed though, why not do that in the\n> table above instead of initialising with BayerFormat::None?\n\nBayerFormat::None isn't very explicit. I'd rather turn the enum into a\nscoped enum. I've sent a patch to do so.\n\n> (Maybe I'll discover later that we don't alway do this ... ?)\n> \n> > + deviceFormat.fourcc = bayer.toV4L2PixelFormat();\n> > + deviceFormat.size = format.size;\n> > + return deviceFormat;\n> > +}\n> > +\n> > bool isRaw(PixelFormat &pixFmt)\n> > {\n> > /*\n> > @@ -74,11 +135,10 @@ double scoreFormat(double desired, double actual)\n> > return score;\n> > }\n> > \n> > -V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > - const Size &req)\n> > +V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &req)\n> > {\n> > double bestScore = std::numeric_limits::max(), score;\n> > - V4L2DeviceFormat bestMode;\n> > + V4L2SubdeviceFormat bestFormat;\n> > \n> > #define PENALTY_AR 1500.0\n> > #define PENALTY_8BIT 2000.0\n> > @@ -88,19 +148,18 @@ V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > \n> > /* Calculate the closest/best mode from the user requested size. */\n> > for (const auto &iter : formatsMap) {\n> > - V4L2PixelFormat v4l2Format = iter.first;\n> > - const PixelFormatInfo &info = PixelFormatInfo::info(v4l2Format);\n> > + const unsigned int mbusCode = iter.first;\n> > + const PixelFormat format = mbusCodeToBayerFormat(mbusCode).toPixelFormat();\n> \n> Aha, ok here it is ... so we can't initialise with Packed in the table.\n> \n> > + const PixelFormatInfo &info = PixelFormatInfo::info(format);\n> > \n> > - for (const SizeRange &sz : iter.second) {\n> > - double modeWidth = sz.contains(req) ? req.width : sz.max.width;\n> > - double modeHeight = sz.contains(req) ? req.height : sz.max.height;\n> > + for (const Size &size : iter.second) {\n> > double reqAr = static_cast(req.width) / req.height;\n> > - double modeAr = modeWidth / modeHeight;\n> > + double fmtAr = size.width / size.height;\n> > \n> > /* Score the dimensions for closeness. */\n> > - score = scoreFormat(req.width, modeWidth);\n> > - score += scoreFormat(req.height, modeHeight);\n> > - score += PENALTY_AR * scoreFormat(reqAr, modeAr);\n> > + score = scoreFormat(req.width, size.width);\n> > + score += scoreFormat(req.height, size.height);\n> > + score += PENALTY_AR * scoreFormat(reqAr, fmtAr);\n> > \n> > /* Add any penalties... this is not an exact science! */\n> > if (!info.packed)\n> > @@ -115,18 +174,18 @@ V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > \n> > if (score <= bestScore) {\n> > bestScore = score;\n> > - bestMode.fourcc = v4l2Format;\n> > - bestMode.size = Size(modeWidth, modeHeight);\n> > + bestFormat.mbus_code = mbusCode;\n> > + bestFormat.size = size;\n> > }\n> > \n> > - LOG(RPI, Info) << \"Mode: \" << modeWidth << \"x\" << modeHeight\n> > - << \" fmt \" << v4l2Format.toString()\n> > + LOG(RPI, Info) << \"Format: \" << size.toString()\n> > + << \" fmt \" << format.toString()\n> > << \" Score: \" << score\n> > << \" (best \" << bestScore << \")\";\n> > }\n> > }\n> > \n> > - return bestMode;\n> > + return bestFormat;\n> > }\n> > \n> > enum class Unicam : unsigned int { Image, Embedded };\n> > @@ -170,6 +229,7 @@ public:\n> > std::unique_ptr ipa_;\n> > \n> > std::unique_ptr sensor_;\n> > + SensorFormats sensorFormats_;\n> > /* Array of Unicam and ISP device streams and associated buffers/streams. */\n> > RPi::Device unicam_;\n> > RPi::Device isp_;\n> > @@ -352,9 +412,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > * Calculate the best sensor mode we can use based on\n> > * the user request.\n> > */\n> > - V4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats();\n> > - V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);\n> > - int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);\n> > + V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size);\n> > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);\n> > + int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);\n> > if (ret)\n> > return Invalid;\n> > \n> > @@ -366,7 +426,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > * fetch the \"native\" (i.e. untransformed) Bayer order,\n> > * because the sensor may currently be flipped!\n> > */\n> > - V4L2PixelFormat fourcc = sensorFormat.fourcc;\n> > + V4L2PixelFormat fourcc = unicamFormat.fourcc;\n> > if (data_->flipsAlterBayerOrder_) {\n> > BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);\n> > bayer.order = data_->nativeBayerOrder_;\n> > @@ -375,15 +435,15 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > }\n> > \n> > PixelFormat sensorPixFormat = fourcc.toPixelFormat();\n> > - if (cfg.size != sensorFormat.size ||\n> > + if (cfg.size != unicamFormat.size ||\n> > cfg.pixelFormat != sensorPixFormat) {\n> > - cfg.size = sensorFormat.size;\n> > + cfg.size = unicamFormat.size;\n> > cfg.pixelFormat = sensorPixFormat;\n> > status = Adjusted;\n> > }\n> > \n> > - cfg.stride = sensorFormat.planes[0].bpl;\n> > - cfg.frameSize = sensorFormat.planes[0].size;\n> > + cfg.stride = unicamFormat.planes[0].bpl;\n> > + cfg.frameSize = unicamFormat.planes[0].size;\n> > \n> > rawCount++;\n> > } else {\n> > @@ -472,7 +532,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> > CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > - V4L2DeviceFormat sensorFormat;\n> > + V4L2SubdeviceFormat sensorFormat;\n> > unsigned int bufferCount;\n> > PixelFormat pixelFormat;\n> > V4L2VideoDevice::Formats fmts;\n> > @@ -487,9 +547,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > switch (role) {\n> > case StreamRole::Raw:\n> > size = data->sensor_->resolution();\n> > - fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > - sensorFormat = findBestMode(fmts, size);\n> > - pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > + sensorFormat = findBestFormat(data->sensorFormats_, size);\n> > + pixelFormat = mbusCodeToBayerFormat(sensorFormat.mbus_code).toPixelFormat();\n> > ASSERT(pixelFormat.isValid());\n> > bufferCount = 2;\n> > rawCount++;\n> > @@ -599,32 +658,29 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > }\n> > \n> > /* First calculate the best sensor mode we can use based on the user request. */\n> > - V4L2VideoDevice::Formats fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > - V4L2DeviceFormat sensorFormat = findBestMode(fmts, rawStream ? sensorSize : maxSize);\n> > + V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);\n> > + ret = data->sensor_->setFormat(&sensorFormat);\n> > + if (ret)\n> > + return ret;\n> > \n> > /*\n> > * Unicam image output format. The ISP input format gets set at start,\n> > * just in case we have swapped bayer orders due to flips.\n> > */\n> > - ret = data->unicam_[Unicam::Image].dev()->setFormat(&sensorFormat);\n> > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);\n> > + ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);\n> > if (ret)\n> > return ret;\n> > \n> > - /*\n> > - * The control ranges associated with the sensor may need updating\n> > - * after a format change.\n> > - * \\todo Use the CameraSensor::setFormat API instead.\n> > - */\n> > - data->sensor_->updateControlInfo();\n> > -\n> > LOG(RPI, Info) << \"Sensor: \" << camera->id()\n> > - << \" - Selected mode: \" << sensorFormat.toString();\n> > + << \" - Selected sensor format: \" << sensorFormat.toString()\n> > + << \" - Selected unicam format: \" << unicamFormat.toString();\n> > \n> > /*\n> > * This format may be reset on start() if the bayer order has changed\n> > * because of flips in the sensor.\n> > */\n> > - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > + ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat);\n> > if (ret)\n> > return ret;\n> > \n> > @@ -746,8 +802,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > data->ispMinCropSize_ = testCrop.size();\n> > \n> > /* Adjust aspect ratio by providing crops on the input image. */\n> > - Size size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> > - Rectangle crop = size.centeredTo(Rectangle(sensorFormat.size).center());\n> > + Size size = unicamFormat.size.boundedToAspectRatio(maxSize);\n> > + Rectangle crop = size.centeredTo(Rectangle(unicamFormat.size).center());\n> > data->ispCrop_ = crop;\n> > \n> > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > @@ -761,8 +817,11 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > * supports it.\n> > */\n> > if (data->sensorMetadata_) {\n> > - format = {};\n> > + V4L2SubdeviceFormat embeddedFormat;\n> > +\n> > + data->sensor_->device()->getFormat(1, &embeddedFormat);\n> > format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);\n> > + format.planes[0].size = embeddedFormat.size.width * embeddedFormat.size.height;\n> > \n> > LOG(RPI, Debug) << \"Setting embedded data format.\";\n> > ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);\n> > @@ -847,9 +906,14 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > * IPA configure may have changed the sensor flips - hence the bayer\n> > * order. Get the sensor format and set the ISP input now.\n> > */\n> > - V4L2DeviceFormat sensorFormat;\n> > - data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n> > - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > + V4L2SubdeviceFormat sensorFormat;\n> > + data->sensor_->device()->getFormat(0, &sensorFormat);\n> > +\n> > + V4L2DeviceFormat ispFormat;\n> > + ispFormat.fourcc = mbusCodeToBayerFormat(sensorFormat.mbus_code).toV4L2PixelFormat();\n> > + ispFormat.size = sensorFormat.size;\n> > +\n> > + ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat);\n> > if (ret) {\n> > stop(camera);\n> > return ret;\n> > @@ -1004,6 +1068,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > if (data->sensor_->init())\n> > return false;\n> > \n> > + data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > +\n> > ipa::RPi::SensorConfig sensorConfig;\n> > if (data->loadIPA(&sensorConfig)) {\n> > LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > @@ -1030,6 +1096,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > return false;\n> > }\n> > \n> > + if (!(data->unicam_[Unicam::Image].dev()->caps().device_caps() & V4L2_CAP_IO_MC)) {\n> \n> I'd probably add this to v4l2_videodevice.h as hasMediaController() on\n> the V4L2Capabilities.\n> \n> Done: libcamera: v4l2_videodevice: provide hasMediaController()\n> \n> No requirement to change here, unless you want to/the above gets in\n> fast, it can be updated later.\n> \n> > + LOG(RPI, Error) << \"Unicam driver did not advertise V4L2_CAP_IO_MC, please update your kernel!\";\n> \n> I would probably have written this as \n> \t\"Unicam driver does not use the MediaController, please update your kernel!\";\n> \n> but I'm pleased to see we can check this at run time anyway.\n> \n> Nothing else stands out to me in this so:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > + return false;\n> > + }\n> > +\n> > /*\n> > * Setup our delayed control writer with the sensor default\n> > * gain and exposure delays. Mark VBLANK for priority write.\n> > @@ -1039,7 +1110,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },\n> > { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }\n> > };\n> > - data->delayedCtrls_ = std::make_unique(data->unicam_[Unicam::Image].dev(), params);\n> > + data->delayedCtrls_ = std::make_unique(data->sensor_->device(), params);\n> > data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> > \n> > /* Register the controls that the Raspberry Pi IPA can handle. */\n> > @@ -1066,15 +1137,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > * As part of answering the final question, we reset the camera to\n> > * no transform at all.\n> > */\n> > -\n> > - V4L2VideoDevice *dev = data->unicam_[Unicam::Image].dev();\n> > - const struct v4l2_query_ext_ctrl *hflipCtrl = dev->controlInfo(V4L2_CID_HFLIP);\n> > + const V4L2Subdevice *sensor = data->sensor_->device();\n> > + const struct v4l2_query_ext_ctrl *hflipCtrl = sensor->controlInfo(V4L2_CID_HFLIP);\n> > if (hflipCtrl) {\n> > /* We assume it will support vflips too... */\n> > data->supportsFlips_ = true;\n> > data->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> > \n> > - ControlList ctrls(dev->controls());\n> > + ControlList ctrls(data->sensor_->controls());\n> > ctrls.set(V4L2_CID_HFLIP, 0);\n> > ctrls.set(V4L2_CID_VFLIP, 0);\n> > data->setSensorControls(ctrls);\n> > @@ -1082,9 +1152,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > \n> > /* Look for a valid Bayer format. */\n> > BayerFormat bayerFormat;\n> > - for (const auto &iter : dev->formats()) {\n> > - V4L2PixelFormat v4l2Format = iter.first;\n> > - bayerFormat = BayerFormat::fromV4L2PixelFormat(v4l2Format);\n> > + for (const auto &iter : data->sensorFormats_) {\n> > + bayerFormat = mbusCodeToBayerFormat(iter.first);\n> > if (bayerFormat.isValid())\n> > break;\n> > }\n> > @@ -1271,7 +1340,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > }\n> > }\n> > \n> > - entityControls.emplace(0, unicam_[Unicam::Image].dev()->controls());\n> > + entityControls.emplace(0, sensor_->controls());\n> > entityControls.emplace(1, isp_[Isp::Input].dev()->controls());\n> > \n> > /* Always send the user transform to the IPA. */\n> > @@ -1406,10 +1475,10 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> > ControlList vblank_ctrl;\n> > \n> > vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK));\n> > - unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> > + sensor_->setControls(&vblank_ctrl);\n> > }\n> > \n> > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > + sensor_->setControls(&controls);\n> > }\n> > \n> > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 01477BF415\n\tfor ;\n\tWed, 27 Oct 2021 13:51:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 466FA64882;\n\tWed, 27 Oct 2021 15:51:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A08060123\n\tfor ;\n\tWed, 27 Oct 2021 15:51:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0969B596;\n\tWed, 27 Oct 2021 15:51:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"I/5IBQ7t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635342715;\n\tbh=1Ss1cN1+49c91RJy+bQxvjoRugklkmOvBSRjah6zrpo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I/5IBQ7t8R51H5VREK8ERAeMmDGy9ENRLmCMGLjCUFPgCJkEKgbqBgrCHso+yma+d\n\tg/cI0NevePsTzNDoAGtQZofZurncxn1BYE1cd+fAzmDAwMG2k2MvZELTa/+lRd6iLm\n\tKtKiaBrInqBAHXmHbBAGp4VqX+6NazvQzxqSk3C4=","Date":"Wed, 27 Oct 2021 16:51:31 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211027092803.3671096-1-naush@raspberrypi.com>\n\t<20211027092803.3671096-6-naush@raspberrypi.com>\n\t<163533683068.1184428.15438489259661859505@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163533683068.1184428.15438489259661859505@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20586,"web_url":"https://patchwork.libcamera.org/comment/20586/","msgid":"","date":"2021-10-27T14:12:01","subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 27, 2021 at 04:51:33PM +0300, Laurent Pinchart wrote:\n> On Wed, Oct 27, 2021 at 01:13:50PM +0100, Kieran Bingham wrote:\n> > Quoting Naushir Patuck (2021-10-27 10:27:59)\n> > > Switch the pipeline handler to use the new Unicam media controller based driver.\n> > > With this change, we directly talk to the sensor device driver to set controls\n> > > and set/get formats in the pipeline handler.\n> > > \n> > > This change requires the accompanying Raspberry Pi linux kernel change at\n> > > https://github.com/raspberrypi/linux/pull/4645. If this kernel change is not\n> > > present, the pipeline handler will fail to run with an error message informing\n> > > the user to update the kernel build.\n> > > \n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 185 ++++++++++++------\n> > > 1 file changed, 127 insertions(+), 58 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 1634ca98f481..48f561d31a31 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -26,6 +26,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \"libcamera/internal/bayer_format.h\"\n> > > @@ -48,6 +49,66 @@ LOG_DEFINE_CATEGORY(RPI)\n> > > \n> > > namespace {\n> > > \n> > > +/* Map of mbus codes to supported sizes reported by the sensor. */\n> > > +using SensorFormats = std::map>;\n> > > +\n> > > +SensorFormats populateSensorFormats(std::unique_ptr &sensor)\n> > > +{\n> > > + SensorFormats formats;\n> > > +\n> > > + for (auto const mbusCode : sensor->mbusCodes())\n> > > + formats.emplace(mbusCode, sensor->sizes(mbusCode));\n> > > +\n> > > + return formats;\n> > > +}\n> > > +\n> > > +BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode)\n> > > +{\n> > > + static const std::unordered_map mbusCodeToBayer {\n> > > + { MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGBRG8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGRBG8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SRGGB8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGBRG10_1X10, { BayerFormat::GBRG, 10, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGRBG10_1X10, { BayerFormat::GRBG, 10, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SRGGB10_1X10, { BayerFormat::RGGB, 10, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SBGGR12_1X12, { BayerFormat::BGGR, 12, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGBRG12_1X12, { BayerFormat::GBRG, 12, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGRBG12_1X12, { BayerFormat::GRBG, 12, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SRGGB12_1X12, { BayerFormat::RGGB, 12, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SBGGR14_1X14, { BayerFormat::BGGR, 14, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGBRG14_1X14, { BayerFormat::GBRG, 14, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGRBG14_1X14, { BayerFormat::GRBG, 14, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SRGGB14_1X14, { BayerFormat::RGGB, 14, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::None } },\n> > > + { MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::None } },\n> > > + };\n> > > +\n> > > + const auto it = mbusCodeToBayer.find(mbusCode);\n> > > + if (it != mbusCodeToBayer.end())\n> > > + return it->second;\n> > > +\n> > > + return BayerFormat{};\n> \n> I'd go the other way around, returning early on errors.\n> \n> > > +}\n> \n> I was expecting media bus code <-> (V4L2)PixelFormat conversion here, as\n> that's the part that is device specific. I understand why it's\n> convenient to use the BayerFormat class as an intermediate format in the\n> media bus code <-> BayerFormat <-> PixelFormat conversion. I'm a bit\n> worried that it will cause issues later, especially when adding support\n> for YUV sensors (as BayerFormat won't be able to represent non-raw\n> formats), but solving this can be deferred.\n> \n> If you prefer going through BayerFormat, I think the map can be dropped,\n> as it essentially duplicates the one from the BayerFormat class. This\n> function should be kept though, given that a packing argument will be\n> added later.\n\nActually, the packing argument is added to toV4L2DeviceFormat(), not\nmbusCodeToBayerFormat(), so BayerFormat::fromMbusCode() can be used\ndirectly.\n\n> > > +\n> > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format)\n> \n> The argument isn't modified, it should be const.\n> \n> > > +{\n> > > + V4L2DeviceFormat deviceFormat;\n> > > + BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code);\n> > > +\n> > > + ASSERT(bayer.isValid());\n> > > +\n> > > + bayer.packing = BayerFormat::Packing::CSI2Packed;\n> > \n> > I think this can be BayerFormat::CSI2Packed; It doesn't hurt to\n> > fully qualify it I suspect, but the packing is used without the\n> > ::Packing:: in the table above.\n> > \n> > If we always set BayerFormat::CSI2Packed though, why not do that in the\n> > table above instead of initialising with BayerFormat::None?\n> \n> BayerFormat::None isn't very explicit. I'd rather turn the enum into a\n> scoped enum. I've sent a patch to do so.\n> \n> > (Maybe I'll discover later that we don't alway do this ... ?)\n> > \n> > > + deviceFormat.fourcc = bayer.toV4L2PixelFormat();\n> > > + deviceFormat.size = format.size;\n> > > + return deviceFormat;\n> > > +}\n> > > +\n> > > bool isRaw(PixelFormat &pixFmt)\n> > > {\n> > > /*\n> > > @@ -74,11 +135,10 @@ double scoreFormat(double desired, double actual)\n> > > return score;\n> > > }\n> > > \n> > > -V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > > - const Size &req)\n> > > +V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &req)\n> > > {\n> > > double bestScore = std::numeric_limits::max(), score;\n> > > - V4L2DeviceFormat bestMode;\n> > > + V4L2SubdeviceFormat bestFormat;\n> > > \n> > > #define PENALTY_AR 1500.0\n> > > #define PENALTY_8BIT 2000.0\n> > > @@ -88,19 +148,18 @@ V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > > \n> > > /* Calculate the closest/best mode from the user requested size. */\n> > > for (const auto &iter : formatsMap) {\n> > > - V4L2PixelFormat v4l2Format = iter.first;\n> > > - const PixelFormatInfo &info = PixelFormatInfo::info(v4l2Format);\n> > > + const unsigned int mbusCode = iter.first;\n> > > + const PixelFormat format = mbusCodeToBayerFormat(mbusCode).toPixelFormat();\n> > \n> > Aha, ok here it is ... so we can't initialise with Packed in the table.\n> > \n> > > + const PixelFormatInfo &info = PixelFormatInfo::info(format);\n> > > \n> > > - for (const SizeRange &sz : iter.second) {\n> > > - double modeWidth = sz.contains(req) ? req.width : sz.max.width;\n> > > - double modeHeight = sz.contains(req) ? req.height : sz.max.height;\n> > > + for (const Size &size : iter.second) {\n> > > double reqAr = static_cast(req.width) / req.height;\n> > > - double modeAr = modeWidth / modeHeight;\n> > > + double fmtAr = size.width / size.height;\n> > > \n> > > /* Score the dimensions for closeness. */\n> > > - score = scoreFormat(req.width, modeWidth);\n> > > - score += scoreFormat(req.height, modeHeight);\n> > > - score += PENALTY_AR * scoreFormat(reqAr, modeAr);\n> > > + score = scoreFormat(req.width, size.width);\n> > > + score += scoreFormat(req.height, size.height);\n> > > + score += PENALTY_AR * scoreFormat(reqAr, fmtAr);\n> > > \n> > > /* Add any penalties... this is not an exact science! */\n> > > if (!info.packed)\n> > > @@ -115,18 +174,18 @@ V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > > \n> > > if (score <= bestScore) {\n> > > bestScore = score;\n> > > - bestMode.fourcc = v4l2Format;\n> > > - bestMode.size = Size(modeWidth, modeHeight);\n> > > + bestFormat.mbus_code = mbusCode;\n> > > + bestFormat.size = size;\n> > > }\n> > > \n> > > - LOG(RPI, Info) << \"Mode: \" << modeWidth << \"x\" << modeHeight\n> > > - << \" fmt \" << v4l2Format.toString()\n> > > + LOG(RPI, Info) << \"Format: \" << size.toString()\n> > > + << \" fmt \" << format.toString()\n> > > << \" Score: \" << score\n> > > << \" (best \" << bestScore << \")\";\n> > > }\n> > > }\n> > > \n> > > - return bestMode;\n> > > + return bestFormat;\n> > > }\n> > > \n> > > enum class Unicam : unsigned int { Image, Embedded };\n> > > @@ -170,6 +229,7 @@ public:\n> > > std::unique_ptr ipa_;\n> > > \n> > > std::unique_ptr sensor_;\n> > > + SensorFormats sensorFormats_;\n> > > /* Array of Unicam and ISP device streams and associated buffers/streams. */\n> > > RPi::Device unicam_;\n> > > RPi::Device isp_;\n> > > @@ -352,9 +412,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > * Calculate the best sensor mode we can use based on\n> > > * the user request.\n> > > */\n> > > - V4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats();\n> > > - V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);\n> > > - int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);\n> > > + V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size);\n> > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);\n> > > + int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);\n> > > if (ret)\n> > > return Invalid;\n> > > \n> > > @@ -366,7 +426,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > * fetch the \"native\" (i.e. untransformed) Bayer order,\n> > > * because the sensor may currently be flipped!\n> > > */\n> > > - V4L2PixelFormat fourcc = sensorFormat.fourcc;\n> > > + V4L2PixelFormat fourcc = unicamFormat.fourcc;\n> > > if (data_->flipsAlterBayerOrder_) {\n> > > BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);\n> > > bayer.order = data_->nativeBayerOrder_;\n> > > @@ -375,15 +435,15 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > }\n> > > \n> > > PixelFormat sensorPixFormat = fourcc.toPixelFormat();\n> > > - if (cfg.size != sensorFormat.size ||\n> > > + if (cfg.size != unicamFormat.size ||\n> > > cfg.pixelFormat != sensorPixFormat) {\n> > > - cfg.size = sensorFormat.size;\n> > > + cfg.size = unicamFormat.size;\n> > > cfg.pixelFormat = sensorPixFormat;\n> > > status = Adjusted;\n> > > }\n> > > \n> > > - cfg.stride = sensorFormat.planes[0].bpl;\n> > > - cfg.frameSize = sensorFormat.planes[0].size;\n> > > + cfg.stride = unicamFormat.planes[0].bpl;\n> > > + cfg.frameSize = unicamFormat.planes[0].size;\n> > > \n> > > rawCount++;\n> > > } else {\n> > > @@ -472,7 +532,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > {\n> > > RPiCameraData *data = cameraData(camera);\n> > > CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > > - V4L2DeviceFormat sensorFormat;\n> > > + V4L2SubdeviceFormat sensorFormat;\n> > > unsigned int bufferCount;\n> > > PixelFormat pixelFormat;\n> > > V4L2VideoDevice::Formats fmts;\n> > > @@ -487,9 +547,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > switch (role) {\n> > > case StreamRole::Raw:\n> > > size = data->sensor_->resolution();\n> > > - fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > > - sensorFormat = findBestMode(fmts, size);\n> > > - pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > > + sensorFormat = findBestFormat(data->sensorFormats_, size);\n> > > + pixelFormat = mbusCodeToBayerFormat(sensorFormat.mbus_code).toPixelFormat();\n> > > ASSERT(pixelFormat.isValid());\n> > > bufferCount = 2;\n> > > rawCount++;\n> > > @@ -599,32 +658,29 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > }\n> > > \n> > > /* First calculate the best sensor mode we can use based on the user request. */\n> > > - V4L2VideoDevice::Formats fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > > - V4L2DeviceFormat sensorFormat = findBestMode(fmts, rawStream ? sensorSize : maxSize);\n> > > + V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);\n> > > + ret = data->sensor_->setFormat(&sensorFormat);\n> > > + if (ret)\n> > > + return ret;\n> > > \n> > > /*\n> > > * Unicam image output format. The ISP input format gets set at start,\n> > > * just in case we have swapped bayer orders due to flips.\n> > > */\n> > > - ret = data->unicam_[Unicam::Image].dev()->setFormat(&sensorFormat);\n> > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);\n> > > + ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);\n> > > if (ret)\n> > > return ret;\n> > > \n> > > - /*\n> > > - * The control ranges associated with the sensor may need updating\n> > > - * after a format change.\n> > > - * \\todo Use the CameraSensor::setFormat API instead.\n> > > - */\n> > > - data->sensor_->updateControlInfo();\n> > > -\n> > > LOG(RPI, Info) << \"Sensor: \" << camera->id()\n> > > - << \" - Selected mode: \" << sensorFormat.toString();\n> > > + << \" - Selected sensor format: \" << sensorFormat.toString()\n> > > + << \" - Selected unicam format: \" << unicamFormat.toString();\n> > > \n> > > /*\n> > > * This format may be reset on start() if the bayer order has changed\n> > > * because of flips in the sensor.\n> > > */\n> > > - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > > + ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat);\n> > > if (ret)\n> > > return ret;\n> > > \n> > > @@ -746,8 +802,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > data->ispMinCropSize_ = testCrop.size();\n> > > \n> > > /* Adjust aspect ratio by providing crops on the input image. */\n> > > - Size size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> > > - Rectangle crop = size.centeredTo(Rectangle(sensorFormat.size).center());\n> > > + Size size = unicamFormat.size.boundedToAspectRatio(maxSize);\n> > > + Rectangle crop = size.centeredTo(Rectangle(unicamFormat.size).center());\n> > > data->ispCrop_ = crop;\n> > > \n> > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > > @@ -761,8 +817,11 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > * supports it.\n> > > */\n> > > if (data->sensorMetadata_) {\n> > > - format = {};\n> > > + V4L2SubdeviceFormat embeddedFormat;\n> > > +\n> > > + data->sensor_->device()->getFormat(1, &embeddedFormat);\n> > > format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);\n> > > + format.planes[0].size = embeddedFormat.size.width * embeddedFormat.size.height;\n> > > \n> > > LOG(RPI, Debug) << \"Setting embedded data format.\";\n> > > ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);\n> > > @@ -847,9 +906,14 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > * IPA configure may have changed the sensor flips - hence the bayer\n> > > * order. Get the sensor format and set the ISP input now.\n> > > */\n> > > - V4L2DeviceFormat sensorFormat;\n> > > - data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n> > > - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > > + V4L2SubdeviceFormat sensorFormat;\n> > > + data->sensor_->device()->getFormat(0, &sensorFormat);\n> > > +\n> > > + V4L2DeviceFormat ispFormat;\n> > > + ispFormat.fourcc = mbusCodeToBayerFormat(sensorFormat.mbus_code).toV4L2PixelFormat();\n> > > + ispFormat.size = sensorFormat.size;\n> > > +\n> > > + ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat);\n> > > if (ret) {\n> > > stop(camera);\n> > > return ret;\n> > > @@ -1004,6 +1068,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > if (data->sensor_->init())\n> > > return false;\n> > > \n> > > + data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > +\n> > > ipa::RPi::SensorConfig sensorConfig;\n> > > if (data->loadIPA(&sensorConfig)) {\n> > > LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > > @@ -1030,6 +1096,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > return false;\n> > > }\n> > > \n> > > + if (!(data->unicam_[Unicam::Image].dev()->caps().device_caps() & V4L2_CAP_IO_MC)) {\n> > \n> > I'd probably add this to v4l2_videodevice.h as hasMediaController() on\n> > the V4L2Capabilities.\n> > \n> > Done: libcamera: v4l2_videodevice: provide hasMediaController()\n> > \n> > No requirement to change here, unless you want to/the above gets in\n> > fast, it can be updated later.\n> > \n> > > + LOG(RPI, Error) << \"Unicam driver did not advertise V4L2_CAP_IO_MC, please update your kernel!\";\n> > \n> > I would probably have written this as \n> > \t\"Unicam driver does not use the MediaController, please update your kernel!\";\n> > \n> > but I'm pleased to see we can check this at run time anyway.\n> > \n> > Nothing else stands out to me in this so:\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > + return false;\n> > > + }\n> > > +\n> > > /*\n> > > * Setup our delayed control writer with the sensor default\n> > > * gain and exposure delays. Mark VBLANK for priority write.\n> > > @@ -1039,7 +1110,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },\n> > > { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }\n> > > };\n> > > - data->delayedCtrls_ = std::make_unique(data->unicam_[Unicam::Image].dev(), params);\n> > > + data->delayedCtrls_ = std::make_unique(data->sensor_->device(), params);\n> > > data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> > > \n> > > /* Register the controls that the Raspberry Pi IPA can handle. */\n> > > @@ -1066,15 +1137,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > * As part of answering the final question, we reset the camera to\n> > > * no transform at all.\n> > > */\n> > > -\n> > > - V4L2VideoDevice *dev = data->unicam_[Unicam::Image].dev();\n> > > - const struct v4l2_query_ext_ctrl *hflipCtrl = dev->controlInfo(V4L2_CID_HFLIP);\n> > > + const V4L2Subdevice *sensor = data->sensor_->device();\n> > > + const struct v4l2_query_ext_ctrl *hflipCtrl = sensor->controlInfo(V4L2_CID_HFLIP);\n> > > if (hflipCtrl) {\n> > > /* We assume it will support vflips too... */\n> > > data->supportsFlips_ = true;\n> > > data->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> > > \n> > > - ControlList ctrls(dev->controls());\n> > > + ControlList ctrls(data->sensor_->controls());\n> > > ctrls.set(V4L2_CID_HFLIP, 0);\n> > > ctrls.set(V4L2_CID_VFLIP, 0);\n> > > data->setSensorControls(ctrls);\n> > > @@ -1082,9 +1152,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > \n> > > /* Look for a valid Bayer format. */\n> > > BayerFormat bayerFormat;\n> > > - for (const auto &iter : dev->formats()) {\n> > > - V4L2PixelFormat v4l2Format = iter.first;\n> > > - bayerFormat = BayerFormat::fromV4L2PixelFormat(v4l2Format);\n> > > + for (const auto &iter : data->sensorFormats_) {\n> > > + bayerFormat = mbusCodeToBayerFormat(iter.first);\n> > > if (bayerFormat.isValid())\n> > > break;\n> > > }\n> > > @@ -1271,7 +1340,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > }\n> > > }\n> > > \n> > > - entityControls.emplace(0, unicam_[Unicam::Image].dev()->controls());\n> > > + entityControls.emplace(0, sensor_->controls());\n> > > entityControls.emplace(1, isp_[Isp::Input].dev()->controls());\n> > > \n> > > /* Always send the user transform to the IPA. */\n> > > @@ -1406,10 +1475,10 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> > > ControlList vblank_ctrl;\n> > > \n> > > vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK));\n> > > - unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> > > + sensor_->setControls(&vblank_ctrl);\n> > > }\n> > > \n> > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > > + sensor_->setControls(&controls);\n> > > }\n> > > \n> > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1D58EBDB1C\n\tfor ;\n\tWed, 27 Oct 2021 14:12:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 813C06487F;\n\tWed, 27 Oct 2021 16:12:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4F0960123\n\tfor ;\n\tWed, 27 Oct 2021 16:12:25 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2EEB1276;\n\tWed, 27 Oct 2021 16:12:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rUPyuiUS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635343945;\n\tbh=Dq6OkODm7SF4eWziE7dZBzK5nejSiwQKuVgisy6iIUw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rUPyuiUSifX9AL1LNu4/WdYG2TF4IIkUUGeHbtVJS3B0XztqVH2eWCnFx91AB1VFd\n\tjLmXmxAVFqrhObCwuyZJOnDxTOWlHuaUP/eWXp+4LYfMMmVEaoOWZQlqpZ/ivrVEp4\n\tfDAeUBJuB8VS8pOWhkb538isaRUBeNWxMx4v6+F0=","Date":"Wed, 27 Oct 2021 17:12:01 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211027092803.3671096-1-naush@raspberrypi.com>\n\t<20211027092803.3671096-6-naush@raspberrypi.com>\n\t<163533683068.1184428.15438489259661859505@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20587,"web_url":"https://patchwork.libcamera.org/comment/20587/","msgid":"","date":"2021-10-27T14:18:41","subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 27, 2021 at 05:12:03PM +0300, Laurent Pinchart wrote:\n> On Wed, Oct 27, 2021 at 04:51:33PM +0300, Laurent Pinchart wrote:\n> > On Wed, Oct 27, 2021 at 01:13:50PM +0100, Kieran Bingham wrote:\n> > > Quoting Naushir Patuck (2021-10-27 10:27:59)\n> > > > Switch the pipeline handler to use the new Unicam media controller based driver.\n> > > > With this change, we directly talk to the sensor device driver to set controls\n> > > > and set/get formats in the pipeline handler.\n> > > > \n> > > > This change requires the accompanying Raspberry Pi linux kernel change at\n> > > > https://github.com/raspberrypi/linux/pull/4645. If this kernel change is not\n> > > > present, the pipeline handler will fail to run with an error message informing\n> > > > the user to update the kernel build.\n> > > > \n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 185 ++++++++++++------\n> > > > 1 file changed, 127 insertions(+), 58 deletions(-)\n> > > > \n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 1634ca98f481..48f561d31a31 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -26,6 +26,7 @@\n> > > > #include \n> > > > \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > \n> > > > #include \"libcamera/internal/bayer_format.h\"\n> > > > @@ -48,6 +49,66 @@ LOG_DEFINE_CATEGORY(RPI)\n> > > > \n> > > > namespace {\n> > > > \n> > > > +/* Map of mbus codes to supported sizes reported by the sensor. */\n> > > > +using SensorFormats = std::map>;\n> > > > +\n> > > > +SensorFormats populateSensorFormats(std::unique_ptr &sensor)\n> > > > +{\n> > > > + SensorFormats formats;\n> > > > +\n> > > > + for (auto const mbusCode : sensor->mbusCodes())\n> > > > + formats.emplace(mbusCode, sensor->sizes(mbusCode));\n> > > > +\n> > > > + return formats;\n> > > > +}\n> > > > +\n> > > > +BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode)\n> > > > +{\n> > > > + static const std::unordered_map mbusCodeToBayer {\n> > > > + { MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGBRG8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGRBG8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SRGGB8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGBRG10_1X10, { BayerFormat::GBRG, 10, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGRBG10_1X10, { BayerFormat::GRBG, 10, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SRGGB10_1X10, { BayerFormat::RGGB, 10, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SBGGR12_1X12, { BayerFormat::BGGR, 12, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGBRG12_1X12, { BayerFormat::GBRG, 12, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGRBG12_1X12, { BayerFormat::GRBG, 12, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SRGGB12_1X12, { BayerFormat::RGGB, 12, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SBGGR14_1X14, { BayerFormat::BGGR, 14, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGBRG14_1X14, { BayerFormat::GBRG, 14, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGRBG14_1X14, { BayerFormat::GRBG, 14, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SRGGB14_1X14, { BayerFormat::RGGB, 14, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::None } },\n> > > > + { MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::None } },\n> > > > + };\n> > > > +\n> > > > + const auto it = mbusCodeToBayer.find(mbusCode);\n> > > > + if (it != mbusCodeToBayer.end())\n> > > > + return it->second;\n> > > > +\n> > > > + return BayerFormat{};\n> > \n> > I'd go the other way around, returning early on errors.\n> > \n> > > > +}\n> > \n> > I was expecting media bus code <-> (V4L2)PixelFormat conversion here, as\n> > that's the part that is device specific. I understand why it's\n> > convenient to use the BayerFormat class as an intermediate format in the\n> > media bus code <-> BayerFormat <-> PixelFormat conversion. I'm a bit\n> > worried that it will cause issues later, especially when adding support\n> > for YUV sensors (as BayerFormat won't be able to represent non-raw\n> > formats), but solving this can be deferred.\n> > \n> > If you prefer going through BayerFormat, I think the map can be dropped,\n> > as it essentially duplicates the one from the BayerFormat class. This\n> > function should be kept though, given that a packing argument will be\n> > added later.\n> \n> Actually, the packing argument is added to toV4L2DeviceFormat(), not\n> mbusCodeToBayerFormat(), so BayerFormat::fromMbusCode() can be used\n> directly.\n> \n> > > > +\n> > > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format)\n> > \n> > The argument isn't modified, it should be const.\n> > \n> > > > +{\n> > > > + V4L2DeviceFormat deviceFormat;\n> > > > + BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code);\n> > > > +\n> > > > + ASSERT(bayer.isValid());\n> > > > +\n> > > > + bayer.packing = BayerFormat::Packing::CSI2Packed;\n> > > \n> > > I think this can be BayerFormat::CSI2Packed; It doesn't hurt to\n> > > fully qualify it I suspect, but the packing is used without the\n> > > ::Packing:: in the table above.\n> > > \n> > > If we always set BayerFormat::CSI2Packed though, why not do that in the\n> > > table above instead of initialising with BayerFormat::None?\n> > \n> > BayerFormat::None isn't very explicit. I'd rather turn the enum into a\n> > scoped enum. I've sent a patch to do so.\n> > \n> > > (Maybe I'll discover later that we don't alway do this ... ?)\n> > > \n> > > > + deviceFormat.fourcc = bayer.toV4L2PixelFormat();\n> > > > + deviceFormat.size = format.size;\n> > > > + return deviceFormat;\n> > > > +}\n> > > > +\n> > > > bool isRaw(PixelFormat &pixFmt)\n> > > > {\n> > > > /*\n> > > > @@ -74,11 +135,10 @@ double scoreFormat(double desired, double actual)\n> > > > return score;\n> > > > }\n> > > > \n> > > > -V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > > > - const Size &req)\n> > > > +V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &req)\n> > > > {\n> > > > double bestScore = std::numeric_limits::max(), score;\n> > > > - V4L2DeviceFormat bestMode;\n> > > > + V4L2SubdeviceFormat bestFormat;\n> > > > \n> > > > #define PENALTY_AR 1500.0\n> > > > #define PENALTY_8BIT 2000.0\n> > > > @@ -88,19 +148,18 @@ V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > > > \n> > > > /* Calculate the closest/best mode from the user requested size. */\n> > > > for (const auto &iter : formatsMap) {\n> > > > - V4L2PixelFormat v4l2Format = iter.first;\n> > > > - const PixelFormatInfo &info = PixelFormatInfo::info(v4l2Format);\n> > > > + const unsigned int mbusCode = iter.first;\n> > > > + const PixelFormat format = mbusCodeToBayerFormat(mbusCode).toPixelFormat();\n> > > \n> > > Aha, ok here it is ... so we can't initialise with Packed in the table.\n> > > \n> > > > + const PixelFormatInfo &info = PixelFormatInfo::info(format);\n> > > > \n> > > > - for (const SizeRange &sz : iter.second) {\n> > > > - double modeWidth = sz.contains(req) ? req.width : sz.max.width;\n> > > > - double modeHeight = sz.contains(req) ? req.height : sz.max.height;\n> > > > + for (const Size &size : iter.second) {\n> > > > double reqAr = static_cast(req.width) / req.height;\n> > > > - double modeAr = modeWidth / modeHeight;\n> > > > + double fmtAr = size.width / size.height;\n> > > > \n> > > > /* Score the dimensions for closeness. */\n> > > > - score = scoreFormat(req.width, modeWidth);\n> > > > - score += scoreFormat(req.height, modeHeight);\n> > > > - score += PENALTY_AR * scoreFormat(reqAr, modeAr);\n> > > > + score = scoreFormat(req.width, size.width);\n> > > > + score += scoreFormat(req.height, size.height);\n> > > > + score += PENALTY_AR * scoreFormat(reqAr, fmtAr);\n> > > > \n> > > > /* Add any penalties... this is not an exact science! */\n> > > > if (!info.packed)\n> > > > @@ -115,18 +174,18 @@ V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap,\n> > > > \n> > > > if (score <= bestScore) {\n> > > > bestScore = score;\n> > > > - bestMode.fourcc = v4l2Format;\n> > > > - bestMode.size = Size(modeWidth, modeHeight);\n> > > > + bestFormat.mbus_code = mbusCode;\n> > > > + bestFormat.size = size;\n> > > > }\n> > > > \n> > > > - LOG(RPI, Info) << \"Mode: \" << modeWidth << \"x\" << modeHeight\n> > > > - << \" fmt \" << v4l2Format.toString()\n> > > > + LOG(RPI, Info) << \"Format: \" << size.toString()\n> > > > + << \" fmt \" << format.toString()\n> > > > << \" Score: \" << score\n> > > > << \" (best \" << bestScore << \")\";\n> > > > }\n> > > > }\n> > > > \n> > > > - return bestMode;\n> > > > + return bestFormat;\n> > > > }\n> > > > \n> > > > enum class Unicam : unsigned int { Image, Embedded };\n> > > > @@ -170,6 +229,7 @@ public:\n> > > > std::unique_ptr ipa_;\n> > > > \n> > > > std::unique_ptr sensor_;\n> > > > + SensorFormats sensorFormats_;\n> > > > /* Array of Unicam and ISP device streams and associated buffers/streams. */\n> > > > RPi::Device unicam_;\n> > > > RPi::Device isp_;\n> > > > @@ -352,9 +412,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > > * Calculate the best sensor mode we can use based on\n> > > > * the user request.\n> > > > */\n> > > > - V4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats();\n> > > > - V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);\n> > > > - int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);\n> > > > + V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size);\n> > > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);\n> > > > + int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);\n> > > > if (ret)\n> > > > return Invalid;\n> > > > \n> > > > @@ -366,7 +426,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > > * fetch the \"native\" (i.e. untransformed) Bayer order,\n> > > > * because the sensor may currently be flipped!\n> > > > */\n> > > > - V4L2PixelFormat fourcc = sensorFormat.fourcc;\n> > > > + V4L2PixelFormat fourcc = unicamFormat.fourcc;\n> > > > if (data_->flipsAlterBayerOrder_) {\n> > > > BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);\n> > > > bayer.order = data_->nativeBayerOrder_;\n> > > > @@ -375,15 +435,15 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > > }\n> > > > \n> > > > PixelFormat sensorPixFormat = fourcc.toPixelFormat();\n> > > > - if (cfg.size != sensorFormat.size ||\n> > > > + if (cfg.size != unicamFormat.size ||\n> > > > cfg.pixelFormat != sensorPixFormat) {\n> > > > - cfg.size = sensorFormat.size;\n> > > > + cfg.size = unicamFormat.size;\n> > > > cfg.pixelFormat = sensorPixFormat;\n> > > > status = Adjusted;\n> > > > }\n> > > > \n> > > > - cfg.stride = sensorFormat.planes[0].bpl;\n> > > > - cfg.frameSize = sensorFormat.planes[0].size;\n> > > > + cfg.stride = unicamFormat.planes[0].bpl;\n> > > > + cfg.frameSize = unicamFormat.planes[0].size;\n> > > > \n> > > > rawCount++;\n> > > > } else {\n> > > > @@ -472,7 +532,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > > {\n> > > > RPiCameraData *data = cameraData(camera);\n> > > > CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > > > - V4L2DeviceFormat sensorFormat;\n> > > > + V4L2SubdeviceFormat sensorFormat;\n> > > > unsigned int bufferCount;\n> > > > PixelFormat pixelFormat;\n> > > > V4L2VideoDevice::Formats fmts;\n> > > > @@ -487,9 +547,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > > switch (role) {\n> > > > case StreamRole::Raw:\n> > > > size = data->sensor_->resolution();\n> > > > - fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > > > - sensorFormat = findBestMode(fmts, size);\n> > > > - pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > > > + sensorFormat = findBestFormat(data->sensorFormats_, size);\n> > > > + pixelFormat = mbusCodeToBayerFormat(sensorFormat.mbus_code).toPixelFormat();\n\nThis means that we'll default to unpacked raw formats in memory,\nrequiring additional memory usage and bandwidth. Is it really the right\noption, shouldn't we default to packed formats ?\n\n> > > > ASSERT(pixelFormat.isValid());\n> > > > bufferCount = 2;\n> > > > rawCount++;\n> > > > @@ -599,32 +658,29 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > }\n> > > > \n> > > > /* First calculate the best sensor mode we can use based on the user request. */\n> > > > - V4L2VideoDevice::Formats fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > > > - V4L2DeviceFormat sensorFormat = findBestMode(fmts, rawStream ? sensorSize : maxSize);\n> > > > + V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);\n> > > > + ret = data->sensor_->setFormat(&sensorFormat);\n> > > > + if (ret)\n> > > > + return ret;\n> > > > \n> > > > /*\n> > > > * Unicam image output format. The ISP input format gets set at start,\n> > > > * just in case we have swapped bayer orders due to flips.\n> > > > */\n> > > > - ret = data->unicam_[Unicam::Image].dev()->setFormat(&sensorFormat);\n> > > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);\n> > > > + ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);\n> > > > if (ret)\n> > > > return ret;\n> > > > \n> > > > - /*\n> > > > - * The control ranges associated with the sensor may need updating\n> > > > - * after a format change.\n> > > > - * \\todo Use the CameraSensor::setFormat API instead.\n> > > > - */\n> > > > - data->sensor_->updateControlInfo();\n> > > > -\n> > > > LOG(RPI, Info) << \"Sensor: \" << camera->id()\n> > > > - << \" - Selected mode: \" << sensorFormat.toString();\n> > > > + << \" - Selected sensor format: \" << sensorFormat.toString()\n> > > > + << \" - Selected unicam format: \" << unicamFormat.toString();\n> > > > \n> > > > /*\n> > > > * This format may be reset on start() if the bayer order has changed\n> > > > * because of flips in the sensor.\n> > > > */\n> > > > - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > > > + ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat);\n> > > > if (ret)\n> > > > return ret;\n> > > > \n> > > > @@ -746,8 +802,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > data->ispMinCropSize_ = testCrop.size();\n> > > > \n> > > > /* Adjust aspect ratio by providing crops on the input image. */\n> > > > - Size size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> > > > - Rectangle crop = size.centeredTo(Rectangle(sensorFormat.size).center());\n> > > > + Size size = unicamFormat.size.boundedToAspectRatio(maxSize);\n> > > > + Rectangle crop = size.centeredTo(Rectangle(unicamFormat.size).center());\n> > > > data->ispCrop_ = crop;\n> > > > \n> > > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > > > @@ -761,8 +817,11 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > * supports it.\n> > > > */\n> > > > if (data->sensorMetadata_) {\n> > > > - format = {};\n> > > > + V4L2SubdeviceFormat embeddedFormat;\n> > > > +\n> > > > + data->sensor_->device()->getFormat(1, &embeddedFormat);\n> > > > format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);\n> > > > + format.planes[0].size = embeddedFormat.size.width * embeddedFormat.size.height;\n> > > > \n> > > > LOG(RPI, Debug) << \"Setting embedded data format.\";\n> > > > ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);\n> > > > @@ -847,9 +906,14 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > > * IPA configure may have changed the sensor flips - hence the bayer\n> > > > * order. Get the sensor format and set the ISP input now.\n> > > > */\n> > > > - V4L2DeviceFormat sensorFormat;\n> > > > - data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n> > > > - ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > > > + V4L2SubdeviceFormat sensorFormat;\n> > > > + data->sensor_->device()->getFormat(0, &sensorFormat);\n> > > > +\n> > > > + V4L2DeviceFormat ispFormat;\n> > > > + ispFormat.fourcc = mbusCodeToBayerFormat(sensorFormat.mbus_code).toV4L2PixelFormat();\n\nAnd unless I'm mistaken, this won't work if the user select a packed\nformat.\n\nHere I would use the V4L2PixelFormat that has been set on the Unicam\nvideo node, without going through the media bus code.\n\n> > > > + ispFormat.size = sensorFormat.size;\n> > > > +\n> > > > + ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat);\n> > > > if (ret) {\n> > > > stop(camera);\n> > > > return ret;\n> > > > @@ -1004,6 +1068,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > if (data->sensor_->init())\n> > > > return false;\n> > > > \n> > > > + data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > > +\n> > > > ipa::RPi::SensorConfig sensorConfig;\n> > > > if (data->loadIPA(&sensorConfig)) {\n> > > > LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > > > @@ -1030,6 +1096,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > return false;\n> > > > }\n> > > > \n> > > > + if (!(data->unicam_[Unicam::Image].dev()->caps().device_caps() & V4L2_CAP_IO_MC)) {\n> > > \n> > > I'd probably add this to v4l2_videodevice.h as hasMediaController() on\n> > > the V4L2Capabilities.\n> > > \n> > > Done: libcamera: v4l2_videodevice: provide hasMediaController()\n> > > \n> > > No requirement to change here, unless you want to/the above gets in\n> > > fast, it can be updated later.\n> > > \n> > > > + LOG(RPI, Error) << \"Unicam driver did not advertise V4L2_CAP_IO_MC, please update your kernel!\";\n> > > \n> > > I would probably have written this as \n> > > \t\"Unicam driver does not use the MediaController, please update your kernel!\";\n> > > \n> > > but I'm pleased to see we can check this at run time anyway.\n> > > \n> > > Nothing else stands out to me in this so:\n> > > \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > > + return false;\n> > > > + }\n> > > > +\n> > > > /*\n> > > > * Setup our delayed control writer with the sensor default\n> > > > * gain and exposure delays. Mark VBLANK for priority write.\n> > > > @@ -1039,7 +1110,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },\n> > > > { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }\n> > > > };\n> > > > - data->delayedCtrls_ = std::make_unique(data->unicam_[Unicam::Image].dev(), params);\n> > > > + data->delayedCtrls_ = std::make_unique(data->sensor_->device(), params);\n> > > > data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> > > > \n> > > > /* Register the controls that the Raspberry Pi IPA can handle. */\n> > > > @@ -1066,15 +1137,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > * As part of answering the final question, we reset the camera to\n> > > > * no transform at all.\n> > > > */\n> > > > -\n> > > > - V4L2VideoDevice *dev = data->unicam_[Unicam::Image].dev();\n> > > > - const struct v4l2_query_ext_ctrl *hflipCtrl = dev->controlInfo(V4L2_CID_HFLIP);\n> > > > + const V4L2Subdevice *sensor = data->sensor_->device();\n> > > > + const struct v4l2_query_ext_ctrl *hflipCtrl = sensor->controlInfo(V4L2_CID_HFLIP);\n> > > > if (hflipCtrl) {\n> > > > /* We assume it will support vflips too... */\n> > > > data->supportsFlips_ = true;\n> > > > data->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> > > > \n> > > > - ControlList ctrls(dev->controls());\n> > > > + ControlList ctrls(data->sensor_->controls());\n> > > > ctrls.set(V4L2_CID_HFLIP, 0);\n> > > > ctrls.set(V4L2_CID_VFLIP, 0);\n> > > > data->setSensorControls(ctrls);\n> > > > @@ -1082,9 +1152,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > \n> > > > /* Look for a valid Bayer format. */\n> > > > BayerFormat bayerFormat;\n> > > > - for (const auto &iter : dev->formats()) {\n> > > > - V4L2PixelFormat v4l2Format = iter.first;\n> > > > - bayerFormat = BayerFormat::fromV4L2PixelFormat(v4l2Format);\n> > > > + for (const auto &iter : data->sensorFormats_) {\n> > > > + bayerFormat = mbusCodeToBayerFormat(iter.first);\n> > > > if (bayerFormat.isValid())\n> > > > break;\n> > > > }\n> > > > @@ -1271,7 +1340,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > > }\n> > > > }\n> > > > \n> > > > - entityControls.emplace(0, unicam_[Unicam::Image].dev()->controls());\n> > > > + entityControls.emplace(0, sensor_->controls());\n> > > > entityControls.emplace(1, isp_[Isp::Input].dev()->controls());\n> > > > \n> > > > /* Always send the user transform to the IPA. */\n> > > > @@ -1406,10 +1475,10 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> > > > ControlList vblank_ctrl;\n> > > > \n> > > > vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK));\n> > > > - unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> > > > + sensor_->setControls(&vblank_ctrl);\n> > > > }\n> > > > \n> > > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > > > + sensor_->setControls(&controls);\n> > > > }\n> > > > \n> > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0C706BF415\n\tfor ;\n\tWed, 27 Oct 2021 14:19:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C2666487F;\n\tWed, 27 Oct 2021 16:19:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E939960123\n\tfor ;\n\tWed, 27 Oct 2021 16:19:04 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5EC41292;\n\tWed, 27 Oct 2021 16:19:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pZihXsXW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635344344;\n\tbh=X2wd9QXCPuf2XMDblp+gy8oi7hJ4J1uv1bWQxaiGC+4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pZihXsXWu2r1dodsRujlTQb+/VlXD6ypE2s1h1mTqlxx2aM4QFhte6NJWpB4j5A4x\n\tfeHXSW5FAuaFDoe/wO/1hlmzStkAloYn6+NUh2EpCMa+ogcygwG64MhpWc54joOCJE\n\txxfyB10kp06IZGXcpI1/wVodlXMz+25bcLzpNkX0=","Date":"Wed, 27 Oct 2021 17:18:41 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211027092803.3671096-1-naush@raspberrypi.com>\n\t<20211027092803.3671096-6-naush@raspberrypi.com>\n\t<163533683068.1184428.15438489259661859505@Monstersaurus>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 5/9] pipeline: raspberrypi: Convert\n\tthe pipeline handler to use media controller","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]