[{"id":1370,"web_url":"https://patchwork.libcamera.org/comment/1370/","msgid":"<20190415220811.GB1980@bigcity.dyn.berto.se>","date":"2019-04-15T22:08:11","subject":"Re: [libcamera-devel] [PATCH 11/11] libcamera: pipeline: Add RKISP1\n\tpipeline","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-04-15 19:57:00 +0300, Laurent Pinchart wrote:\n> The pipeline handler for the Rockchip ISP creates one camera instance\n> per detected raw Bayer CSI-2 sensor. Parallel sensors and YUV sensors\n> are not supported yet.\n> \n> As the ISP has a single CSI-2 receiver, only one camera can be used at a\n> time. Mutual exclusion isn't implemented yet.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nI still want to test this before adding my tag so I need to setup my \nrkisp1 environment. Over all I think this looks good, I have some small \ncomments bellow which you might to have a go at while I get my hardware \nin line.\n\n> ---\n>  src/libcamera/pipeline/meson.build        |   1 +\n>  src/libcamera/pipeline/rkisp1/meson.build |   3 +\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp  | 448 ++++++++++++++++++++++\n>  3 files changed, 452 insertions(+)\n>  create mode 100644 src/libcamera/pipeline/rkisp1/meson.build\n>  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> \n> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> index 40bb26405b88..0d466225a72e 100644\n> --- a/src/libcamera/pipeline/meson.build\n> +++ b/src/libcamera/pipeline/meson.build\n> @@ -4,3 +4,4 @@ libcamera_sources += files([\n>  ])\n>  \n>  subdir('ipu3')\n> +subdir('rkisp1')\n> diff --git a/src/libcamera/pipeline/rkisp1/meson.build b/src/libcamera/pipeline/rkisp1/meson.build\n> new file mode 100644\n> index 000000000000..f1cc4046b5d0\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rkisp1/meson.build\n> @@ -0,0 +1,3 @@\n> +libcamera_sources += files([\n> +    'rkisp1.cpp',\n> +])\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> new file mode 100644\n> index 000000000000..887c879d9024\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -0,0 +1,448 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * rkisp1.cpp - Pipeline handler for Rockchip ISP1\n> + */\n> +\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_device.h\"\n> +#include \"v4l2_subdevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1)\n> +\n> +class PipelineHandlerRkISP1 : public PipelineHandler\n> +{\n> +public:\n> +\tPipelineHandlerRkISP1(CameraManager *manager);\n> +\t~PipelineHandlerRkISP1();\n> +\n> +\tCameraConfiguration streamConfiguration(Camera *camera,\n> +\t\tconst std::vector<StreamUsage> &usages) override;\n> +\tint configureStreams(Camera *camera,\n> +\t\t\t     const CameraConfiguration &config) override;\n> +\n> +\tint allocateBuffers(Camera *camera, Stream *stream) override;\n> +\tint freeBuffers(Camera *camera, Stream *stream) override;\n> +\n> +\tint start(Camera *camera) override;\n> +\tvoid stop(Camera *camera) override;\n> +\n> +\tint queueRequest(Camera *camera, Request *request) override;\n> +\n> +\tbool match(DeviceEnumerator *enumerator) override;\n> +\n> +private:\n> +\tclass RkISP1CameraData : public CameraData\n> +\t{\n> +\tpublic:\n> +\t\tRkISP1CameraData(PipelineHandler *pipe)\n> +\t\t\t: CameraData(pipe), sensor_(nullptr)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\t~RkISP1CameraData()\n> +\t\t{\n> +\t\t\tdelete sensor_;\n> +\t\t}\n> +\n> +\t\tStream stream_;\n> +\t\tCameraSensor *sensor_;\n> +\t};\n> +\n> +\tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> +\n> +\tRkISP1CameraData *cameraData(const Camera *camera)\n> +\t{\n> +\t\treturn static_cast<RkISP1CameraData *>(\n> +\t\t\tPipelineHandler::cameraData(camera));\n> +\t}\n> +\n> +\tint initLinks();\n> +\tint createCamera(MediaEntity *sensor);\n> +\tvoid bufferReady(Buffer *buffer);\n> +\n> +\tstd::shared_ptr<MediaDevice> media_;\n> +\tV4L2Subdevice *dphy_;\n> +\tV4L2Subdevice *isp_;\n> +\tV4L2Device *video_;\n> +\n> +\tCamera *activeCamera_;\n> +};\n> +\n> +PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> +\t: PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),\n> +\t  video_(nullptr)\n> +{\n> +}\n> +\n> +PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n> +{\n> +\tdelete video_;\n> +\tdelete isp_;\n> +\tdelete dphy_;\n> +\n> +\tif (media_)\n> +\t\tmedia_->release();\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Pipeline Operations\n> + */\n> +\n> +CameraConfiguration PipelineHandlerRkISP1::streamConfiguration(Camera *camera,\n> +\tconst std::vector<StreamUsage> &usages)\n> +{\n> +\tRkISP1CameraData *data = cameraData(camera);\n> +\tCameraConfiguration configs;\n> +\tStreamConfiguration config{};\n> +\n> +\tconst Size &resolution = data->sensor_->resolution();\n> +\tconfig.width = resolution.width;\n> +\tconfig.height = resolution.height;\n> +\tconfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> +\tconfig.bufferCount = RKISP1_BUFFER_COUNT;\n> +\n> +\tconfigs[&data->stream_] = config;\n> +\n> +\tLOG(RkISP1, Debug)\n> +\t\t<< \"Stream format set to \" << config.width << \"x\"\n> +\t\t<< config.height << \"-0x\" << std::hex << std::setfill('0')\n> +\t\t<< std::setw(8) << config.pixelFormat;\n> +\n> +\treturn configs;\n> +}\n> +\n> +int PipelineHandlerRkISP1::configureStreams(Camera *camera,\n> +\t\t\t\t\t    const CameraConfiguration &config)\n> +{\n> +\tRkISP1CameraData *data = cameraData(camera);\n> +\tconst StreamConfiguration &cfg = config[&data->stream_];\n> +\tCameraSensor *sensor = data->sensor_;\n> +\tint ret;\n> +\n> +\t/* Verify the configuration. */\n> +\tconst Size &resolution = sensor->resolution();\n> +\tif (cfg.width > resolution.width ||\n> +\t    cfg.height > resolution.height) {\n> +\t\tLOG(RkISP1, Error)\n> +\t\t\t<< \"Invalid stream size: larger than sensor resolution\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/*\n> +\t * Configure the sensor links: enable the link corresponding to this\n> +\t * camera and disable all the other sensor links.\n> +\t */\n> +\tconst MediaPad *pad = dphy_->entity()->getPadByIndex(0);\n> +\n> +\tret = media_->open();\n> +\tif (ret < 0)\n> +\t\treturn ret;\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(RkISP1, 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\tbreak;\n> +\t}\n> +\n> +\tmedia_->close();\n> +\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/*\n> +\t * Configure the format on the sensor output and propagate it through\n> +\t * the pipeline.\n> +\t */\n> +\tV4L2SubdeviceFormat format;\n> +\tformat = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> +\t\t\t\t     MEDIA_BUS_FMT_SGBRG12_1X12,\n> +\t\t\t\t     MEDIA_BUS_FMT_SGRBG12_1X12,\n> +\t\t\t\t     MEDIA_BUS_FMT_SRGGB12_1X12,\n> +\t\t\t\t     MEDIA_BUS_FMT_SBGGR10_1X10,\n> +\t\t\t\t     MEDIA_BUS_FMT_SGBRG10_1X10,\n> +\t\t\t\t     MEDIA_BUS_FMT_SGRBG10_1X10,\n> +\t\t\t\t     MEDIA_BUS_FMT_SRGGB10_1X10,\n> +\t\t\t\t     MEDIA_BUS_FMT_SBGGR8_1X8,\n> +\t\t\t\t     MEDIA_BUS_FMT_SGBRG8_1X8,\n> +\t\t\t\t     MEDIA_BUS_FMT_SGRBG8_1X8,\n> +\t\t\t\t     MEDIA_BUS_FMT_SRGGB8_1X8 },\n> +\t\t\t\t   Size(cfg.width, cfg.height));\n> +\tret = sensor->setFormat(&format);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tret = dphy_->setFormat(0, &format);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tret = dphy_->getFormat(1, &format);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tret = isp_->setFormat(0, &format);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tV4L2DeviceFormat outputFormat = {};\n> +\toutputFormat.width = cfg.width;\n> +\toutputFormat.height = cfg.height;\n> +\toutputFormat.fourcc = V4L2_PIX_FMT_NV12;\n> +\toutputFormat.planesCount = 2;\n> +\n> +\tret = video_->setFormat(&outputFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(RkISP1, Debug)\n> +\t\t<< \"Configured pipeline for \" << format.toString() << \" -> \"\n> +\t\t<< outputFormat.toString();\n\nIs this LOG needed as you in 3/11 print this in \nCamera::configureStreams() ?\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, Stream *stream)\n> +{\n> +\treturn video_->exportBuffers(&stream->bufferPool());\n> +}\n> +\n> +int PipelineHandlerRkISP1::freeBuffers(Camera *camera, Stream *stream)\n> +{\n> +\tif (video_->releaseBuffers())\n> +\t\tLOG(RkISP1, Error) << \"Failed to release buffers\";\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerRkISP1::start(Camera *camera)\n> +{\n> +\tint ret;\n> +\n> +\tret = video_->streamOn();\n> +\tif (ret)\n> +\t\tLOG(RkISP1, Error)\n> +\t\t\t<< \"Failed to start camera \" << camera->name();\n> +\n> +\tactiveCamera_ = camera;\n> +\n> +\treturn ret;\n> +}\n> +\n> +void PipelineHandlerRkISP1::stop(Camera *camera)\n> +{\n> +\tint ret;\n> +\n> +\tret = video_->streamOff();\n> +\tif (ret)\n> +\t\tLOG(RkISP1, Warning)\n> +\t\t\t<< \"Failed to stop camera \" << camera->name();\n> +\n> +\tPipelineHandler::stop(camera);\n> +\n> +\tactiveCamera_ = nullptr;\n> +}\n> +\n> +int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n> +{\n> +\tRkISP1CameraData *data = cameraData(camera);\n> +\tStream *stream = &data->stream_;\n> +\n> +\tBuffer *buffer = request->findBuffer(stream);\n> +\tif (!buffer) {\n> +\t\tLOG(RkISP1, Error)\n> +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> +\t\treturn -ENOENT;\n> +\t}\n\nShould we move this check to Camera::queueRequest() ?\n\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 PipelineHandlerRkISP1::initLinks()\n> +{\n> +\tMediaLink *link;\n> +\tint ret;\n> +\n> +\tret = media_->disableLinks();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tlink = media_->link(\"rockchip-sy-mipi-dphy\", 1, \"rkisp1-isp-subdev\", 0);\n> +\tif (!link)\n> +\t\treturn -ENODEV;\n> +\n> +\tret = link->setEnabled(true);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tlink = media_->link(\"rkisp1-isp-subdev\", 2, \"rkisp1_mainpath\", 0);\n> +\tif (!link)\n> +\t\treturn -ENODEV;\n> +\n> +\tret = link->setEnabled(true);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> +{\n> +\tint ret;\n> +\n> +\tstd::unique_ptr<RkISP1CameraData> data =\n> +\t\tutils::make_unique<RkISP1CameraData>(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 PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> +{\n> +\tint ret;\n> +\n> +\tDeviceMatch dm(\"rkisp1\");\n> +\tdm.add(\"rkisp1-isp-subdev\");\n> +\tdm.add(\"rkisp1_selfpath\");\n> +\tdm.add(\"rkisp1_mainpath\");\n> +\tdm.add(\"rkisp1-statistics\");\n> +\tdm.add(\"rkisp1-input-params\");\n> +\tdm.add(\"rockchip-sy-mipi-dphy\");\n> +\n> +\tmedia_ = enumerator->search(dm);\n> +\tif (!media_)\n> +\t\treturn false;\n> +\n> +\tmedia_->acquire();\n> +\n> +\tret = media_->open();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/* Create the V4L2 subdevices we will need. */\n> +\tMediaEntity *dphy = media_->getEntityByName(\"rockchip-sy-mipi-dphy\");\n> +\tdphy_ = new V4L2Subdevice(dphy);\n> +\tret = dphy_->open();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tMediaEntity *isp = media_->getEntityByName(\"rkisp1-isp-subdev\");\n> +\tisp_ = new V4L2Subdevice(isp);\n> +\tret = isp_->open();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/* Locate and open the capture video node. */\n> +\tvideo_ = new V4L2Device(media_->getEntityByName(\"rkisp1_mainpath\"));\n> +\tif (video_->open())\n> +\t\treturn false;\n> +\n> +\tvideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> +\n> +\t/* Configure default links. */\n> +\tret = initLinks();\n> +\tif (ret < 0) {\n> +\t\tLOG(RkISP1, Error) << \"Failed to setup links\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\t/*\n> +\t * Enumerate all sensors connected to the CSI-2 receiver and create one\n> +\t * camera instance for each of them.\n> +\t */\n> +\tconst MediaPad *pad = dphy->getPadByIndex(0);\n> +\tif (!pad) {\n> +\t\tret = -EINVAL;\n> +\t\tgoto done;\n> +\t}\n> +\n> +\tfor (MediaLink *link : pad->links()) {\n> +\t\t/*\n> +\t\t * A sensor is expected to have a single pad and expose the\n> +\t\t * SENSOR function. This is overly restrictive, a better logic\n> +\t\t * is needed to support sensor that expose multiple subdevs.\n> +\t\t *\n> +\t\t * \\todo Create a helper to identify sensors\n> +\t\t */\n> +\t\tMediaEntity *sensor = link->source()->entity();\n> +\t\tif (!sensor)\n> +\t\t\tcontinue;\n> +\t\tif (sensor->pads().size() != 1)\n> +\t\t\tcontinue;\n> +\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)\n> +\t\t\tcontinue;\n\nSome of these checks overlap with CameraSensor::init(). Should all of \nthem be moved there? createCamera() will fail if CameraSensor::init() \nfails anyhow.\n\n> +\n> +\t\tcreateCamera(sensor);\n> +\t}\n> +\n> +done:\n> +\tmedia_->close();\n> +\n> +\treturn ret == 0;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Buffer Handling\n> + */\n> +\n> +void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> +{\n> +\tASSERT(activeCamera_);\n> +\n> +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> +\tRequest *request = data->queuedRequests_.front();\n> +\n> +\tcompleteBuffer(activeCamera_, request, buffer);\n> +\tcompleteRequest(activeCamera_, request);\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n> +\n> +} /* namespace libcamera */\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B406560B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 16 Apr 2019 00:08:13 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id t30so14298624lfd.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 15 Apr 2019 15:08:13 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tx76sm10147139ljb.17.2019.04.15.15.08.12\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 15 Apr 2019 15:08:12 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=96kq7EWt7Io5DWd4IrxaNT9uGGdqLL+fRaC4ZeQ3WUQ=;\n\tb=GRMr5v8RluWMuWQE53BWDLWKNPcllHuj9jWlRA2+d0EpMg8kSeKvhqKm3q7sY4NdJO\n\tn4i8+2ZPy4eyjo7fne37gPenc0mNlwZNBTfjnJfgzsnR4gNeFucWPVGjupDxH//UK1Hw\n\texE+vJOlJm+yHNKQsP0WfRhHbcNZCwwrPM6MR1eKc5OIPOqnS1bdRuX2WFRQ8IAu/NFl\n\tLIwpA7jUAIwWpsVFfF/pvGPVf/72CNhMpAg7AGux3VwKvOmJL2vuX54PYkj3+Er9eZpX\n\tj2P5wMyPyt7AY/2CR8sFr7TjvAHVgA9etHnhSkaIWGX1Y/OzCGdJuiEhUVIUIrcLi4xn\n\tpTQg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=96kq7EWt7Io5DWd4IrxaNT9uGGdqLL+fRaC4ZeQ3WUQ=;\n\tb=pQQ6pvmHtR16Yb2Fdjcns10ejO7Ku9SRCkfKLipvIfNkVwJk+8fMRc+tkbrWLG2FFL\n\tOoPbr5t0ZrILqYicmCDtnovPXwQQgjwdq2ViF/uGnIlSX5dcBNQAiCLu7Fly8rCWovwn\n\tyEmSRotMTQEsWKiUQW6S5v5YMFndvGGusNya9V37E8F0W4I0CUVz4WumKZ/ntrC1R6Dt\n\tOvj61qCYCUq/EkyfC7UqdqK68SRI1M0/ddjImMcuHVdf+7MGFS+wp6oWNjJxKipL6zCh\n\tnoYNJrcEnCOxuo7PQu41vZcd9QbPXxRSt61Wa4XfiM3vSWtj+43BpE/1Tb5P1z97k+4N\n\t16FA==","X-Gm-Message-State":"APjAAAVe2jWl3oR3QFJsD0e/gJz2uJ1uLXDWETgyTpCL1OgzD9UJUedY\n\tDbVmAqewWpVnhWShpqci7Nu3jyolHjs=","X-Google-Smtp-Source":"APXvYqxESnXWZoOOn3w0Rwk1rHkEnjk8oZiadqM/Fo5i8iHXS2aEb92eRSb2/TjLpsM2HYYeyEbrew==","X-Received":"by 2002:a19:6911:: with SMTP id e17mr12595822lfc.5.1555366092801;\n\tMon, 15 Apr 2019 15:08:12 -0700 (PDT)","Date":"Tue, 16 Apr 2019 00:08:11 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415220811.GB1980@bigcity.dyn.berto.se>","References":"<20190415165700.22970-1-laurent.pinchart@ideasonboard.com>\n\t<20190415165700.22970-12-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415165700.22970-12-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH 11/11] libcamera: pipeline: Add RKISP1\n\tpipeline","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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":"Mon, 15 Apr 2019 22:08:14 -0000"}},{"id":1372,"web_url":"https://patchwork.libcamera.org/comment/1372/","msgid":"<20190415230952.GN17083@pendragon.ideasonboard.com>","date":"2019-04-15T23:09:52","subject":"Re: [libcamera-devel] [PATCH 11/11] libcamera: pipeline: Add RKISP1\n\tpipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Apr 16, 2019 at 12:08:11AM +0200, Niklas Söderlund wrote:\n> On 2019-04-15 19:57:00 +0300, Laurent Pinchart wrote:\n> > The pipeline handler for the Rockchip ISP creates one camera instance\n> > per detected raw Bayer CSI-2 sensor. Parallel sensors and YUV sensors\n> > are not supported yet.\n> > \n> > As the ISP has a single CSI-2 receiver, only one camera can be used at a\n> > time. Mutual exclusion isn't implemented yet.\n> > \n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> I still want to test this before adding my tag so I need to setup my \n> rkisp1 environment. Over all I think this looks good, I have some small \n> comments bellow which you might to have a go at while I get my hardware \n> in line.\n> \n> > ---\n> >  src/libcamera/pipeline/meson.build        |   1 +\n> >  src/libcamera/pipeline/rkisp1/meson.build |   3 +\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp  | 448 ++++++++++++++++++++++\n> >  3 files changed, 452 insertions(+)\n> >  create mode 100644 src/libcamera/pipeline/rkisp1/meson.build\n> >  create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > \n> > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> > index 40bb26405b88..0d466225a72e 100644\n> > --- a/src/libcamera/pipeline/meson.build\n> > +++ b/src/libcamera/pipeline/meson.build\n> > @@ -4,3 +4,4 @@ libcamera_sources += files([\n> >  ])\n> >  \n> >  subdir('ipu3')\n> > +subdir('rkisp1')\n> > diff --git a/src/libcamera/pipeline/rkisp1/meson.build b/src/libcamera/pipeline/rkisp1/meson.build\n> > new file mode 100644\n> > index 000000000000..f1cc4046b5d0\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/rkisp1/meson.build\n> > @@ -0,0 +1,3 @@\n> > +libcamera_sources += files([\n> > +    'rkisp1.cpp',\n> > +])\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > new file mode 100644\n> > index 000000000000..887c879d9024\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -0,0 +1,448 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * rkisp1.cpp - Pipeline handler for Rockchip ISP1\n> > + */\n> > +\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_device.h\"\n> > +#include \"v4l2_subdevice.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(RkISP1)\n> > +\n> > +class PipelineHandlerRkISP1 : public PipelineHandler\n> > +{\n> > +public:\n> > +\tPipelineHandlerRkISP1(CameraManager *manager);\n> > +\t~PipelineHandlerRkISP1();\n> > +\n> > +\tCameraConfiguration streamConfiguration(Camera *camera,\n> > +\t\tconst std::vector<StreamUsage> &usages) override;\n> > +\tint configureStreams(Camera *camera,\n> > +\t\t\t     const CameraConfiguration &config) override;\n> > +\n> > +\tint allocateBuffers(Camera *camera, Stream *stream) override;\n> > +\tint freeBuffers(Camera *camera, Stream *stream) override;\n> > +\n> > +\tint start(Camera *camera) override;\n> > +\tvoid stop(Camera *camera) override;\n> > +\n> > +\tint queueRequest(Camera *camera, Request *request) override;\n> > +\n> > +\tbool match(DeviceEnumerator *enumerator) override;\n> > +\n> > +private:\n> > +\tclass RkISP1CameraData : public CameraData\n> > +\t{\n> > +\tpublic:\n> > +\t\tRkISP1CameraData(PipelineHandler *pipe)\n> > +\t\t\t: CameraData(pipe), sensor_(nullptr)\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\t~RkISP1CameraData()\n> > +\t\t{\n> > +\t\t\tdelete sensor_;\n> > +\t\t}\n> > +\n> > +\t\tStream stream_;\n> > +\t\tCameraSensor *sensor_;\n> > +\t};\n> > +\n> > +\tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> > +\n> > +\tRkISP1CameraData *cameraData(const Camera *camera)\n> > +\t{\n> > +\t\treturn static_cast<RkISP1CameraData *>(\n> > +\t\t\tPipelineHandler::cameraData(camera));\n> > +\t}\n> > +\n> > +\tint initLinks();\n> > +\tint createCamera(MediaEntity *sensor);\n> > +\tvoid bufferReady(Buffer *buffer);\n> > +\n> > +\tstd::shared_ptr<MediaDevice> media_;\n> > +\tV4L2Subdevice *dphy_;\n> > +\tV4L2Subdevice *isp_;\n> > +\tV4L2Device *video_;\n> > +\n> > +\tCamera *activeCamera_;\n> > +};\n> > +\n> > +PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > +\t: PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),\n> > +\t  video_(nullptr)\n> > +{\n> > +}\n> > +\n> > +PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n> > +{\n> > +\tdelete video_;\n> > +\tdelete isp_;\n> > +\tdelete dphy_;\n> > +\n> > +\tif (media_)\n> > +\t\tmedia_->release();\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Pipeline Operations\n> > + */\n> > +\n> > +CameraConfiguration PipelineHandlerRkISP1::streamConfiguration(Camera *camera,\n> > +\tconst std::vector<StreamUsage> &usages)\n> > +{\n> > +\tRkISP1CameraData *data = cameraData(camera);\n> > +\tCameraConfiguration configs;\n> > +\tStreamConfiguration config{};\n> > +\n> > +\tconst Size &resolution = data->sensor_->resolution();\n> > +\tconfig.width = resolution.width;\n> > +\tconfig.height = resolution.height;\n> > +\tconfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> > +\tconfig.bufferCount = RKISP1_BUFFER_COUNT;\n> > +\n> > +\tconfigs[&data->stream_] = config;\n> > +\n> > +\tLOG(RkISP1, Debug)\n> > +\t\t<< \"Stream format set to \" << config.width << \"x\"\n> > +\t\t<< config.height << \"-0x\" << std::hex << std::setfill('0')\n> > +\t\t<< std::setw(8) << config.pixelFormat;\n> > +\n> > +\treturn configs;\n> > +}\n> > +\n> > +int PipelineHandlerRkISP1::configureStreams(Camera *camera,\n> > +\t\t\t\t\t    const CameraConfiguration &config)\n> > +{\n> > +\tRkISP1CameraData *data = cameraData(camera);\n> > +\tconst StreamConfiguration &cfg = config[&data->stream_];\n> > +\tCameraSensor *sensor = data->sensor_;\n> > +\tint ret;\n> > +\n> > +\t/* Verify the configuration. */\n> > +\tconst Size &resolution = sensor->resolution();\n> > +\tif (cfg.width > resolution.width ||\n> > +\t    cfg.height > resolution.height) {\n> > +\t\tLOG(RkISP1, Error)\n> > +\t\t\t<< \"Invalid stream size: larger than sensor resolution\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Configure the sensor links: enable the link corresponding to this\n> > +\t * camera and disable all the other sensor links.\n> > +\t */\n> > +\tconst MediaPad *pad = dphy_->entity()->getPadByIndex(0);\n> > +\n> > +\tret = media_->open();\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\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(RkISP1, 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\tbreak;\n> > +\t}\n> > +\n> > +\tmedia_->close();\n> > +\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\t/*\n> > +\t * Configure the format on the sensor output and propagate it through\n> > +\t * the pipeline.\n> > +\t */\n> > +\tV4L2SubdeviceFormat format;\n> > +\tformat = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SGBRG12_1X12,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SGRBG12_1X12,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SRGGB12_1X12,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SBGGR10_1X10,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SGBRG10_1X10,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SGRBG10_1X10,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SRGGB10_1X10,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SBGGR8_1X8,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SGBRG8_1X8,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SGRBG8_1X8,\n> > +\t\t\t\t     MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > +\t\t\t\t   Size(cfg.width, cfg.height));\n> > +\tret = sensor->setFormat(&format);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tret = dphy_->setFormat(0, &format);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tret = dphy_->getFormat(1, &format);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tret = isp_->setFormat(0, &format);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tV4L2DeviceFormat outputFormat = {};\n> > +\toutputFormat.width = cfg.width;\n> > +\toutputFormat.height = cfg.height;\n> > +\toutputFormat.fourcc = V4L2_PIX_FMT_NV12;\n> > +\toutputFormat.planesCount = 2;\n> > +\n> > +\tret = video_->setFormat(&outputFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tLOG(RkISP1, Debug)\n> > +\t\t<< \"Configured pipeline for \" << format.toString() << \" -> \"\n> > +\t\t<< outputFormat.toString();\n> \n> Is this LOG needed as you in 3/11 print this in \n> Camera::configureStreams() ?\n\nThe first part could be useful, as it tells which format was configured\non the sensor (before scaling) so I think it's worth keeping it. I will\nremove the second part (outputFormat) and make it more explicit that the\nprinted format relates to the sensor.\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, Stream *stream)\n> > +{\n> > +\treturn video_->exportBuffers(&stream->bufferPool());\n> > +}\n> > +\n> > +int PipelineHandlerRkISP1::freeBuffers(Camera *camera, Stream *stream)\n> > +{\n> > +\tif (video_->releaseBuffers())\n> > +\t\tLOG(RkISP1, Error) << \"Failed to release buffers\";\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int PipelineHandlerRkISP1::start(Camera *camera)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tret = video_->streamOn();\n> > +\tif (ret)\n> > +\t\tLOG(RkISP1, Error)\n> > +\t\t\t<< \"Failed to start camera \" << camera->name();\n> > +\n> > +\tactiveCamera_ = camera;\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +void PipelineHandlerRkISP1::stop(Camera *camera)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tret = video_->streamOff();\n> > +\tif (ret)\n> > +\t\tLOG(RkISP1, Warning)\n> > +\t\t\t<< \"Failed to stop camera \" << camera->name();\n> > +\n> > +\tPipelineHandler::stop(camera);\n> > +\n> > +\tactiveCamera_ = nullptr;\n> > +}\n> > +\n> > +int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n> > +{\n> > +\tRkISP1CameraData *data = cameraData(camera);\n> > +\tStream *stream = &data->stream_;\n> > +\n> > +\tBuffer *buffer = request->findBuffer(stream);\n> > +\tif (!buffer) {\n> > +\t\tLOG(RkISP1, Error)\n> > +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> > +\t\treturn -ENOENT;\n> > +\t}\n> \n> Should we move this check to Camera::queueRequest() ?\n\nThe Camera class doesn't have access to data->stream_. I expect this to\nbe reworked with multiple streams support anyway, so I'd keep it as-is\nfor now.\n\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 PipelineHandlerRkISP1::initLinks()\n> > +{\n> > +\tMediaLink *link;\n> > +\tint ret;\n> > +\n> > +\tret = media_->disableLinks();\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tlink = media_->link(\"rockchip-sy-mipi-dphy\", 1, \"rkisp1-isp-subdev\", 0);\n> > +\tif (!link)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tret = link->setEnabled(true);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tlink = media_->link(\"rkisp1-isp-subdev\", 2, \"rkisp1_mainpath\", 0);\n> > +\tif (!link)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tret = link->setEnabled(true);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tstd::unique_ptr<RkISP1CameraData> data =\n> > +\t\tutils::make_unique<RkISP1CameraData>(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 PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tDeviceMatch dm(\"rkisp1\");\n> > +\tdm.add(\"rkisp1-isp-subdev\");\n> > +\tdm.add(\"rkisp1_selfpath\");\n> > +\tdm.add(\"rkisp1_mainpath\");\n> > +\tdm.add(\"rkisp1-statistics\");\n> > +\tdm.add(\"rkisp1-input-params\");\n> > +\tdm.add(\"rockchip-sy-mipi-dphy\");\n> > +\n> > +\tmedia_ = enumerator->search(dm);\n> > +\tif (!media_)\n> > +\t\treturn false;\n> > +\n> > +\tmedia_->acquire();\n> > +\n> > +\tret = media_->open();\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Create the V4L2 subdevices we will need. */\n> > +\tMediaEntity *dphy = media_->getEntityByName(\"rockchip-sy-mipi-dphy\");\n> > +\tdphy_ = new V4L2Subdevice(dphy);\n> > +\tret = dphy_->open();\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tMediaEntity *isp = media_->getEntityByName(\"rkisp1-isp-subdev\");\n> > +\tisp_ = new V4L2Subdevice(isp);\n> > +\tret = isp_->open();\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Locate and open the capture video node. */\n> > +\tvideo_ = new V4L2Device(media_->getEntityByName(\"rkisp1_mainpath\"));\n> > +\tif (video_->open())\n> > +\t\treturn false;\n> > +\n> > +\tvideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > +\n> > +\t/* Configure default links. */\n> > +\tret = initLinks();\n> > +\tif (ret < 0) {\n> > +\t\tLOG(RkISP1, Error) << \"Failed to setup links\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Enumerate all sensors connected to the CSI-2 receiver and create one\n> > +\t * camera instance for each of them.\n> > +\t */\n> > +\tconst MediaPad *pad = dphy->getPadByIndex(0);\n> > +\tif (!pad) {\n> > +\t\tret = -EINVAL;\n> > +\t\tgoto done;\n> > +\t}\n> > +\n> > +\tfor (MediaLink *link : pad->links()) {\n> > +\t\t/*\n> > +\t\t * A sensor is expected to have a single pad and expose the\n> > +\t\t * SENSOR function. This is overly restrictive, a better logic\n> > +\t\t * is needed to support sensor that expose multiple subdevs.\n> > +\t\t *\n> > +\t\t * \\todo Create a helper to identify sensors\n> > +\t\t */\n> > +\t\tMediaEntity *sensor = link->source()->entity();\n> > +\t\tif (!sensor)\n> > +\t\t\tcontinue;\n> > +\t\tif (sensor->pads().size() != 1)\n> > +\t\t\tcontinue;\n> > +\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > +\t\t\tcontinue;\n> \n> Some of these checks overlap with CameraSensor::init(). Should all of \n> them be moved there? createCamera() will fail if CameraSensor::init() \n> fails anyhow.\n\nGood point, I'll fix this. I will also remove the comment, as the\nlimitation will then be in the CameraSensor class, not here.\n\n> > +\n> > +\t\tcreateCamera(sensor);\n> > +\t}\n> > +\n> > +done:\n> > +\tmedia_->close();\n> > +\n> > +\treturn ret == 0;\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Buffer Handling\n> > + */\n> > +\n> > +void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> > +{\n> > +\tASSERT(activeCamera_);\n> > +\n> > +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> > +\tRequest *request = data->queuedRequests_.front();\n> > +\n> > +\tcompleteBuffer(activeCamera_, request, buffer);\n> > +\tcompleteRequest(activeCamera_, request);\n> > +}\n> > +\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\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 DD7C360B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 16 Apr 2019 01:10:01 +0200 (CEST)","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 3B872320;\n\tTue, 16 Apr 2019 01:10:01 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555369801;\n\tbh=weMLTg6kjrKPYEY38E3BVNkl0r4fdVbCrd6H60HDuI0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B9yRFbkxKVYqcfN95+40lz48Lp6CYL/ed3eDFoVomJyUAnNA+/ch9v96G0OyLtaal\n\tMG5x89vOGuYGZZF8/yps41LqdkXH/fSe/qqK2pMwrGxYugUARCC8J6OltQ0JBO0foz\n\tHxa79CtEAatIkhQgK/r0JpjPT6txMF+ycXzdDtSM=","Date":"Tue, 16 Apr 2019 02:09:52 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415230952.GN17083@pendragon.ideasonboard.com>","References":"<20190415165700.22970-1-laurent.pinchart@ideasonboard.com>\n\t<20190415165700.22970-12-laurent.pinchart@ideasonboard.com>\n\t<20190415220811.GB1980@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415220811.GB1980@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 11/11] libcamera: pipeline: Add RKISP1\n\tpipeline","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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":"Mon, 15 Apr 2019 23:10:02 -0000"}},{"id":1374,"web_url":"https://patchwork.libcamera.org/comment/1374/","msgid":"<20190415232929.GD1980@bigcity.dyn.berto.se>","date":"2019-04-15T23:29:29","subject":"Re: [libcamera-devel] [PATCH 11/11] libcamera: pipeline: Add RKISP1\n\tpipeline","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2019-04-16 02:09:52 +0300, Laurent Pinchart wrote:\n> > > +int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request \n> > > *request)\n> > > +{\n> > > +\tRkISP1CameraData *data = cameraData(camera);\n> > > +\tStream *stream = &data->stream_;\n> > > +\n> > > +\tBuffer *buffer = request->findBuffer(stream);\n> > > +\tif (!buffer) {\n> > > +\t\tLOG(RkISP1, Error)\n> > > +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> > > +\t\treturn -ENOENT;\n> > > +\t}\n> > \n> > Should we move this check to Camera::queueRequest() ?\n> \n> The Camera class doesn't have access to data->stream_. I expect this to\n> be reworked with multiple streams support anyway, so I'd keep it as-is\n> for now.\n\nThe Camera has access to something better, all streams configured. With \nthe pending patch [1] from Jacopo adding Request::buffers(), something \nlike this in Camera::queueRequest() would address both single and \nmultiple streams.\n\n+       for (auto const &it : request->buffers()) {\n+               Stream *stream = it.first;\n+               if (activeStreams_.find(stream) == activeStreams_.end()) {\n+                       LOG(Camera, Error)\n+                               << \"Attempt to queue request with invalid stream\";\n+                       return -EINVAL;\n+               }\n+       }\n\n1. [PATCH v5 6/7] libcamera: request: Expose the Stream to Buffers map","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7FE1560B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 16 Apr 2019 01:29:31 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id v1so5123303lfg.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 15 Apr 2019 16:29:31 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tq6sm6995414lfj.36.2019.04.15.16.29.29\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 15 Apr 2019 16:29:29 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=fKjLhUK55ZAcmtIt8VQhslGeYka0pBxHQQ+xflAwgoU=;\n\tb=HSyFNag+jldb5cKAf9bDfBG8OvmV7Z08l2pqEwzrBvNccQ8AjZzLbqEDwXcZKtBN7N\n\tpKwcPh7Jf+2C+TUn4gdUdNii9/N7Hpf00aU0h7w9nxtwjfbUd5pPrk9/mAWyGflBxr4E\n\tgIOBJKOey3cEh8bh67EHON7Jnoyzcon6Ec5/NTctvSekUkRusYaixSvZM4soXXKu68Bh\n\txMmJ5o3e0jADoreKejp4N7pCYcmXD56omfUJMx10P+yZvuPp/0jKGbKDGrJtpTk1eD/r\n\tbITuuYD2MQWP1VE85Q5qhk+tmpjul47SSBmZUxJmPIJByhL/1saF/2v1Q4NT54OtL2PF\n\t14iw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=fKjLhUK55ZAcmtIt8VQhslGeYka0pBxHQQ+xflAwgoU=;\n\tb=tSrZcoXKNlfU2avi4zjOZvJIb6+ht/o2n+LwriNW/E2Tp0mbY7LorIiXg8JJQNjYl2\n\t/7/7MZT2r5lUgJqIentPV8d2q6ovlTOJcClazNznWuTf298+aBqZNSpKhHYKDH7mimnX\n\tUf+ID63CCHcMhYwU57dVXrMIPZa4l35OJmTAgNjqI/IzbXJY9nF5C1bzLQJHw6iLF4W9\n\tRq7331pvKddVFnUAONZKlu0knSPjuHE5c+Oz6/R6UGQM8guTRdj71Q1eZgyMWq9thccb\n\tcZFMCD4KgWkQIwmDkrbfggPS79ju4KaAWnxXr9HqpTZHIPezuOrRUmwI4tcyMSNkXxK7\n\t/aWw==","X-Gm-Message-State":"APjAAAXY3AIdDzoyOV2Jn8N91WHZS6zfKSE7l3lqed8LuhHhGUtvgkgi\n\tJkUIDmAOylDhrLH7rbWcH9cSCw==","X-Google-Smtp-Source":"APXvYqzUFHvqdybpmBpHStH3x5VWU4pHoBdDvma/MATbedTOcqaF/tae9g4qciLD5ZQe6fw/czNTpQ==","X-Received":"by 2002:a19:5e56:: with SMTP id\n\tz22mr14321693lfi.81.1555370970676; \n\tMon, 15 Apr 2019 16:29:30 -0700 (PDT)","Date":"Tue, 16 Apr 2019 01:29:29 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415232929.GD1980@bigcity.dyn.berto.se>","References":"<20190415165700.22970-1-laurent.pinchart@ideasonboard.com>\n\t<20190415165700.22970-12-laurent.pinchart@ideasonboard.com>\n\t<20190415220811.GB1980@bigcity.dyn.berto.se>\n\t<20190415230952.GN17083@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415230952.GN17083@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH 11/11] libcamera: pipeline: Add RKISP1\n\tpipeline","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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":"Mon, 15 Apr 2019 23:29:31 -0000"}},{"id":1415,"web_url":"https://patchwork.libcamera.org/comment/1415/","msgid":"<20190416223116.GM4822@pendragon.ideasonboard.com>","date":"2019-04-16T22:31:16","subject":"Re: [libcamera-devel] [PATCH 11/11] libcamera: pipeline: Add RKISP1\n\tpipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Apr 16, 2019 at 01:29:29AM +0200, Niklas Söderlund wrote:\n> On 2019-04-16 02:09:52 +0300, Laurent Pinchart wrote:\n> >>> +int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request \n> >>> *request)\n> >>> +{\n> >>> +\tRkISP1CameraData *data = cameraData(camera);\n> >>> +\tStream *stream = &data->stream_;\n> >>> +\n> >>> +\tBuffer *buffer = request->findBuffer(stream);\n> >>> +\tif (!buffer) {\n> >>> +\t\tLOG(RkISP1, Error)\n> >>> +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> >>> +\t\treturn -ENOENT;\n> >>> +\t}\n> >> \n> >> Should we move this check to Camera::queueRequest() ?\n> > \n> > The Camera class doesn't have access to data->stream_. I expect this to\n> > be reworked with multiple streams support anyway, so I'd keep it as-is\n> > for now.\n> \n> The Camera has access to something better, all streams configured. With \n> the pending patch [1] from Jacopo adding Request::buffers(), something \n> like this in Camera::queueRequest() would address both single and \n> multiple streams.\n> \n> +       for (auto const &it : request->buffers()) {\n> +               Stream *stream = it.first;\n> +               if (activeStreams_.find(stream) == activeStreams_.end()) {\n> +                       LOG(Camera, Error)\n> +                               << \"Attempt to queue request with invalid stream\";\n> +                       return -EINVAL;\n> +               }\n> +       }\n> \n> 1. [PATCH v5 6/7] libcamera: request: Expose the Stream to Buffers map\n\nTo avoid adding a dependency between the two series, let's do this on\ntop when they will be merged. Would you like to send a patch ? :-)","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 05B0A60004\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 17 Apr 2019 00:31:25 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9854EE2;\n\tWed, 17 Apr 2019 00:31:24 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555453884;\n\tbh=ZIk6CinWq6ZxcmsUZvk7xGSOKkTtjMh0c7xj5ChMNu0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MsEmRqWAby8ODAlA0LmEp9ZQ2xusGFYjgWnyGEg7Zd+DnJTeos0rcDLlN/Trw9w/J\n\tWcom80cNYvqCIKZ5XY4mi9ZpKDxdQTDtIwO5NdVCu0kAY6/5lPgfZcVEOCwTLfeJKn\n\thLk/ZmEKu3Aj/uDgkf/XSQoFiWMEY2frdKxINYwk=","Date":"Wed, 17 Apr 2019 01:31:16 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190416223116.GM4822@pendragon.ideasonboard.com>","References":"<20190415165700.22970-1-laurent.pinchart@ideasonboard.com>\n\t<20190415165700.22970-12-laurent.pinchart@ideasonboard.com>\n\t<20190415220811.GB1980@bigcity.dyn.berto.se>\n\t<20190415230952.GN17083@pendragon.ideasonboard.com>\n\t<20190415232929.GD1980@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415232929.GD1980@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 11/11] libcamera: pipeline: Add RKISP1\n\tpipeline","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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":"Tue, 16 Apr 2019 22:31:25 -0000"}}]