[{"id":2795,"web_url":"https://patchwork.libcamera.org/comment/2795/","msgid":"<20191006024815.GB20839@pendragon.ideasonboard.com>","date":"2019-10-06T03:48:15","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: Add Simple\n\tpipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Martijn,\n\nThank you for the patch.\n\nOn Fri, Oct 04, 2019 at 09:45:34PM +0200, Martijn Braam wrote:\n> This add a generic pipeline handler for simple pipelines. It currently\n> handles pipelines where there's only a PHY node and a sensor node and\n> the kernel sets up the graph.\n> \n> The current implementation can deal with sunxi sun6i-csi pipelines but\n> other simple pipelines can be added to the infos array.\n> \n> Signed-off-by: Martijn Braam <martijn@brixit.nl>\n> ---\n>  src/libcamera/pipeline/meson.build |   1 +\n>  src/libcamera/pipeline/simple.cpp  | 457 +++++++++++++++++++++++++++++\n>  2 files changed, 458 insertions(+)\n>  create mode 100644 src/libcamera/pipeline/simple.cpp\n> \n> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> index 0d46622..df95a43 100644\n> --- a/src/libcamera/pipeline/meson.build\n> +++ b/src/libcamera/pipeline/meson.build\n> @@ -1,6 +1,7 @@\n>  libcamera_sources += files([\n>      'uvcvideo.cpp',\n>      'vimc.cpp',\n> +    'simple.cpp'\n\nCould you please keep the entries alphabetically sorted ? The last entry\nshould also end with a comma. If the vimc entry hadn't ended with a\ncomma, your change would have been\n\n libcamera_sources += files([\n     'uvcvideo.cpp',\n-    'vimc.cpp'\n+    'vimc.cpp',\n+    'simple.cpp'\n ])\n\nEnding the last entry with a comma eases addition of new entries.\n\n>  ])\n>  \n>  subdir('ipu3')\n> diff --git a/src/libcamera/pipeline/simple.cpp b/src/libcamera/pipeline/simple.cpp\n> new file mode 100644\n> index 0000000..f150d7e\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/simple.cpp\n> @@ -0,0 +1,457 @@\n> +\n> +#include <algorithm>\n> +#include <array>\n> +#include <iomanip>\n> +#include <memory>\n> +#include <vector>\n> +\n> +#include <linux/media-bus-format.h>\n> +\n> +#include <libcamera/camera.h>\n> +#include <libcamera/request.h>\n> +#include <libcamera/stream.h>\n> +\n> +#include \"camera_sensor.h\"\n> +#include \"device_enumerator.h\"\n> +#include \"log.h\"\n> +#include \"media_device.h\"\n> +#include \"pipeline_handler.h\"\n> +#include \"utils.h\"\n> +#include \"v4l2_subdevice.h\"\n> +#include \"v4l2_videodevice.h\"\n> +\n> +namespace libcamera {\n\nOne blank line here plase.\n\n> +LOG_DEFINE_CATEGORY(Simple)\n\nI would name the log category SimplePipeline, as Simple is quite generic\nand would likely be confusing in log messages.\n\n> +\n> +struct SimplePipelineInfo {\n> +\tstd::string driverName;\n> +\tstd::string phyName;\n\nShould we use \"camera interface\" instead of \"PHY\", as the simple\npipeline handler shouldn't be limited to CSI-2 ? Field names could be\nabbreviated to interface or even intf.\n\n> +\tstd::string v4l2Name;\n> +\tunsigned int v4l2PixFmt;\n> +\tunsigned int mediaBusFmt;\n> +\tunsigned int maxWidth;\n> +\tunsigned int maxHeight;\n> +};\n> +\n> +class SimpleCameraData : public CameraData\n> +{\n> +public:\n> +\tSimpleCameraData(PipelineHandler *pipe)\n> +\t\t: CameraData(pipe), sensor_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\t~SimpleCameraData()\n> +\t{\n> +\t\tdelete sensor_;\n> +\t}\n> +\n> +\tStream stream_;\n> +\tCameraSensor *sensor_;\n> +};\n> +\n> +class SimpleCameraConfiguration : public CameraConfiguration\n> +{\n> +public:\n> +\tSimpleCameraConfiguration(Camera *camera, SimpleCameraData *data, const SimplePipelineInfo *pipelineInfo);\n\nCould you please wrap lines to keep them short ? The soft limit is 80\ncolumns and the hard limit 120. While you're below 120, the following\nwould be more readable.\n\n\tSimpleCameraConfiguration(Camera *camera, SimpleCameraData *data,\n\t\t\t\t  const SimplePipelineInfo *pipelineInfo);\n\n> +\n> +\tStatus validate() override;\n> +\n> +\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> +\n> +private:\n> +\t/*\n> +         * The SimpleCameraData instance is guaranteed to be valid as long as the\n> +         * corresponding Camera instance is valid. In order to borrow a\n> +         * reference to the camera data, store a new reference to the camera.\n> +         */\n\nPlease indent using tabs (here and for other comments below).\n\n> +\tstd::shared_ptr<Camera> camera_;\n> +\tconst SimpleCameraData *data_;\n> +\n> +\tV4L2SubdeviceFormat sensorFormat_;\n> +\n> +\tconst SimplePipelineInfo *pipelineInfo_;\n\nIf you want to shorten lines you can rename this to info_. Same for the\nidentically named field in PipelineHandlerSimple.\n\n> +};\n> +\n> +class PipelineHandlerSimple : public PipelineHandler\n> +{\n> +public:\n> +\tPipelineHandlerSimple(CameraManager *manager);\n> +\n\nNo need for a blank line betwen the constructor and destructor.\n\n> +\t~PipelineHandlerSimple();\n> +\n> +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> +\t\t\t\t\t\t   const StreamRoles &roles) override;\n> +\n> +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> +\n> +\tint allocateBuffers(Camera *camera,\n> +\t\t\t    const std::set<Stream *> &streams) override;\n> +\n> +\tint freeBuffers(Camera *camera,\n> +\t\t\tconst std::set<Stream *> &streams) override;\n> +\n> +\tint start(Camera *camera) override;\n> +\n> +\tvoid stop(Camera *camera) override;\n> +\n> +\tint queueRequest(Camera *camera, Request *request) override;\n> +\n> +\tbool match(DeviceEnumerator *enumerator) override;\n\nYou can also remove most of the blank lines to group related functions\ntogether. I recommend using the same style as the base PipelineHandler\nclass.\n\n> +\n> +private:\n> +\tSimpleCameraData *cameraData(const Camera *camera)\n> +\t{\n> +\t\treturn static_cast<SimpleCameraData *>(\n> +\t\t\tPipelineHandler::cameraData(camera));\n> +\t}\n> +\n> +\tint initLinks();\n> +\n> +\tint createCamera(MediaEntity *sensor);\n> +\n> +\tvoid bufferReady(Buffer *buffer);\n> +\n> +\tMediaDevice *media_;\n> +\tV4L2Subdevice *dphy_;\n> +\tV4L2VideoDevice *video_;\n> +\n> +\tCamera *activeCamera_;\n> +\n> +\tconst SimplePipelineInfo *pipelineInfo_;\n> +};\n> +\n> +SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,\n> +\t\t\t\t\t\t     SimpleCameraData *data,\n> +\t\t\t\t\t\t     const SimplePipelineInfo *pipelineInfo)\n> +\t: CameraConfiguration()\n> +{\n> +\tcamera_ = camera->shared_from_this();\n> +\tdata_ = data;\n> +\tpipelineInfo_ = pipelineInfo;\n> +}\n> +\n> +CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> +{\n> +\tconst CameraSensor *sensor = data_->sensor_;\n> +\tStatus status = Valid;\n> +\n> +\tif (config_.empty())\n> +\t\treturn Invalid;\n> +\n> +\t/* Cap the number of entries to the available streams. */\n> +\tif (config_.size() > 1) {\n> +\t\tconfig_.resize(1);\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\tStreamConfiguration &cfg = config_[0];\n> +\n> +\t/* Adjust the pixel format. */\n> +\tif (cfg.pixelFormat != pipelineInfo_->v4l2PixFmt) {\n> +\t\tLOG(Simple, Debug) << \"Adjusting pixel format\";\n> +\t\tcfg.pixelFormat = pipelineInfo_->v4l2PixFmt;\n> +\t\tstatus = Adjusted;\n> +\t}\n\nThis is the first big change that I think is required, you shouldn't\nhardcode the v4l2PixFmt, mediaBusFmt or max width/height in\nSimplePipelineInfo. They should instead be determined dynamically at\ninit time through the V4L2 API on the video node and subdevs.\n\n> +\n> +\t/* Select the sensor format. */\n> +\tsensorFormat_ = sensor->getFormat({ pipelineInfo_->mediaBusFmt },\n> +\t\t\t\t\t  cfg.size);\n> +\tif (!sensorFormat_.size.width || !sensorFormat_.size.height)\n> +\t\tsensorFormat_.size = sensor->resolution();\n> +\n> +\t/*\n> +         * Provide a suitable default that matches the sensor aspect\n> +         * ratio and clamp the size to the hardware bounds.\n> +         *\n> +         * \\todo: Check the hardware alignment constraints.\n> +         */\n> +\tconst Size size = cfg.size;\n> +\n> +\tunsigned int pipelineMaxWidth = std::min(sensorFormat_.size.width, pipelineInfo_->maxWidth);\n> +\tunsigned int pipelineMaxHeight = std::min(sensorFormat_.size.height, pipelineInfo_->maxHeight);\n> +\n> +\tif (!cfg.size.width || !cfg.size.height) {\n> +\t\tcfg.size.width = pipelineMaxWidth;\n> +\t\tcfg.size.height = pipelineMaxWidth * sensorFormat_.size.height / sensorFormat_.size.width;\n> +\t}\n> +\n> +\tcfg.size.width = std::min(pipelineMaxWidth, cfg.size.width);\n> +\tcfg.size.height = std::min(pipelineMaxHeight, cfg.size.height);\n> +\n> +\tcfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));\n> +\tcfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));\n> +\n> +\tif (cfg.size != size) {\n> +\t\tLOG(Simple, Debug)\n> +\t\t\t<< \"Adjusting size from \" << size.toString()\n> +\t\t\t<< \" to \" << cfg.size.toString();\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\tcfg.bufferCount = 3;\n> +\n> +\treturn status;\n> +}\n> +\n> +PipelineHandlerSimple::PipelineHandlerSimple(CameraManager *manager)\n> +\t: PipelineHandler(manager), dphy_(nullptr), video_(nullptr)\n> +{\n> +}\n> +\n> +PipelineHandlerSimple::~PipelineHandlerSimple()\n> +{\n> +\tdelete video_;\n> +\tdelete dphy_;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Pipeline Operations\n> + */\n> +\n> +CameraConfiguration *PipelineHandlerSimple::generateConfiguration(Camera *camera,\n> +\t\t\t\t\t\t\t\t  const StreamRoles &roles)\n> +{\n> +\tSimpleCameraData *data = cameraData(camera);\n> +\tCameraConfiguration *config = new SimpleCameraConfiguration(camera, data, PipelineHandlerSimple::pipelineInfo_);\n\nNo need to prefix pipelineInfo_ with PipelineHandlerSimple::. The\ncompiler will resolve symbols based on the context, as this method is a\nmember of the PipelineHandlerSimple class, it will look for\npipelineInfo_ there.\n\n\tCameraConfiguration *config = new SimpleCameraConfiguration(camera, data,\n\t\t\t\t\t\t\t\t    pipelineInfo_);\n\n> +\n> +\tif (roles.empty())\n> +\t\treturn config;\n> +\n> +\tStreamConfiguration cfg{};\n> +\tcfg.pixelFormat = pipelineInfo_->v4l2PixFmt;\n> +\tcfg.size = data->sensor_->resolution();\n> +\n> +\tconfig->addConfiguration(cfg);\n> +\n> +\tconfig->validate();\n> +\n> +\treturn config;\n> +}\n> +\n> +int PipelineHandlerSimple::configure(Camera *camera, CameraConfiguration *c)\n> +{\n> +\tSimpleCameraConfiguration *config =\n> +\t\tstatic_cast<SimpleCameraConfiguration *>(c);\n> +\tSimpleCameraData *data = cameraData(camera);\n> +\tStreamConfiguration &cfg = config->at(0);\n> +\tCameraSensor *sensor = data->sensor_;\n> +\tint ret;\n> +\n> +\t/*\n> +         * Configure the sensor links: enable the link corresponding to this\n> +         * camera and disable all the other sensor links.\n> +         */\n> +\tconst MediaPad *pad = dphy_->entity()->getPadByIndex(0);\n> +\n> +\tfor (MediaLink *link : pad->links()) {\n> +\t\tbool enable = link->source()->entity() == sensor->entity();\n> +\n> +\t\tif (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable)\n> +\t\t\tcontinue;\n> +\n> +\t\tLOG(Simple, Debug)\n> +\t\t\t<< (enable ? \"Enabling\" : \"Disabling\")\n> +\t\t\t<< \" link from sensor '\"\n> +\t\t\t<< link->source()->entity()->name()\n> +\t\t\t<< \"' to CSI-2 receiver\";\n> +\n> +\t\tret = link->setEnabled(enable);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\t/*\n> +         * Configure the format on the sensor output and propagate it through\n> +         * the pipeline.\n> +         */\n> +\tV4L2SubdeviceFormat format = config->sensorFormat();\n> +\tLOG(Simple, Debug) << \"Configuring sensor with \" << format.toString();\n> +\n> +\tret = sensor->setFormat(&format);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tLOG(Simple, Debug) << \"Sensor configured with \" << format.toString();\n> +\n> +\tV4L2DeviceFormat outputFormat = {};\n> +\toutputFormat.fourcc = cfg.pixelFormat;\n> +\toutputFormat.size = cfg.size;\n> +\toutputFormat.planesCount = 2;\n> +\n> +\tret = video_->setFormat(&outputFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (outputFormat.size != cfg.size ||\n> +\t    outputFormat.fourcc != cfg.pixelFormat) {\n> +\t\tLOG(Simple, Error)\n> +\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcfg.setStream(&data->stream_);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerSimple::allocateBuffers(Camera *camera,\n> +\t\t\t\t\t   const std::set<Stream *> &streams)\n> +{\n> +\tStream *stream = *streams.begin();\n> +\n> +\tif (stream->memoryType() == InternalMemory)\n> +\t\treturn video_->exportBuffers(&stream->bufferPool());\n> +\telse\n> +\t\treturn video_->importBuffers(&stream->bufferPool());\n> +}\n> +\n> +int PipelineHandlerSimple::freeBuffers(Camera *camera,\n> +\t\t\t\t       const std::set<Stream *> &streams)\n> +{\n> +\tif (video_->releaseBuffers())\n> +\t\tLOG(Simple, Error) << \"Failed to release buffers\";\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerSimple::start(Camera *camera)\n> +{\n> +\tint ret;\n> +\n> +\tret = video_->streamOn();\n> +\tif (ret)\n> +\t\tLOG(Simple, Error)\n> +\t\t\t<< \"Failed to start camera \" << camera->name();\n> +\n> +\tactiveCamera_ = camera;\n> +\n> +\treturn ret;\n\nYou can return 0 here.\n\n> +}\n> +\n> +void PipelineHandlerSimple::stop(Camera *camera)\n> +{\n> +\tint ret;\n> +\n> +\tret = video_->streamOff();\n> +\tif (ret)\n> +\t\tLOG(Simple, Warning)\n> +\t\t\t<< \"Failed to stop camera \" << camera->name();\n> +\n> +\tactiveCamera_ = nullptr;\n> +}\n> +\n> +int PipelineHandlerSimple::queueRequest(Camera *camera, Request *request)\n> +{\n> +\tSimpleCameraData *data = cameraData(camera);\n> +\tStream *stream = &data->stream_;\n> +\n> +\tBuffer *buffer = request->findBuffer(stream);\n> +\tif (!buffer) {\n> +\t\tLOG(Simple, Error)\n> +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\tint ret = video_->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tPipelineHandler::queueRequest(camera, request);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Match and Setup\n> + */\n> +\n> +int PipelineHandlerSimple::createCamera(MediaEntity *sensor)\n> +{\n> +\tint ret;\n> +\n> +\tstd::unique_ptr<SimpleCameraData> data =\n> +\t\tutils::make_unique<SimpleCameraData>(this);\n> +\n> +\tdata->sensor_ = new CameraSensor(sensor);\n> +\tret = data->sensor_->init();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstd::set<Stream *> streams{ &data->stream_ };\n> +\tstd::shared_ptr<Camera> camera =\n> +\t\tCamera::create(this, sensor->name(), streams);\n> +\tregisterCamera(std::move(camera), std::move(data));\n> +\n> +\treturn 0;\n> +}\n> +\n> +bool PipelineHandlerSimple::match(DeviceEnumerator *enumerator)\n> +{\n> +\tconst MediaPad *pad;\n> +\n> +\tstatic const SimplePipelineInfo infos[1] = {\n\nNo need to specify the array size, the compiler will deduce it from the\ninitialiser.\n\n> +\t\t{ .driverName = \"sun6i-csi\",\n> +\t\t  .phyName = \"sun6i-csi\",\n> +\t\t  .v4l2Name = \"sun6i-csi\",\n> +\t\t  .v4l2PixFmt = V4L2_PIX_FMT_UYVY,\n> +\t\t  .mediaBusFmt = MEDIA_BUS_FMT_UYVY8_2X8,\n> +\t\t  .maxWidth = 1280,\n> +\t\t  .maxHeight = 720 }\n\nPlease add a comma after the last elements, with a new line after the\nopening and before the closing curly braces.\n\n\t\t{\n\t\t\t.driverName = \"sun6i-csi\",\n\t\t\t.phyName = \"sun6i-csi\",\n\t\t\t.v4l2Name = \"sun6i-csi\",\n\t\t\t.v4l2PixFmt = V4L2_PIX_FMT_UYVY,\n\t\t\t.mediaBusFmt = MEDIA_BUS_FMT_UYVY8_2X8,\n\t\t\t.maxWidth = 1280,\n\t\t\t.maxHeight = 720,\n\t\t},\n\n> +\t};\n> +\n> +\tconst SimplePipelineInfo *ptr = infos;\n> +\tfor (int i = 0; i < 1; i++, ptr++) {\n\ni is never negative, it should be an unsigned int. You can replace the\nhardcoded 1 with ARRAY_SIZE(info). Or even better,\n\n\tfor (const SimplePipelineInfo *info : infos) {\n\n> +\t\tDeviceMatch dm(ptr->driverName);\n> +\t\tdm.add(ptr->phyName);\n> +\n> +\t\tmedia_ = acquireMediaDevice(enumerator, dm);\n> +\t\tif (!media_)\n> +\t\t\tcontinue;\n> +\n> +\t\tPipelineHandlerSimple::pipelineInfo_ = ptr;\n> +\n\nAs the rest of the loop will only be executed once, how about\n\n\t\tif (media_) {\n\t\t\tpipelineInfo_ = info;\n\t\t\tbreak;\n\t\t}\n\t}\n\n\tif (!media_)\n\t\treturn false;\n\nand then lowering the indentation of the rest of the code ?\n\n> +\t\t/* Create the V4L2 subdevices we will need. */\n> +\t\tdphy_ = V4L2Subdevice::fromEntityName(media_, ptr->phyName);\n> +\t\tif (dphy_->open() < 0)\n> +\t\t\treturn false;\n> +\n> +\t\t/* Locate and open the capture video node. */\n> +\t\tvideo_ = V4L2VideoDevice::fromEntityName(media_, ptr->v4l2Name);\n> +\t\tif (video_->open() < 0)\n> +\t\t\treturn false;\n> +\n> +\t\tvideo_->bufferReady.connect(this, &PipelineHandlerSimple::bufferReady);\n> +\n> +\t\t/*\n> +             * Enumerate all sensors connected to the CSI-2 receiver and create one\n> +             * camera instance for each of them.\n> +             */\n> +\t\tpad = dphy_->entity()->getPadByIndex(0);\n> +\t\tif (!pad)\n> +\t\t\treturn false;\n\nI think we need to make this a bit more generic. I would like to avoid\nhaving to hardcode phyName and v4l2Name in SimplePipelineInfo, and\ninstead discover the pipeline by looking at entities. In a nutshell, the\ncode should\n\n- Find the video capture entity. You can do this by looking for an\n  entity that reports the MEDIA_ENT_F_IO_V4L function. You can return an\n  error if there's more than one.\n\n- Walk the pipeline from entity to entity by going through links to\n  locate sensors and the camera interface subdev. You should return an\n  error if the pipeline isn't linear (if an entity has more than one\n  sink pad or more than one source pad). You shouldn't assume that the\n  sink pad will be numbered 0 and the source pad numbered 1, you should\n  instead check the pad flags.\n\nFor now you can limit support to pipelines with the following topology.\n\nSensor 1 --\\\n...         }--> Camera interface subdev -> Video capture device\nSensor n --/\n\nWe can later support pipelines than have additional entities.\n\nThanks a lot for your work, we really appreciate your contribution. I\nrealise I'm requesting relatively large changes, so please feel free to\nrequest help if needed.\n\n> +\n> +\t\tfor (MediaLink *link : pad->links())\n> +\t\t\tcreateCamera(link->source()->entity());\n> +\n> +\t\treturn true;\n> +\t}\n> +\treturn false;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Buffer Handling\n> + */\n> +\n> +void PipelineHandlerSimple::bufferReady(Buffer *buffer)\n> +{\n> +\tASSERT(activeCamera_);\n> +\tLOG(Simple, Debug) << \"bufferReady\";\n\nI think you can drop the log message here. The V4L2VideoDevice class\nalready has a debug message before emitting the buffer ready signal, it\nshould be enough for debugging purpose.\n\n> +\tRequest *request = buffer->request();\n> +\tcompleteBuffer(activeCamera_, request, buffer);\n> +\tcompleteRequest(activeCamera_, request);\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerSimple);\n> +\n> +} // namespace libcamera","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88E3D61565\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  6 Oct 2019 04:48:19 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(modemcable151.96-160-184.mc.videotron.ca [184.160.96.151])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6EFC2DD;\n\tSun,  6 Oct 2019 04:48:18 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570330099;\n\tbh=qogEGt9ZuGegmq3XqAj1T8lg7hAM6wNN9nhsMuA3vco=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=frGWsqNw+ICcBOSvxVoHU1wpzSMDdK5KNWEHMh2ajx6IUvfC0Lx7eP96F77V6WK3i\n\t4oZ1D+tokwb1LOuzYyJKSvQBoS7/bgTdV03NgEEKU3ogDouj/lPisHGNzZid8UUKE8\n\ta4CXQwloVYaF4eYwAcWkKZAF6sOEVxmVO9FLjru8=","Date":"Sun, 6 Oct 2019 05:48:15 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Martijn Braam <martijn@brixit.nl>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191006024815.GB20839@pendragon.ideasonboard.com>","References":"<20191004194534.25287-1-martijn@brixit.nl>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191004194534.25287-1-martijn@brixit.nl>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: Add Simple\n\tpipeline","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sun, 06 Oct 2019 02:48:19 -0000"}},{"id":4012,"web_url":"https://patchwork.libcamera.org/comment/4012/","msgid":"<20200308192101.GA22614@pendragon.ideasonboard.com>","date":"2020-03-08T19:21:01","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: Add Simple\n\tpipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Martijn,\n\nI'm resuming work on this, and I've noticed that the simple.cpp file is\nmissing the copyright header. Could you please confirm that the source\nis covered by LGPL-2.1-or-later, and tell me what copyright notice I\nshould add (whether to attribute the copyright to you, or your employer)\n?\n\nOn Sun, Oct 06, 2019 at 05:48:15AM +0300, Laurent Pinchart wrote:\n> On Fri, Oct 04, 2019 at 09:45:34PM +0200, Martijn Braam wrote:\n> > This add a generic pipeline handler for simple pipelines. It currently\n> > handles pipelines where there's only a PHY node and a sensor node and\n> > the kernel sets up the graph.\n> > \n> > The current implementation can deal with sunxi sun6i-csi pipelines but\n> > other simple pipelines can be added to the infos array.\n> > \n> > Signed-off-by: Martijn Braam <martijn@brixit.nl>\n> > ---\n> >  src/libcamera/pipeline/meson.build |   1 +\n> >  src/libcamera/pipeline/simple.cpp  | 457 +++++++++++++++++++++++++++++\n> >  2 files changed, 458 insertions(+)\n> >  create mode 100644 src/libcamera/pipeline/simple.cpp\n> > \n> > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> > index 0d46622..df95a43 100644\n> > --- a/src/libcamera/pipeline/meson.build\n> > +++ b/src/libcamera/pipeline/meson.build\n> > @@ -1,6 +1,7 @@\n> >  libcamera_sources += files([\n> >      'uvcvideo.cpp',\n> >      'vimc.cpp',\n> > +    'simple.cpp'\n> \n> Could you please keep the entries alphabetically sorted ? The last entry\n> should also end with a comma. If the vimc entry hadn't ended with a\n> comma, your change would have been\n> \n>  libcamera_sources += files([\n>      'uvcvideo.cpp',\n> -    'vimc.cpp'\n> +    'vimc.cpp',\n> +    'simple.cpp'\n>  ])\n> \n> Ending the last entry with a comma eases addition of new entries.\n> \n> >  ])\n> >  \n> >  subdir('ipu3')\n> > diff --git a/src/libcamera/pipeline/simple.cpp b/src/libcamera/pipeline/simple.cpp\n> > new file mode 100644\n> > index 0000000..f150d7e\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/simple.cpp\n> > @@ -0,0 +1,457 @@\n> > +\n> > +#include <algorithm>\n> > +#include <array>\n> > +#include <iomanip>\n> > +#include <memory>\n> > +#include <vector>\n> > +\n> > +#include <linux/media-bus-format.h>\n> > +\n> > +#include <libcamera/camera.h>\n> > +#include <libcamera/request.h>\n> > +#include <libcamera/stream.h>\n> > +\n> > +#include \"camera_sensor.h\"\n> > +#include \"device_enumerator.h\"\n> > +#include \"log.h\"\n> > +#include \"media_device.h\"\n> > +#include \"pipeline_handler.h\"\n> > +#include \"utils.h\"\n> > +#include \"v4l2_subdevice.h\"\n> > +#include \"v4l2_videodevice.h\"\n> > +\n> > +namespace libcamera {\n> \n> One blank line here plase.\n> \n> > +LOG_DEFINE_CATEGORY(Simple)\n> \n> I would name the log category SimplePipeline, as Simple is quite generic\n> and would likely be confusing in log messages.\n> \n> > +\n> > +struct SimplePipelineInfo {\n> > +\tstd::string driverName;\n> > +\tstd::string phyName;\n> \n> Should we use \"camera interface\" instead of \"PHY\", as the simple\n> pipeline handler shouldn't be limited to CSI-2 ? Field names could be\n> abbreviated to interface or even intf.\n> \n> > +\tstd::string v4l2Name;\n> > +\tunsigned int v4l2PixFmt;\n> > +\tunsigned int mediaBusFmt;\n> > +\tunsigned int maxWidth;\n> > +\tunsigned int maxHeight;\n> > +};\n> > +\n> > +class SimpleCameraData : public CameraData\n> > +{\n> > +public:\n> > +\tSimpleCameraData(PipelineHandler *pipe)\n> > +\t\t: CameraData(pipe), sensor_(nullptr)\n> > +\t{\n> > +\t}\n> > +\n> > +\t~SimpleCameraData()\n> > +\t{\n> > +\t\tdelete sensor_;\n> > +\t}\n> > +\n> > +\tStream stream_;\n> > +\tCameraSensor *sensor_;\n> > +};\n> > +\n> > +class SimpleCameraConfiguration : public CameraConfiguration\n> > +{\n> > +public:\n> > +\tSimpleCameraConfiguration(Camera *camera, SimpleCameraData *data, const SimplePipelineInfo *pipelineInfo);\n> \n> Could you please wrap lines to keep them short ? The soft limit is 80\n> columns and the hard limit 120. While you're below 120, the following\n> would be more readable.\n> \n> \tSimpleCameraConfiguration(Camera *camera, SimpleCameraData *data,\n> \t\t\t\t  const SimplePipelineInfo *pipelineInfo);\n> \n> > +\n> > +\tStatus validate() override;\n> > +\n> > +\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> > +\n> > +private:\n> > +\t/*\n> > +         * The SimpleCameraData instance is guaranteed to be valid as long as the\n> > +         * corresponding Camera instance is valid. In order to borrow a\n> > +         * reference to the camera data, store a new reference to the camera.\n> > +         */\n> \n> Please indent using tabs (here and for other comments below).\n> \n> > +\tstd::shared_ptr<Camera> camera_;\n> > +\tconst SimpleCameraData *data_;\n> > +\n> > +\tV4L2SubdeviceFormat sensorFormat_;\n> > +\n> > +\tconst SimplePipelineInfo *pipelineInfo_;\n> \n> If you want to shorten lines you can rename this to info_. Same for the\n> identically named field in PipelineHandlerSimple.\n> \n> > +};\n> > +\n> > +class PipelineHandlerSimple : public PipelineHandler\n> > +{\n> > +public:\n> > +\tPipelineHandlerSimple(CameraManager *manager);\n> > +\n> \n> No need for a blank line betwen the constructor and destructor.\n> \n> > +\t~PipelineHandlerSimple();\n> > +\n> > +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> > +\t\t\t\t\t\t   const StreamRoles &roles) override;\n> > +\n> > +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> > +\n> > +\tint allocateBuffers(Camera *camera,\n> > +\t\t\t    const std::set<Stream *> &streams) override;\n> > +\n> > +\tint freeBuffers(Camera *camera,\n> > +\t\t\tconst std::set<Stream *> &streams) override;\n> > +\n> > +\tint start(Camera *camera) override;\n> > +\n> > +\tvoid stop(Camera *camera) override;\n> > +\n> > +\tint queueRequest(Camera *camera, Request *request) override;\n> > +\n> > +\tbool match(DeviceEnumerator *enumerator) override;\n> \n> You can also remove most of the blank lines to group related functions\n> together. I recommend using the same style as the base PipelineHandler\n> class.\n> \n> > +\n> > +private:\n> > +\tSimpleCameraData *cameraData(const Camera *camera)\n> > +\t{\n> > +\t\treturn static_cast<SimpleCameraData *>(\n> > +\t\t\tPipelineHandler::cameraData(camera));\n> > +\t}\n> > +\n> > +\tint initLinks();\n> > +\n> > +\tint createCamera(MediaEntity *sensor);\n> > +\n> > +\tvoid bufferReady(Buffer *buffer);\n> > +\n> > +\tMediaDevice *media_;\n> > +\tV4L2Subdevice *dphy_;\n> > +\tV4L2VideoDevice *video_;\n> > +\n> > +\tCamera *activeCamera_;\n> > +\n> > +\tconst SimplePipelineInfo *pipelineInfo_;\n> > +};\n> > +\n> > +SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,\n> > +\t\t\t\t\t\t     SimpleCameraData *data,\n> > +\t\t\t\t\t\t     const SimplePipelineInfo *pipelineInfo)\n> > +\t: CameraConfiguration()\n> > +{\n> > +\tcamera_ = camera->shared_from_this();\n> > +\tdata_ = data;\n> > +\tpipelineInfo_ = pipelineInfo;\n> > +}\n> > +\n> > +CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > +{\n> > +\tconst CameraSensor *sensor = data_->sensor_;\n> > +\tStatus status = Valid;\n> > +\n> > +\tif (config_.empty())\n> > +\t\treturn Invalid;\n> > +\n> > +\t/* Cap the number of entries to the available streams. */\n> > +\tif (config_.size() > 1) {\n> > +\t\tconfig_.resize(1);\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\tStreamConfiguration &cfg = config_[0];\n> > +\n> > +\t/* Adjust the pixel format. */\n> > +\tif (cfg.pixelFormat != pipelineInfo_->v4l2PixFmt) {\n> > +\t\tLOG(Simple, Debug) << \"Adjusting pixel format\";\n> > +\t\tcfg.pixelFormat = pipelineInfo_->v4l2PixFmt;\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> \n> This is the first big change that I think is required, you shouldn't\n> hardcode the v4l2PixFmt, mediaBusFmt or max width/height in\n> SimplePipelineInfo. They should instead be determined dynamically at\n> init time through the V4L2 API on the video node and subdevs.\n> \n> > +\n> > +\t/* Select the sensor format. */\n> > +\tsensorFormat_ = sensor->getFormat({ pipelineInfo_->mediaBusFmt },\n> > +\t\t\t\t\t  cfg.size);\n> > +\tif (!sensorFormat_.size.width || !sensorFormat_.size.height)\n> > +\t\tsensorFormat_.size = sensor->resolution();\n> > +\n> > +\t/*\n> > +         * Provide a suitable default that matches the sensor aspect\n> > +         * ratio and clamp the size to the hardware bounds.\n> > +         *\n> > +         * \\todo: Check the hardware alignment constraints.\n> > +         */\n> > +\tconst Size size = cfg.size;\n> > +\n> > +\tunsigned int pipelineMaxWidth = std::min(sensorFormat_.size.width, pipelineInfo_->maxWidth);\n> > +\tunsigned int pipelineMaxHeight = std::min(sensorFormat_.size.height, pipelineInfo_->maxHeight);\n> > +\n> > +\tif (!cfg.size.width || !cfg.size.height) {\n> > +\t\tcfg.size.width = pipelineMaxWidth;\n> > +\t\tcfg.size.height = pipelineMaxWidth * sensorFormat_.size.height / sensorFormat_.size.width;\n> > +\t}\n> > +\n> > +\tcfg.size.width = std::min(pipelineMaxWidth, cfg.size.width);\n> > +\tcfg.size.height = std::min(pipelineMaxHeight, cfg.size.height);\n> > +\n> > +\tcfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));\n> > +\tcfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));\n> > +\n> > +\tif (cfg.size != size) {\n> > +\t\tLOG(Simple, Debug)\n> > +\t\t\t<< \"Adjusting size from \" << size.toString()\n> > +\t\t\t<< \" to \" << cfg.size.toString();\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\tcfg.bufferCount = 3;\n> > +\n> > +\treturn status;\n> > +}\n> > +\n> > +PipelineHandlerSimple::PipelineHandlerSimple(CameraManager *manager)\n> > +\t: PipelineHandler(manager), dphy_(nullptr), video_(nullptr)\n> > +{\n> > +}\n> > +\n> > +PipelineHandlerSimple::~PipelineHandlerSimple()\n> > +{\n> > +\tdelete video_;\n> > +\tdelete dphy_;\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Pipeline Operations\n> > + */\n> > +\n> > +CameraConfiguration *PipelineHandlerSimple::generateConfiguration(Camera *camera,\n> > +\t\t\t\t\t\t\t\t  const StreamRoles &roles)\n> > +{\n> > +\tSimpleCameraData *data = cameraData(camera);\n> > +\tCameraConfiguration *config = new SimpleCameraConfiguration(camera, data, PipelineHandlerSimple::pipelineInfo_);\n> \n> No need to prefix pipelineInfo_ with PipelineHandlerSimple::. The\n> compiler will resolve symbols based on the context, as this method is a\n> member of the PipelineHandlerSimple class, it will look for\n> pipelineInfo_ there.\n> \n> \tCameraConfiguration *config = new SimpleCameraConfiguration(camera, data,\n> \t\t\t\t\t\t\t\t    pipelineInfo_);\n> \n> > +\n> > +\tif (roles.empty())\n> > +\t\treturn config;\n> > +\n> > +\tStreamConfiguration cfg{};\n> > +\tcfg.pixelFormat = pipelineInfo_->v4l2PixFmt;\n> > +\tcfg.size = data->sensor_->resolution();\n> > +\n> > +\tconfig->addConfiguration(cfg);\n> > +\n> > +\tconfig->validate();\n> > +\n> > +\treturn config;\n> > +}\n> > +\n> > +int PipelineHandlerSimple::configure(Camera *camera, CameraConfiguration *c)\n> > +{\n> > +\tSimpleCameraConfiguration *config =\n> > +\t\tstatic_cast<SimpleCameraConfiguration *>(c);\n> > +\tSimpleCameraData *data = cameraData(camera);\n> > +\tStreamConfiguration &cfg = config->at(0);\n> > +\tCameraSensor *sensor = data->sensor_;\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +         * Configure the sensor links: enable the link corresponding to this\n> > +         * camera and disable all the other sensor links.\n> > +         */\n> > +\tconst MediaPad *pad = dphy_->entity()->getPadByIndex(0);\n> > +\n> > +\tfor (MediaLink *link : pad->links()) {\n> > +\t\tbool enable = link->source()->entity() == sensor->entity();\n> > +\n> > +\t\tif (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tLOG(Simple, Debug)\n> > +\t\t\t<< (enable ? \"Enabling\" : \"Disabling\")\n> > +\t\t\t<< \" link from sensor '\"\n> > +\t\t\t<< link->source()->entity()->name()\n> > +\t\t\t<< \"' to CSI-2 receiver\";\n> > +\n> > +\t\tret = link->setEnabled(enable);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/*\n> > +         * Configure the format on the sensor output and propagate it through\n> > +         * the pipeline.\n> > +         */\n> > +\tV4L2SubdeviceFormat format = config->sensorFormat();\n> > +\tLOG(Simple, Debug) << \"Configuring sensor with \" << format.toString();\n> > +\n> > +\tret = sensor->setFormat(&format);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tLOG(Simple, Debug) << \"Sensor configured with \" << format.toString();\n> > +\n> > +\tV4L2DeviceFormat outputFormat = {};\n> > +\toutputFormat.fourcc = cfg.pixelFormat;\n> > +\toutputFormat.size = cfg.size;\n> > +\toutputFormat.planesCount = 2;\n> > +\n> > +\tret = video_->setFormat(&outputFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tif (outputFormat.size != cfg.size ||\n> > +\t    outputFormat.fourcc != cfg.pixelFormat) {\n> > +\t\tLOG(Simple, Error)\n> > +\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcfg.setStream(&data->stream_);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int PipelineHandlerSimple::allocateBuffers(Camera *camera,\n> > +\t\t\t\t\t   const std::set<Stream *> &streams)\n> > +{\n> > +\tStream *stream = *streams.begin();\n> > +\n> > +\tif (stream->memoryType() == InternalMemory)\n> > +\t\treturn video_->exportBuffers(&stream->bufferPool());\n> > +\telse\n> > +\t\treturn video_->importBuffers(&stream->bufferPool());\n> > +}\n> > +\n> > +int PipelineHandlerSimple::freeBuffers(Camera *camera,\n> > +\t\t\t\t       const std::set<Stream *> &streams)\n> > +{\n> > +\tif (video_->releaseBuffers())\n> > +\t\tLOG(Simple, Error) << \"Failed to release buffers\";\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int PipelineHandlerSimple::start(Camera *camera)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tret = video_->streamOn();\n> > +\tif (ret)\n> > +\t\tLOG(Simple, Error)\n> > +\t\t\t<< \"Failed to start camera \" << camera->name();\n> > +\n> > +\tactiveCamera_ = camera;\n> > +\n> > +\treturn ret;\n> \n> You can return 0 here.\n> \n> > +}\n> > +\n> > +void PipelineHandlerSimple::stop(Camera *camera)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tret = video_->streamOff();\n> > +\tif (ret)\n> > +\t\tLOG(Simple, Warning)\n> > +\t\t\t<< \"Failed to stop camera \" << camera->name();\n> > +\n> > +\tactiveCamera_ = nullptr;\n> > +}\n> > +\n> > +int PipelineHandlerSimple::queueRequest(Camera *camera, Request *request)\n> > +{\n> > +\tSimpleCameraData *data = cameraData(camera);\n> > +\tStream *stream = &data->stream_;\n> > +\n> > +\tBuffer *buffer = request->findBuffer(stream);\n> > +\tif (!buffer) {\n> > +\t\tLOG(Simple, Error)\n> > +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\tint ret = video_->queueBuffer(buffer);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tPipelineHandler::queueRequest(camera, request);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Match and Setup\n> > + */\n> > +\n> > +int PipelineHandlerSimple::createCamera(MediaEntity *sensor)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tstd::unique_ptr<SimpleCameraData> data =\n> > +\t\tutils::make_unique<SimpleCameraData>(this);\n> > +\n> > +\tdata->sensor_ = new CameraSensor(sensor);\n> > +\tret = data->sensor_->init();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tstd::set<Stream *> streams{ &data->stream_ };\n> > +\tstd::shared_ptr<Camera> camera =\n> > +\t\tCamera::create(this, sensor->name(), streams);\n> > +\tregisterCamera(std::move(camera), std::move(data));\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +bool PipelineHandlerSimple::match(DeviceEnumerator *enumerator)\n> > +{\n> > +\tconst MediaPad *pad;\n> > +\n> > +\tstatic const SimplePipelineInfo infos[1] = {\n> \n> No need to specify the array size, the compiler will deduce it from the\n> initialiser.\n> \n> > +\t\t{ .driverName = \"sun6i-csi\",\n> > +\t\t  .phyName = \"sun6i-csi\",\n> > +\t\t  .v4l2Name = \"sun6i-csi\",\n> > +\t\t  .v4l2PixFmt = V4L2_PIX_FMT_UYVY,\n> > +\t\t  .mediaBusFmt = MEDIA_BUS_FMT_UYVY8_2X8,\n> > +\t\t  .maxWidth = 1280,\n> > +\t\t  .maxHeight = 720 }\n> \n> Please add a comma after the last elements, with a new line after the\n> opening and before the closing curly braces.\n> \n> \t\t{\n> \t\t\t.driverName = \"sun6i-csi\",\n> \t\t\t.phyName = \"sun6i-csi\",\n> \t\t\t.v4l2Name = \"sun6i-csi\",\n> \t\t\t.v4l2PixFmt = V4L2_PIX_FMT_UYVY,\n> \t\t\t.mediaBusFmt = MEDIA_BUS_FMT_UYVY8_2X8,\n> \t\t\t.maxWidth = 1280,\n> \t\t\t.maxHeight = 720,\n> \t\t},\n> \n> > +\t};\n> > +\n> > +\tconst SimplePipelineInfo *ptr = infos;\n> > +\tfor (int i = 0; i < 1; i++, ptr++) {\n> \n> i is never negative, it should be an unsigned int. You can replace the\n> hardcoded 1 with ARRAY_SIZE(info). Or even better,\n> \n> \tfor (const SimplePipelineInfo *info : infos) {\n> \n> > +\t\tDeviceMatch dm(ptr->driverName);\n> > +\t\tdm.add(ptr->phyName);\n> > +\n> > +\t\tmedia_ = acquireMediaDevice(enumerator, dm);\n> > +\t\tif (!media_)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tPipelineHandlerSimple::pipelineInfo_ = ptr;\n> > +\n> \n> As the rest of the loop will only be executed once, how about\n> \n> \t\tif (media_) {\n> \t\t\tpipelineInfo_ = info;\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> \n> \tif (!media_)\n> \t\treturn false;\n> \n> and then lowering the indentation of the rest of the code ?\n> \n> > +\t\t/* Create the V4L2 subdevices we will need. */\n> > +\t\tdphy_ = V4L2Subdevice::fromEntityName(media_, ptr->phyName);\n> > +\t\tif (dphy_->open() < 0)\n> > +\t\t\treturn false;\n> > +\n> > +\t\t/* Locate and open the capture video node. */\n> > +\t\tvideo_ = V4L2VideoDevice::fromEntityName(media_, ptr->v4l2Name);\n> > +\t\tif (video_->open() < 0)\n> > +\t\t\treturn false;\n> > +\n> > +\t\tvideo_->bufferReady.connect(this, &PipelineHandlerSimple::bufferReady);\n> > +\n> > +\t\t/*\n> > +             * Enumerate all sensors connected to the CSI-2 receiver and create one\n> > +             * camera instance for each of them.\n> > +             */\n> > +\t\tpad = dphy_->entity()->getPadByIndex(0);\n> > +\t\tif (!pad)\n> > +\t\t\treturn false;\n> \n> I think we need to make this a bit more generic. I would like to avoid\n> having to hardcode phyName and v4l2Name in SimplePipelineInfo, and\n> instead discover the pipeline by looking at entities. In a nutshell, the\n> code should\n> \n> - Find the video capture entity. You can do this by looking for an\n>   entity that reports the MEDIA_ENT_F_IO_V4L function. You can return an\n>   error if there's more than one.\n> \n> - Walk the pipeline from entity to entity by going through links to\n>   locate sensors and the camera interface subdev. You should return an\n>   error if the pipeline isn't linear (if an entity has more than one\n>   sink pad or more than one source pad). You shouldn't assume that the\n>   sink pad will be numbered 0 and the source pad numbered 1, you should\n>   instead check the pad flags.\n> \n> For now you can limit support to pipelines with the following topology.\n> \n> Sensor 1 --\\\n> ...         }--> Camera interface subdev -> Video capture device\n> Sensor n --/\n> \n> We can later support pipelines than have additional entities.\n> \n> Thanks a lot for your work, we really appreciate your contribution. I\n> realise I'm requesting relatively large changes, so please feel free to\n> request help if needed.\n> \n> > +\n> > +\t\tfor (MediaLink *link : pad->links())\n> > +\t\t\tcreateCamera(link->source()->entity());\n> > +\n> > +\t\treturn true;\n> > +\t}\n> > +\treturn false;\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Buffer Handling\n> > + */\n> > +\n> > +void PipelineHandlerSimple::bufferReady(Buffer *buffer)\n> > +{\n> > +\tASSERT(activeCamera_);\n> > +\tLOG(Simple, Debug) << \"bufferReady\";\n> \n> I think you can drop the log message here. The V4L2VideoDevice class\n> already has a debug message before emitting the buffer ready signal, it\n> should be enough for debugging purpose.\n> \n> > +\tRequest *request = buffer->request();\n> > +\tcompleteBuffer(activeCamera_, request, buffer);\n> > +\tcompleteRequest(activeCamera_, request);\n> > +}\n> > +\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerSimple);\n> > +\n> > +} // namespace libcamera","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["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 7793660425\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  8 Mar 2020 20:21:05 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D9FE5F;\n\tSun,  8 Mar 2020 20:21:04 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1583695264;\n\tbh=JSlksWpW1OatDmAw60malv7N9AxotSE7KUuk+GsmusU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H41hdhPBGtYkvs8bzt08GfSRPel/lRCyYub7P1loT0THrspBWPwCd6MJ0/9CyxZXx\n\tBxX+7lXzz7L4NpVetvV3bAQstDsfgBNilyJLvMr/6+hJCBiGLm8f0CEmy6BF0kbP+K\n\tXwHgXDF5bTTgPfma1B5isPz7hXIa6Asq6djdr0PA=","Date":"Sun, 8 Mar 2020 21:21:01 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Martijn Braam <martijn@brixit.nl>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200308192101.GA22614@pendragon.ideasonboard.com>","References":"<20191004194534.25287-1-martijn@brixit.nl>\n\t<20191006024815.GB20839@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191006024815.GB20839@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: Add Simple\n\tpipeline","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sun, 08 Mar 2020 19:21:05 -0000"}}]