[{"id":2344,"web_url":"https://patchwork.libcamera.org/comment/2344/","msgid":"","date":"2019-08-08T15:58:54","subject":"Re: [libcamera-devel] [PATCH 6/6] [PoC/RFC] libcamera: pipeline:\n\tAdd RaspberryPi handler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Just in case anyone actually tries this patch out - I've now updated my\nkernel branch which will require the following fixup to this patch to\nfunction: (or wait for a v2 for any testing)\n\ndiff --git a/src/libcamera/pipeline/raspberrypi.cpp\nb/src/libcamera/pipeline/raspberrypi.cpp\nindex 3d0b4453bfad..12512ebf62db 100644\n--- a/src/libcamera/pipeline/raspberrypi.cpp\n+++ b/src/libcamera/pipeline/raspberrypi.cpp\n@@ -341,7 +341,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator\n*enumerator)\n DeviceMatch codec(\"bcm2835-codec\");\n\n /* We explicitly need the ISP device from the MMAL codec driver. */\n- codec.add(\"bcm2835-codec-isp\");\n+ codec.add(\"bcm2835-codec-isp-source\");\n\n unicam_ = enumerator->search(unicam);\n if (!unicam_)\n@@ -362,9 +362,11 @@ bool PipelineHandlerRPi::match(DeviceEnumerator\n*enumerator)\n return false;\n\n /* Locate the ISP M2M node */\n- MediaEntity *isp = codec_->getEntityByName(\"bcm2835-codec-isp\");\n- if (!isp)\n+ MediaEntity *isp =\ncodec_->getEntityByName(\"bcm2835-codec-isp-source\");\n+ if (!isp) {\n+ LOG(RPI, Error) << \"Could not identify the ISP\";\n return false;\n+ }\n\n data->isp_ = new V4L2M2MDevice(isp->deviceNode());\n if (data->isp_->status()) {\n\n\nOn 08/08/2019 16:12, Kieran Bingham wrote:\n> Utilise the CameraSensor class and construct a pipeline for a single\n> sensor on the Unicam, routed through the V4L2 Codec ISP interface.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/pipeline/meson.build | 1 +\n> src/libcamera/pipeline/raspberrypi.cpp | 425 +++++++++++++++++++++++++\n> 2 files changed, 426 insertions(+)\n> create mode 100644 src/libcamera/pipeline/raspberrypi.cpp\n> \n> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> index 0d466225a72e..7ed7b26f3033 100644\n> --- a/src/libcamera/pipeline/meson.build\n> +++ b/src/libcamera/pipeline/meson.build\n> @@ -1,4 +1,5 @@\n> libcamera_sources += files([\n> + 'raspberrypi.cpp',\n> 'uvcvideo.cpp',\n> 'vimc.cpp',\n> ])\n> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp\n> new file mode 100644\n> index 000000000000..4c4c3dea0113\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi.cpp\n> @@ -0,0 +1,425 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * raspberrypi.cpp - Pipeline handler for raspberrypi devices\n> + */\n> +\n> +#include \n> +#include \n> +#include \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_controls.h\"\n> +#include \"v4l2_videodevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(RPI)\n> +\n> +class RPiCameraData : public CameraData\n> +{\n> +public:\n> +\tRPiCameraData(PipelineHandler *pipe)\n> +\t\t: CameraData(pipe), sensor_(nullptr), unicam_(nullptr),\n> +\t\t isp_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\t~RPiCameraData()\n> +\t{\n> +\t\tbayerBuffers_.destroyBuffers();\n> +\t\tdelete unicam_;\n> +\t\tdelete isp_;\n> +\t}\n> +\n> +\tvoid sensorReady(Buffer *buffer);\n> +\tvoid ispOutputReady(Buffer *buffer);\n> +\tvoid ispCaptureReady(Buffer *buffer);\n> +\n> +\tCameraSensor *sensor_;\n> +\tV4L2VideoDevice *unicam_;\n> +\tV4L2M2MDevice *isp_;\n> +\tStream stream_;\n> +\n> +\tBufferPool bayerBuffers_;\n> +\tstd::vector> rawBuffers_;\n> +};\n> +\n> +class RPiCameraConfiguration : public CameraConfiguration\n> +{\n> +public:\n> +\tRPiCameraConfiguration();\n> +\n> +\tStatus validate() override;\n> +};\n> +\n> +class PipelineHandlerRPi : public PipelineHandler\n> +{\n> +public:\n> +\tPipelineHandlerRPi(CameraManager *manager);\n> +\t~PipelineHandlerRPi();\n> +\n> +\tCameraConfiguration *\n> +\tgenerateConfiguration(Camera *camera,\n> +\t\t\t const StreamRoles &roles) override;\n> +\tint configure(Camera *camera,\n> +\t\t CameraConfiguration *config) override;\n> +\n> +\tint allocateBuffers(Camera *camera,\n> +\t\t\t const std::set &streams) override;\n> +\tint freeBuffers(Camera *camera,\n> +\t\t\tconst std::set &streams) 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> +\tRPiCameraData *cameraData(const Camera *camera)\n> +\t{\n> +\t\treturn static_cast(\n> +\t\t\tPipelineHandler::cameraData(camera));\n> +\t}\n> +\n> +\tstd::shared_ptr unicam_;\n> +\tstd::shared_ptr codec_;\n> +};\n> +\n> +RPiCameraConfiguration::RPiCameraConfiguration()\n> +\t: CameraConfiguration()\n> +{\n> +}\n> +\n> +CameraConfiguration::Status RPiCameraConfiguration::validate()\n> +{\n> +\tStatus status = Valid;\n> +\n> +\tif (config_.empty())\n> +\t\treturn Invalid;\n> +\n> +\t/* Todo: Experiment with increased stream support through the ISP. */\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/* Todo: restrict to hardware capabilities. */\n> +\n> +\tcfg.bufferCount = 4;\n> +\n> +\treturn status;\n> +}\n> +\n> +PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> +\t: PipelineHandler(manager), unicam_(nullptr), codec_(nullptr)\n> +{\n> +}\n> +\n> +PipelineHandlerRPi::~PipelineHandlerRPi()\n> +{\n> +\tif (unicam_)\n> +\t\tunicam_->release();\n> +\n> +\tif (codec_)\n> +\t\tcodec_->release();\n> +}\n> +\n> +CameraConfiguration *\n> +PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> +\t\t\t\t\t const StreamRoles &roles)\n> +{\n> +\tCameraConfiguration *config = new RPiCameraConfiguration();\n> +\n> +\tif (roles.empty())\n> +\t\treturn config;\n> +\n> +\tStreamConfiguration cfg{};\n> +\tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n> +\tcfg.size = { 1920, 1080 }; // data->sensor_->resolution();\n> +\tcfg.bufferCount = 4;\n> +\n> +\tLOG(RPI, Debug) << \"Config default as \" << cfg.toString();\n> +\n> +\tconfig->addConfiguration(cfg);\n> +\n> +\tconfig->validate();\n> +\n> +\treturn config;\n> +}\n> +\n> +int PipelineHandlerRPi::configure(Camera *camera,\n> +\t\t\t\t CameraConfiguration *config)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tStreamConfiguration &cfg = config->at(0);\n> +\tint ret;\n> +\n> +\tSize sensorSize = { 1920, 1080 };\n> +\tSize outputSize = { 1920, 1088 };\n> +\n> +\tV4L2DeviceFormat format = {};\n> +\tformat.size = sensorSize;\n> +\tformat.fourcc = V4L2_PIX_FMT_SRGGB10P;\n> +\n> +\tLOG(RPI, Debug) << \"Setting format to \" << format.toString();\n> +\n> +\tret = data->unicam_->setFormat(&format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (format.size != sensorSize ||\n> +\t format.fourcc != V4L2_PIX_FMT_SRGGB10P) {\n> +\t\tLOG(RPI, Error)\n> +\t\t\t<< \"Failed to set format on Video device: \"\n> +\t\t\t<< format.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tformat.size = outputSize;\n> +\n> +\tret = data->isp_->output()->setFormat(&format);\n> +\n> +\tif (format.size != outputSize ||\n> +\t format.fourcc != V4L2_PIX_FMT_SRGGB10P) {\n> +\t\tLOG(RPI, Error) << \"Failed to set format on ISP output device: \"\n> +\t\t\t\t<< format.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* Configure the ISP based to generate the requested size and format. */\n> +\tformat.size = cfg.size;\n> +\tformat.fourcc = cfg.pixelFormat;\n> +\n> +\tret = data->isp_->capture()->setFormat(&format);\n> +\n> +\tif (format.size != cfg.size ||\n> +\t format.fourcc != cfg.pixelFormat) {\n> +\t\tLOG(RPI, Error)\n> +\t\t\t<< \"Failed to set format on ISP capture device: \"\n> +\t\t\t<< format.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcfg.setStream(&data->stream_);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerRPi::allocateBuffers(Camera *camera,\n> +\t\t\t\t\tconst std::set &streams)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tStream *stream = *streams.begin();\n> +\tconst StreamConfiguration &cfg = stream->configuration();\n> +\tint ret;\n> +\n> +\tLOG(RPI, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n> +\n> +\t/*\n> +\t * Buffers are allocated on the camera, and the capture pad of the ISP\n> +\t * unicam -> isp.output -> isp.capture -> Application\n> +\t */\n> +\n> +\t/* Create a new intermediate buffer pool */\n> +\tdata->bayerBuffers_.createBuffers(cfg.bufferCount);\n> +\n> +\t/* Tie the unicam video buffers to the intermediate pool */\n> +\tret = data->unicam_->exportBuffers(&data->bayerBuffers_);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = data->isp_->output()->importBuffers(&data->bayerBuffers_);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* Tie the stream buffers to the capture device of the ISP */\n> +\tif (stream->memoryType() == InternalMemory)\n> +\t\tret = data->isp_->capture()->exportBuffers(&stream->bufferPool());\n> +\telse\n> +\t\tret = data->isp_->capture()->importBuffers(&stream->bufferPool());\n> +\n> +\treturn ret;\n> +}\n> +\n> +int PipelineHandlerRPi::freeBuffers(Camera *camera,\n> +\t\t\t\t const std::set &streams)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tint ret;\n> +\n> +\tret = data->unicam_->releaseBuffers();\n> +\tret = data->isp_->output()->releaseBuffers();\n> +\tret = data->isp_->capture()->releaseBuffers();\n> +\n> +\tdata->bayerBuffers_.destroyBuffers();\n> +\n> +\treturn ret;\n> +}\n> +\n> +int PipelineHandlerRPi::start(Camera *camera)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tint ret;\n> +\n> +\tdata->rawBuffers_ = data->unicam_->queueAllBuffers();\n> +\tif (data->rawBuffers_.empty()) {\n> +\t\tLOG(RPI, Debug) << \"Failed to queue unicam buffers\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tLOG(RPI, Warning) << \"Using hard-coded exposure/gain defaults\";\n> +\n> +\tV4L2ControlList controls;\n> +\tcontrols.add(V4L2_CID_EXPOSURE, 1700);\n> +\tcontrols.add(V4L2_CID_ANALOGUE_GAIN, 180);\n> +\tret = data->unicam_->setControls(&controls);\n> +\tif (ret) {\n> +\t\tLOG(RPI, Error) << \"Failed to set controls\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = data->isp_->output()->streamOn();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = data->isp_->capture()->streamOn();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = data->unicam_->streamOn();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn ret;\n> +}\n> +\n> +void PipelineHandlerRPi::stop(Camera *camera)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\n> +\tdata->isp_->capture()->streamOff();\n> +\tdata->isp_->output()->streamOff();\n> +\tdata->unicam_->streamOff();\n> +\n> +\tdata->rawBuffers_.clear();\n> +}\n> +\n> +int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tBuffer *buffer = request->findBuffer(&data->stream_);\n> +\tif (!buffer) {\n> +\t\tLOG(RPI, Error)\n> +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> +\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\tint ret = data->isp_->capture()->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tPipelineHandler::queueRequest(camera, request);\n> +\n> +\treturn 0;\n> +}\n> +\n> +bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> +{\n> +\tDeviceMatch unicam(\"unicam\");\n> +\tDeviceMatch codec(\"bcm2835-codec\");\n> +\n> +\t/* We explicitly need the ISP device from the MMAL codec driver. */\n> +\tcodec.add(\"bcm2835-codec-isp\");\n> +\n> +\tunicam_ = enumerator->search(unicam);\n> +\tif (!unicam_)\n> +\t\treturn false;\n> +\n> +\tcodec_ = enumerator->search(codec);\n> +\tif (!codec_)\n> +\t\treturn false;\n> +\n> +\tunicam_->acquire();\n> +\tcodec_->acquire();\n> +\n> +\tstd::unique_ptr data = utils::make_unique(this);\n> +\n> +\t/* Locate and open the unicam video node. */\n> +\tdata->unicam_ = new V4L2VideoDevice(unicam_->getEntityByName(\"unicam\"));\n> +\tif (data->unicam_->open())\n> +\t\treturn false;\n> +\n> +\t/* Locate the ISP M2M node */\n> +\tMediaEntity *isp = codec_->getEntityByName(\"bcm2835-codec-isp\");\n> +\tif (!isp)\n> +\t\treturn false;\n> +\n> +\tdata->isp_ = new V4L2M2MDevice(isp->deviceNode());\n> +\tif (data->isp_->status()) {\n> +\t\tLOG(RPI, Error) << \"Could not create the ISP device\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tdata->unicam_->bufferReady.connect(data.get(), &RPiCameraData::sensorReady);\n> +\tdata->isp_->output()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputReady);\n> +\tdata->isp_->capture()->bufferReady.connect(data.get(), &RPiCameraData::ispCaptureReady);\n> +\n> +\t/* Identify the sensor */\n> +\tfor (MediaEntity *entity : unicam_->entities()) {\n> +\t\tif (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> +\t\t\tdata->sensor_ = new CameraSensor(entity);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!data->sensor_)\n> +\t\treturn false;\n> +\n> +\tint ret = data->sensor_->init();\n> +\tif (ret)\n> +\t\treturn false;\n> +\n> +\t/* Create and register the camera. */\n> +\tstd::set streams{ &data->stream_ };\n> +\tstd::shared_ptr camera =\n> +\t\tCamera::create(this, data->sensor_->entity()->name(), streams);\n> +\tregisterCamera(std::move(camera), std::move(data));\n> +\n> +\treturn true;\n> +}\n> +\n> +void RPiCameraData::sensorReady(Buffer *buffer)\n> +{\n> +\t/* Deliver the frame from the sensor to the ISP */\n> +\tisp_->output()->queueBuffer(buffer);\n> +}\n> +\n> +void RPiCameraData::ispOutputReady(Buffer *buffer)\n> +{\n> +\t/* Return a completed buffer from the ISP back to the sensor */\n> +\tunicam_->queueBuffer(buffer);\n> +}\n> +\n> +void RPiCameraData::ispCaptureReady(Buffer *buffer)\n> +{\n> +\tRequest *request = buffer->request();\n> +\n> +\tpipe_->completeBuffer(camera_, request, buffer);\n> +\tpipe_->completeRequest(camera_, request);\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi);\n> +\n> +} /* namespace libcamera */\n>","headers":{"Return-Path":"","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 10FF0615FF\n\tfor ;\n\tThu, 8 Aug 2019 17:58:58 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 64B9CCC;\n\tThu, 8 Aug 2019 17:58:57 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565279937;\n\tbh=BVaIWCFZ4m+klwuhbV8b8VlS5xfDITJeHYGeP+mRyoI=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=lE/5xJX1XKfdjSkzf2/tpDcgAhIhxu/iUpO7D2WLzetqAifgngWjNlJoFjjjjDGx8\n\tH1j4fHDjl0ZCjWuBIPL7zTeZOcn6M404H+cr8aRzrZ3wXFWrxcbeXEdeiNUAuJeC9I\n\tx1HDhxSWliHXlgF2FGhp36KSIikMaK8DnD6XOGgw=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kieran Bingham ,\n\tLibCamera Devel ","References":"<20190808151221.24254-1-kieran.bingham@ideasonboard.com>\n\t<20190808151221.24254-7-kieran.bingham@ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 8 Aug 2019 16:58:54 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190808151221.24254-7-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 6/6] [PoC/RFC] libcamera: pipeline:\n\tAdd RaspberryPi handler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 08 Aug 2019 15:58:58 -0000"}},{"id":2351,"web_url":"https://patchwork.libcamera.org/comment/2351/","msgid":"<20190808215114.GH6055@pendragon.ideasonboard.com>","date":"2019-08-08T21:51:14","subject":"Re: [libcamera-devel] [PATCH 6/6] [PoC/RFC] libcamera: pipeline:\n\tAdd RaspberryPi handler","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Aug 08, 2019 at 04:12:21PM +0100, Kieran Bingham wrote:\n> Utilise the CameraSensor class and construct a pipeline for a single\n> sensor on the Unicam, routed through the V4L2 Codec ISP interface.\n> \n> Signed-off-by: Kieran Bingham \n\nA pipeline handler for the RPi, very nice :-)\n\n> ---\n> src/libcamera/pipeline/meson.build | 1 +\n> src/libcamera/pipeline/raspberrypi.cpp | 425 +++++++++++++++++++++++++\n> 2 files changed, 426 insertions(+)\n> create mode 100644 src/libcamera/pipeline/raspberrypi.cpp\n> \n> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> index 0d466225a72e..7ed7b26f3033 100644\n> --- a/src/libcamera/pipeline/meson.build\n> +++ b/src/libcamera/pipeline/meson.build\n> @@ -1,4 +1,5 @@\n> libcamera_sources += files([\n> + 'raspberrypi.cpp',\n\nI would expect the same level of complexity as the IPU3 and RkISP1\npipeline handlers, should this get its own directory ?\n\n> 'uvcvideo.cpp',\n> 'vimc.cpp',\n> ])\n> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp\n> new file mode 100644\n> index 000000000000..4c4c3dea0113\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi.cpp\n> @@ -0,0 +1,425 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * raspberrypi.cpp - Pipeline handler for raspberrypi devices\n\ns/raspberrypi devices/Raspberry Pi devices/ ?\n\nShould we name it Raspberry Pi, or based on the SoC ? It could be used\non other systems using the same SoC (or family of SoCs).\n\n> + */\n> +\n> +#include \n> +#include \n> +#include \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_controls.h\"\n> +#include \"v4l2_videodevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(RPI)\n> +\n> +class RPiCameraData : public CameraData\n> +{\n> +public:\n> +\tRPiCameraData(PipelineHandler *pipe)\n> +\t\t: CameraData(pipe), sensor_(nullptr), unicam_(nullptr),\n> +\t\t isp_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\t~RPiCameraData()\n> +\t{\n> +\t\tbayerBuffers_.destroyBuffers();\n\nShouldn't you also delete sensor_ ?\n\n> +\t\tdelete unicam_;\n> +\t\tdelete isp_;\n> +\t}\n> +\n> +\tvoid sensorReady(Buffer *buffer);\n> +\tvoid ispOutputReady(Buffer *buffer);\n> +\tvoid ispCaptureReady(Buffer *buffer);\n> +\n> +\tCameraSensor *sensor_;\n> +\tV4L2VideoDevice *unicam_;\n> +\tV4L2M2MDevice *isp_;\n> +\tStream stream_;\n> +\n> +\tBufferPool bayerBuffers_;\n> +\tstd::vector> rawBuffers_;\n> +};\n> +\n> +class RPiCameraConfiguration : public CameraConfiguration\n> +{\n> +public:\n> +\tRPiCameraConfiguration();\n> +\n> +\tStatus validate() override;\n> +};\n> +\n> +class PipelineHandlerRPi : public PipelineHandler\n> +{\n> +public:\n> +\tPipelineHandlerRPi(CameraManager *manager);\n> +\t~PipelineHandlerRPi();\n> +\n> +\tCameraConfiguration *\n> +\tgenerateConfiguration(Camera *camera,\n> +\t\t\t const StreamRoles &roles) override;\n> +\tint configure(Camera *camera,\n> +\t\t CameraConfiguration *config) override;\n> +\n> +\tint allocateBuffers(Camera *camera,\n> +\t\t\t const std::set &streams) override;\n> +\tint freeBuffers(Camera *camera,\n> +\t\t\tconst std::set &streams) 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> +\tRPiCameraData *cameraData(const Camera *camera)\n> +\t{\n> +\t\treturn static_cast(\n> +\t\t\tPipelineHandler::cameraData(camera));\n> +\t}\n> +\n> +\tstd::shared_ptr unicam_;\n> +\tstd::shared_ptr codec_;\n> +};\n> +\n> +RPiCameraConfiguration::RPiCameraConfiguration()\n> +\t: CameraConfiguration()\n> +{\n> +}\n> +\n> +CameraConfiguration::Status RPiCameraConfiguration::validate()\n> +{\n> +\tStatus status = Valid;\n> +\n> +\tif (config_.empty())\n> +\t\treturn Invalid;\n> +\n> +\t/* Todo: Experiment with increased stream support through the ISP. */\n\ns/Todo:/\\todo/\n\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/* Todo: restrict to hardware capabilities. */\n\nI think this one should be addressed before merging the code.\n\n> +\n> +\tcfg.bufferCount = 4;\n> +\n> +\treturn status;\n> +}\n> +\n> +PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> +\t: PipelineHandler(manager), unicam_(nullptr), codec_(nullptr)\n> +{\n> +}\n> +\n> +PipelineHandlerRPi::~PipelineHandlerRPi()\n> +{\n> +\tif (unicam_)\n> +\t\tunicam_->release();\n> +\n> +\tif (codec_)\n> +\t\tcodec_->release();\n> +}\n> +\n> +CameraConfiguration *\n> +PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> +\t\t\t\t\t const StreamRoles &roles)\n> +{\n> +\tCameraConfiguration *config = new RPiCameraConfiguration();\n> +\n> +\tif (roles.empty())\n> +\t\treturn config;\n> +\n> +\tStreamConfiguration cfg{};\n> +\tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n> +\tcfg.size = { 1920, 1080 }; // data->sensor_->resolution();\n\nLet's remove commented-out code.\n\nWhat prevents from using the sensor resolution, and how is 1080p\nselected as the default resolution ?\n\n> +\tcfg.bufferCount = 4;\n> +\n> +\tLOG(RPI, Debug) << \"Config default as \" << cfg.toString();\n\nI think the configuration is printed in the caller already.\n\n> +\n> +\tconfig->addConfiguration(cfg);\n> +\n> +\tconfig->validate();\n> +\n> +\treturn config;\n> +}\n> +\n> +int PipelineHandlerRPi::configure(Camera *camera,\n> +\t\t\t\t CameraConfiguration *config)\n\nThis holds on a single line.\n\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tStreamConfiguration &cfg = config->at(0);\n> +\tint ret;\n> +\n> +\tSize sensorSize = { 1920, 1080 };\n> +\tSize outputSize = { 1920, 1088 };\n> +\n> +\tV4L2DeviceFormat format = {};\n> +\tformat.size = sensorSize;\n> +\tformat.fourcc = V4L2_PIX_FMT_SRGGB10P;\n\nThis all seems to lack genericity :-) I think that at least he format\nshould support different Bayer patterns and bpp values.\n\nDon't you need to set the format on the sensor subdev ?\n\n> +\n> +\tLOG(RPI, Debug) << \"Setting format to \" << format.toString();\n> +\n> +\tret = data->unicam_->setFormat(&format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (format.size != sensorSize ||\n> +\t format.fourcc != V4L2_PIX_FMT_SRGGB10P) {\n> +\t\tLOG(RPI, Error)\n> +\t\t\t<< \"Failed to set format on Video device: \"\n> +\t\t\t<< format.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tformat.size = outputSize;\n> +\n> +\tret = data->isp_->output()->setFormat(&format);\n> +\n> +\tif (format.size != outputSize ||\n> +\t format.fourcc != V4L2_PIX_FMT_SRGGB10P) {\n> +\t\tLOG(RPI, Error) << \"Failed to set format on ISP output device: \"\n> +\t\t\t\t<< format.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* Configure the ISP based to generate the requested size and format. */\n\nAre you missing something after \"based\" ?\n\n> +\tformat.size = cfg.size;\n> +\tformat.fourcc = cfg.pixelFormat;\n> +\n> +\tret = data->isp_->capture()->setFormat(&format);\n> +\n> +\tif (format.size != cfg.size ||\n> +\t format.fourcc != cfg.pixelFormat) {\n> +\t\tLOG(RPI, Error)\n> +\t\t\t<< \"Failed to set format on ISP capture device: \"\n> +\t\t\t<< format.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcfg.setStream(&data->stream_);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerRPi::allocateBuffers(Camera *camera,\n> +\t\t\t\t\tconst std::set &streams)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tStream *stream = *streams.begin();\n> +\tconst StreamConfiguration &cfg = stream->configuration();\n> +\tint ret;\n> +\n> +\tLOG(RPI, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n\nI'd drop this message, or move it to Camera::allocateBuffers().\n\n> +\n> +\t/*\n> +\t * Buffers are allocated on the camera, and the capture pad of the ISP\n> +\t * unicam -> isp.output -> isp.capture -> Application\n> +\t */\n> +\n> +\t/* Create a new intermediate buffer pool */\n\ns/pool/pool./ (same comment for all other comments in this file that\ndon't end with a period)\n\n> +\tdata->bayerBuffers_.createBuffers(cfg.bufferCount);\n> +\n> +\t/* Tie the unicam video buffers to the intermediate pool */\n> +\tret = data->unicam_->exportBuffers(&data->bayerBuffers_);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = data->isp_->output()->importBuffers(&data->bayerBuffers_);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* Tie the stream buffers to the capture device of the ISP */\n> +\tif (stream->memoryType() == InternalMemory)\n> +\t\tret = data->isp_->capture()->exportBuffers(&stream->bufferPool());\n> +\telse\n> +\t\tret = data->isp_->capture()->importBuffers(&stream->bufferPool());\n> +\n> +\treturn ret;\n> +}\n> +\n> +int PipelineHandlerRPi::freeBuffers(Camera *camera,\n> +\t\t\t\t const std::set &streams)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tint ret;\n> +\n> +\tret = data->unicam_->releaseBuffers();\n> +\tret = data->isp_->output()->releaseBuffers();\n> +\tret = data->isp_->capture()->releaseBuffers();\n\nYou're losing the first two error values.\n\n> +\n> +\tdata->bayerBuffers_.destroyBuffers();\n> +\n> +\treturn ret;\n> +}\n> +\n> +int PipelineHandlerRPi::start(Camera *camera)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tint ret;\n> +\n> +\tdata->rawBuffers_ = data->unicam_->queueAllBuffers();\n> +\tif (data->rawBuffers_.empty()) {\n> +\t\tLOG(RPI, Debug) << \"Failed to queue unicam buffers\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tLOG(RPI, Warning) << \"Using hard-coded exposure/gain defaults\";\n> +\n> +\tV4L2ControlList controls;\n> +\tcontrols.add(V4L2_CID_EXPOSURE, 1700);\n> +\tcontrols.add(V4L2_CID_ANALOGUE_GAIN, 180);\n> +\tret = data->unicam_->setControls(&controls);\n> +\tif (ret) {\n> +\t\tLOG(RPI, Error) << \"Failed to set controls\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = data->isp_->output()->streamOn();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = data->isp_->capture()->streamOn();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = data->unicam_->streamOn();\n> +\tif (ret)\n> +\t\treturn ret;\n\nShouldn't you stop streaming on the successfully started streams when an\nerror happens ?\n\n> +\n> +\treturn ret;\n\n\treturn 0;\n\n> +}\n> +\n> +void PipelineHandlerRPi::stop(Camera *camera)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\n> +\tdata->isp_->capture()->streamOff();\n> +\tdata->isp_->output()->streamOff();\n> +\tdata->unicam_->streamOff();\n> +\n> +\tdata->rawBuffers_.clear();\n> +}\n> +\n> +int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tBuffer *buffer = request->findBuffer(&data->stream_);\n> +\tif (!buffer) {\n> +\t\tLOG(RPI, Error)\n> +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> +\n> +\t\treturn -ENOENT;\n\nCan this happen ?\n\n> +\t}\n> +\n> +\tint ret = data->isp_->capture()->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tPipelineHandler::queueRequest(camera, request);\n> +\n> +\treturn 0;\n> +}\n> +\n> +bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> +{\n> +\tDeviceMatch unicam(\"unicam\");\n> +\tDeviceMatch codec(\"bcm2835-codec\");\n> +\n> +\t/* We explicitly need the ISP device from the MMAL codec driver. */\n> +\tcodec.add(\"bcm2835-codec-isp\");\n\nIs there any subdev in the unicam device that we could match on ?\n\n> +\n> +\tunicam_ = enumerator->search(unicam);\n> +\tif (!unicam_)\n> +\t\treturn false;\n> +\n> +\tcodec_ = enumerator->search(codec);\n> +\tif (!codec_)\n> +\t\treturn false;\n> +\n> +\tunicam_->acquire();\n> +\tcodec_->acquire();\n> +\n> +\tstd::unique_ptr data = utils::make_unique(this);\n> +\n> +\t/* Locate and open the unicam video node. */\n> +\tdata->unicam_ = new V4L2VideoDevice(unicam_->getEntityByName(\"unicam\"));\n> +\tif (data->unicam_->open())\n> +\t\treturn false;\n> +\n> +\t/* Locate the ISP M2M node */\n> +\tMediaEntity *isp = codec_->getEntityByName(\"bcm2835-codec-isp\");\n> +\tif (!isp)\n> +\t\treturn false;\n> +\n> +\tdata->isp_ = new V4L2M2MDevice(isp->deviceNode());\n> +\tif (data->isp_->status()) {\n> +\t\tLOG(RPI, Error) << \"Could not create the ISP device\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tdata->unicam_->bufferReady.connect(data.get(), &RPiCameraData::sensorReady);\n> +\tdata->isp_->output()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputReady);\n> +\tdata->isp_->capture()->bufferReady.connect(data.get(), &RPiCameraData::ispCaptureReady);\n> +\n> +\t/* Identify the sensor */\n> +\tfor (MediaEntity *entity : unicam_->entities()) {\n> +\t\tif (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> +\t\t\tdata->sensor_ = new CameraSensor(entity);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!data->sensor_)\n> +\t\treturn false;\n> +\n> +\tint ret = data->sensor_->init();\n> +\tif (ret)\n> +\t\treturn false;\n\nNo need for an intermediate variable, you can write\n\n\tif (data->sensor_->init())\n\t\treturn false;\n\n> +\n> +\t/* Create and register the camera. */\n> +\tstd::set streams{ &data->stream_ };\n> +\tstd::shared_ptr camera =\n> +\t\tCamera::create(this, data->sensor_->entity()->name(), streams);\n> +\tregisterCamera(std::move(camera), std::move(data));\n> +\n> +\treturn true;\n> +}\n> +\n> +void RPiCameraData::sensorReady(Buffer *buffer)\n> +{\n> +\t/* Deliver the frame from the sensor to the ISP */\n\nYou should skip this when buffer->status() == Buffer::BufferCancelled\n(see the IPU3 pipeline handler).\n\n> +\tisp_->output()->queueBuffer(buffer);\n> +}\n> +\n> +void RPiCameraData::ispOutputReady(Buffer *buffer)\n> +{\n> +\t/* Return a completed buffer from the ISP back to the sensor */\n\nSame comment here.\n\n> +\tunicam_->queueBuffer(buffer);\n> +}\n> +\n> +void RPiCameraData::ispCaptureReady(Buffer *buffer)\n> +{\n> +\tRequest *request = buffer->request();\n> +\n> +\tpipe_->completeBuffer(camera_, request, buffer);\n> +\tpipe_->completeRequest(camera_, request);\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi);\n> +\n> +} /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 143686161B\n\tfor ;\n\tThu, 8 Aug 2019 23:51:18 +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 73D6DCC;\n\tThu, 8 Aug 2019 23:51:17 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565301077;\n\tbh=lMztNXxSE3FgUDrTcGEomKVcGw1apV9tuhR2n2IR7Bk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WBX1aBvZRLuyERyBFW9reoMoCfhhHUOM9e6X1SECS8hkhBbiUUUbyJ57XCOBZt1Fz\n\t6sBqCrq1rqC0/nsVNW9WF5RLj+MAJtqhvZ3DFbYB2fG4K1fRGsf589RTIMFjSei1IU\n\tLYmxnsoU5Fj4HvnCwNaFml9P/3PtUC6xVMwN/Ly8=","Date":"Fri, 9 Aug 2019 00:51:14 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190808215114.GH6055@pendragon.ideasonboard.com>","References":"<20190808151221.24254-1-kieran.bingham@ideasonboard.com>\n\t<20190808151221.24254-7-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190808151221.24254-7-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 6/6] [PoC/RFC] libcamera: pipeline:\n\tAdd RaspberryPi handler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 08 Aug 2019 21:51:18 -0000"}},{"id":2359,"web_url":"https://patchwork.libcamera.org/comment/2359/","msgid":"<304fd2c6-3929-0363-db78-2f2a06b6d5b5@ideasonboard.com>","date":"2019-08-09T11:19:17","subject":"Re: [libcamera-devel] [PATCH 6/6] [PoC/RFC] libcamera: pipeline:\n\tAdd RaspberryPi handler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 08/08/2019 22:51, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Aug 08, 2019 at 04:12:21PM +0100, Kieran Bingham wrote:\n>> Utilise the CameraSensor class and construct a pipeline for a single\n>> sensor on the Unicam, routed through the V4L2 Codec ISP interface.\n>>\n>> Signed-off-by: Kieran Bingham \n> \n> A pipeline handler for the RPi, very nice :-)\n\nA \"Proof of Concept\" pipeline handler :-) - It works - but needs more\ndevelopment and requires out of tree drivers for the kernel, and\nout-of-driver patches for the out of tree drivers :S\n\nWe'll get there :-D\n\nThanks for the review comments. I've pretty much taken most of them in.\n\n>> ---\n>> src/libcamera/pipeline/meson.build | 1 +\n>> src/libcamera/pipeline/raspberrypi.cpp | 425 +++++++++++++++++++++++++\n>> 2 files changed, 426 insertions(+)\n>> create mode 100644 src/libcamera/pipeline/raspberrypi.cpp\n>>\n>> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n>> index 0d466225a72e..7ed7b26f3033 100644\n>> --- a/src/libcamera/pipeline/meson.build\n>> +++ b/src/libcamera/pipeline/meson.build\n>> @@ -1,4 +1,5 @@\n>> libcamera_sources += files([\n>> + 'raspberrypi.cpp',\n> \n> I would expect the same level of complexity as the IPU3 and RkISP1\n> pipeline handlers, should this get its own directory ?\n\nAs yet, I've seen only a single file for the IPU3, and RKISP1.\n\nWhat other files are expected? The IPA code will go in a separate\ndirectory right? So I'm not yet sure what will be broken out in the\npipeline handlers to need their own directory.\n\n(Except the IPU3 - That looks like it could be split to have an\nipu3.cpp, imgu.cpp, cio2.cpp.)\n\n\n\n> \n>> 'uvcvideo.cpp',\n>> 'vimc.cpp',\n>> ])\n>> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp\n>> new file mode 100644\n>> index 000000000000..4c4c3dea0113\n>> --- /dev/null\n>> +++ b/src/libcamera/pipeline/raspberrypi.cpp\n>> @@ -0,0 +1,425 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * raspberrypi.cpp - Pipeline handler for raspberrypi devices\n> \n> s/raspberrypi devices/Raspberry Pi devices/ ?\n> \n> Should we name it Raspberry Pi, or based on the SoC ? It could be used\n> on other systems using the same SoC (or family of SoCs).\n\nI expect this pipeline handler to function on all of the Raspberry Pi's\nwith a camera interface. This covers multiple SoCs.\n\nSo lets stick with RaspberryPi as the name I think.\n\n\n>> + */\n>> +\n>> +#include \n>> +#include \n>> +#include \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_controls.h\"\n>> +#include \"v4l2_videodevice.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(RPI)\n>> +\n>> +class RPiCameraData : public CameraData\n>> +{\n>> +public:\n>> +\tRPiCameraData(PipelineHandler *pipe)\n>> +\t\t: CameraData(pipe), sensor_(nullptr), unicam_(nullptr),\n>> +\t\t isp_(nullptr)\n>> +\t{\n>> +\t}\n>> +\n>> +\t~RPiCameraData()\n>> +\t{\n>> +\t\tbayerBuffers_.destroyBuffers();\n> \n> Shouldn't you also delete sensor_ ?\n\nYup.\n\n\n> \n>> +\t\tdelete unicam_;\n>> +\t\tdelete isp_;\n>> +\t}\n>> +\n>> +\tvoid sensorReady(Buffer *buffer);\n>> +\tvoid ispOutputReady(Buffer *buffer);\n>> +\tvoid ispCaptureReady(Buffer *buffer);\n>> +\n>> +\tCameraSensor *sensor_;\n>> +\tV4L2VideoDevice *unicam_;\n>> +\tV4L2M2MDevice *isp_;\n>> +\tStream stream_;\n>> +\n>> +\tBufferPool bayerBuffers_;\n>> +\tstd::vector> rawBuffers_;\n>> +};\n>> +\n>> +class RPiCameraConfiguration : public CameraConfiguration\n>> +{\n>> +public:\n>> +\tRPiCameraConfiguration();\n>> +\n>> +\tStatus validate() override;\n>> +};\n>> +\n>> +class PipelineHandlerRPi : public PipelineHandler\n>> +{\n>> +public:\n>> +\tPipelineHandlerRPi(CameraManager *manager);\n>> +\t~PipelineHandlerRPi();\n>> +\n>> +\tCameraConfiguration *\n>> +\tgenerateConfiguration(Camera *camera,\n>> +\t\t\t const StreamRoles &roles) override;\n>> +\tint configure(Camera *camera,\n>> +\t\t CameraConfiguration *config) override;\n>> +\n>> +\tint allocateBuffers(Camera *camera,\n>> +\t\t\t const std::set &streams) override;\n>> +\tint freeBuffers(Camera *camera,\n>> +\t\t\tconst std::set &streams) 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>> +\tRPiCameraData *cameraData(const Camera *camera)\n>> +\t{\n>> +\t\treturn static_cast(\n>> +\t\t\tPipelineHandler::cameraData(camera));\n>> +\t}\n>> +\n>> +\tstd::shared_ptr unicam_;\n>> +\tstd::shared_ptr codec_;\n>> +};\n>> +\n>> +RPiCameraConfiguration::RPiCameraConfiguration()\n>> +\t: CameraConfiguration()\n>> +{\n>> +}\n>> +\n>> +CameraConfiguration::Status RPiCameraConfiguration::validate()\n>> +{\n>> +\tStatus status = Valid;\n>> +\n>> +\tif (config_.empty())\n>> +\t\treturn Invalid;\n>> +\n>> +\t/* Todo: Experiment with increased stream support through the ISP. */\n> \n> s/Todo:/\\todo/\n> \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/* Todo: restrict to hardware capabilities. */\n> \n> I think this one should be addressed before merging the code.\n\n\nYes, needs more development.\n\n\n\n> \n>> +\n>> +\tcfg.bufferCount = 4;\n>> +\n>> +\treturn status;\n>> +}\n>> +\n>> +PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n>> +\t: PipelineHandler(manager), unicam_(nullptr), codec_(nullptr)\n>> +{\n>> +}\n>> +\n>> +PipelineHandlerRPi::~PipelineHandlerRPi()\n>> +{\n>> +\tif (unicam_)\n>> +\t\tunicam_->release();\n>> +\n>> +\tif (codec_)\n>> +\t\tcodec_->release();\n>> +}\n>> +\n>> +CameraConfiguration *\n>> +PipelineHandlerRPi::generateConfiguration(Camera *camera,\n>> +\t\t\t\t\t const StreamRoles &roles)\n>> +{\n>> +\tCameraConfiguration *config = new RPiCameraConfiguration();\n>> +\n>> +\tif (roles.empty())\n>> +\t\treturn config;\n>> +\n>> +\tStreamConfiguration cfg{};\n>> +\tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n>> +\tcfg.size = { 1920, 1080 }; // data->sensor_->resolution();\n> \n> Let's remove commented-out code.\n> \n> What prevents from using the sensor resolution, and how is 1080p\n> selected as the default resolution ?\n\nThe RaspberryPi v2 camera defaults as a 3280x2464 output.\n\nThe ISP has currently got an (incorrect) artificial limitation of 1080p.\n\nWe can work through this when we get updated ISP support.\n\n\n>> +\tcfg.bufferCount = 4;\n>> +\n>> +\tLOG(RPI, Debug) << \"Config default as \" << cfg.toString();\n> \n> I think the configuration is printed in the caller already.\n> \n\nRemoved.\n\n>> +\n>> +\tconfig->addConfiguration(cfg);\n>> +\n>> +\tconfig->validate();\n>> +\n>> +\treturn config;\n>> +}\n>> +\n>> +int PipelineHandlerRPi::configure(Camera *camera,\n>> +\t\t\t\t CameraConfiguration *config)\n> \n> This holds on a single line.\n> \n>> +{\n>> +\tRPiCameraData *data = cameraData(camera);\n>> +\tStreamConfiguration &cfg = config->at(0);\n>> +\tint ret;\n>> +\n>> +\tSize sensorSize = { 1920, 1080 };\n>> +\tSize outputSize = { 1920, 1088 };\n>> +\n>> +\tV4L2DeviceFormat format = {};\n>> +\tformat.size = sensorSize;\n>> +\tformat.fourcc = V4L2_PIX_FMT_SRGGB10P;\n> \n> This all seems to lack genericity :-) I think that at least he format\n> should support different Bayer patterns and bpp values.\n> \n> Don't you need to set the format on the sensor subdev ?\n> \n>> +\n>> +\tLOG(RPI, Debug) << \"Setting format to \" << format.toString();\n>> +\n>> +\tret = data->unicam_->setFormat(&format);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tif (format.size != sensorSize ||\n>> +\t format.fourcc != V4L2_PIX_FMT_SRGGB10P) {\n>> +\t\tLOG(RPI, Error)\n>> +\t\t\t<< \"Failed to set format on Video device: \"\n>> +\t\t\t<< format.toString();\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tformat.size = outputSize;\n>> +\n>> +\tret = data->isp_->output()->setFormat(&format);\n>> +\n>> +\tif (format.size != outputSize ||\n>> +\t format.fourcc != V4L2_PIX_FMT_SRGGB10P) {\n>> +\t\tLOG(RPI, Error) << \"Failed to set format on ISP output device: \"\n>> +\t\t\t\t<< format.toString();\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\t/* Configure the ISP based to generate the requested size and format. */\n> \n> Are you missing something after \"based\" ?\n\ns/based//\n\nOr it could simply be \"Configure the ISP\" ... but we have limited\nconfiguration so far...\n\n\n> \n>> +\tformat.size = cfg.size;\n>> +\tformat.fourcc = cfg.pixelFormat;\n>> +\n>> +\tret = data->isp_->capture()->setFormat(&format);\n>> +\n>> +\tif (format.size != cfg.size ||\n>> +\t format.fourcc != cfg.pixelFormat) {\n>> +\t\tLOG(RPI, Error)\n>> +\t\t\t<< \"Failed to set format on ISP capture device: \"\n>> +\t\t\t<< format.toString();\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tcfg.setStream(&data->stream_);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +int PipelineHandlerRPi::allocateBuffers(Camera *camera,\n>> +\t\t\t\t\tconst std::set &streams)\n>> +{\n>> +\tRPiCameraData *data = cameraData(camera);\n>> +\tStream *stream = *streams.begin();\n>> +\tconst StreamConfiguration &cfg = stream->configuration();\n>> +\tint ret;\n>> +\n>> +\tLOG(RPI, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n> \n> I'd drop this message, or move it to Camera::allocateBuffers().\n\nIts stream dependent ... so I'll just drop it.\n\n\n> \n>> +\n>> +\t/*\n>> +\t * Buffers are allocated on the camera, and the capture pad of the ISP\n>> +\t * unicam -> isp.output -> isp.capture -> Application\n>> +\t */\n>> +\n>> +\t/* Create a new intermediate buffer pool */\n> \n> s/pool/pool./ (same comment for all other comments in this file that\n> don't end with a period)\n\nDo you think you can make a rule for this in checkstyle.py?\n\n\n>> +\tdata->bayerBuffers_.createBuffers(cfg.bufferCount);\n>> +\n>> +\t/* Tie the unicam video buffers to the intermediate pool */\n>> +\tret = data->unicam_->exportBuffers(&data->bayerBuffers_);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tret = data->isp_->output()->importBuffers(&data->bayerBuffers_);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\t/* Tie the stream buffers to the capture device of the ISP */\n>> +\tif (stream->memoryType() == InternalMemory)\n>> +\t\tret = data->isp_->capture()->exportBuffers(&stream->bufferPool());\n>> +\telse\n>> +\t\tret = data->isp_->capture()->importBuffers(&stream->bufferPool());\n>> +\n>> +\treturn ret;\n>> +}\n>> +\n>> +int PipelineHandlerRPi::freeBuffers(Camera *camera,\n>> +\t\t\t\t const std::set &streams)\n>> +{\n>> +\tRPiCameraData *data = cameraData(camera);\n>> +\tint ret;\n>> +\n>> +\tret = data->unicam_->releaseBuffers();\n>> +\tret = data->isp_->output()->releaseBuffers();\n>> +\tret = data->isp_->capture()->releaseBuffers();\n> \n> You're losing the first two error values.\n\nYup - quick and dirty PoC :D\n\nThere are three cleanup operations. If one fails - should the other\nstill be attempted? Or just give up ?\n\n> \n>> +\n>> +\tdata->bayerBuffers_.destroyBuffers();\n>> +\n>> +\treturn ret;\n>> +}\n>> +\n>> +int PipelineHandlerRPi::start(Camera *camera)\n>> +{\n>> +\tRPiCameraData *data = cameraData(camera);\n>> +\tint ret;\n>> +\n>> +\tdata->rawBuffers_ = data->unicam_->queueAllBuffers();\n>> +\tif (data->rawBuffers_.empty()) {\n>> +\t\tLOG(RPI, Debug) << \"Failed to queue unicam buffers\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tLOG(RPI, Warning) << \"Using hard-coded exposure/gain defaults\";\n>> +\n>> +\tV4L2ControlList controls;\n>> +\tcontrols.add(V4L2_CID_EXPOSURE, 1700);\n>> +\tcontrols.add(V4L2_CID_ANALOGUE_GAIN, 180);\n>> +\tret = data->unicam_->setControls(&controls);\n>> +\tif (ret) {\n>> +\t\tLOG(RPI, Error) << \"Failed to set controls\";\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tret = data->isp_->output()->streamOn();\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tret = data->isp_->capture()->streamOn();\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tret = data->unicam_->streamOn();\n>> +\tif (ret)\n>> +\t\treturn ret;\n> \n> Shouldn't you stop streaming on the successfully started streams when an\n> error happens ?\n> \n>> +\n>> +\treturn ret;\n> \n> \treturn 0;\n> \n>> +}\n>> +\n>> +void PipelineHandlerRPi::stop(Camera *camera)\n>> +{\n>> +\tRPiCameraData *data = cameraData(camera);\n>> +\n>> +\tdata->isp_->capture()->streamOff();\n>> +\tdata->isp_->output()->streamOff();\n>> +\tdata->unicam_->streamOff();\n>> +\n>> +\tdata->rawBuffers_.clear();\n>> +}\n>> +\n>> +int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n>> +{\n>> +\tRPiCameraData *data = cameraData(camera);\n>> +\tBuffer *buffer = request->findBuffer(&data->stream_);\n>> +\tif (!buffer) {\n>> +\t\tLOG(RPI, Error)\n>> +\t\t\t<< \"Attempt to queue request with invalid stream\";\n>> +\n>> +\t\treturn -ENOENT;\n> \n> Can this happen ?\n\nI don't know - I think this was taken from the new buffer handling bits\nwhich have changed quite a lot lately.\n\nThis code exactly mirrors the queueRequest in the RkISP1, and Vimc.\n\n\n> \n>> +\t}\n>> +\n>> +\tint ret = data->isp_->capture()->queueBuffer(buffer);\n>> +\tif (ret < 0)\n>> +\t\treturn ret;\n>> +\n>> +\tPipelineHandler::queueRequest(camera, request);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n>> +{\n>> +\tDeviceMatch unicam(\"unicam\");\n>> +\tDeviceMatch codec(\"bcm2835-codec\");\n>> +\n>> +\t/* We explicitly need the ISP device from the MMAL codec driver. */\n>> +\tcodec.add(\"bcm2835-codec-isp\");\n> \n> Is there any subdev in the unicam device that we could match on ?\n\n\nPossibly one called \"unicam\" ... should we match \"unicam\":\"unicam\" ? or\nis \"unicam\" sufficient...\n\n\nI'll add:\n\n\t/* The video node is also named unicam. */\n\tunicam.add(\"unicam\");\n\n>> +\n>> +\tunicam_ = enumerator->search(unicam);\n>> +\tif (!unicam_)\n>> +\t\treturn false;\n>> +\n>> +\tcodec_ = enumerator->search(codec);\n>> +\tif (!codec_)\n>> +\t\treturn false;\n>> +\n>> +\tunicam_->acquire();\n>> +\tcodec_->acquire();\n>> +\n>> +\tstd::unique_ptr data = utils::make_unique(this);\n>> +\n>> +\t/* Locate and open the unicam video node. */\n>> +\tdata->unicam_ = new V4L2VideoDevice(unicam_->getEntityByName(\"unicam\"));\n>> +\tif (data->unicam_->open())\n>> +\t\treturn false;\n>> +\n>> +\t/* Locate the ISP M2M node */\n>> +\tMediaEntity *isp = codec_->getEntityByName(\"bcm2835-codec-isp\");\n>> +\tif (!isp)\n>> +\t\treturn false;\n>> +\n>> +\tdata->isp_ = new V4L2M2MDevice(isp->deviceNode());\n>> +\tif (data->isp_->status()) {\n>> +\t\tLOG(RPI, Error) << \"Could not create the ISP device\";\n>> +\t\treturn false;\n>> +\t}\n>> +\n>> +\tdata->unicam_->bufferReady.connect(data.get(), &RPiCameraData::sensorReady);\n>> +\tdata->isp_->output()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputReady);\n>> +\tdata->isp_->capture()->bufferReady.connect(data.get(), &RPiCameraData::ispCaptureReady);\n>> +\n>> +\t/* Identify the sensor */\n>> +\tfor (MediaEntity *entity : unicam_->entities()) {\n>> +\t\tif (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n>> +\t\t\tdata->sensor_ = new CameraSensor(entity);\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\tif (!data->sensor_)\n>> +\t\treturn false;\n>> +\n>> +\tint ret = data->sensor_->init();\n>> +\tif (ret)\n>> +\t\treturn false;\n> \n> No need for an intermediate variable, you can write\n> \n> \tif (data->sensor_->init())\n> \t\treturn false;\n\ndone\n\n>> +\n>> +\t/* Create and register the camera. */\n>> +\tstd::set streams{ &data->stream_ };\n>> +\tstd::shared_ptr camera =\n>> +\t\tCamera::create(this, data->sensor_->entity()->name(), streams);\n>> +\tregisterCamera(std::move(camera), std::move(data));\n>> +\n>> +\treturn true;\n>> +}\n>> +\n>> +void RPiCameraData::sensorReady(Buffer *buffer)\n>> +{\n>> +\t/* Deliver the frame from the sensor to the ISP */\n> \n> You should skip this when buffer->status() == Buffer::BufferCancelled\n> (see the IPU3 pipeline handler).\n\nHrm ... I had this ... I must have lost them along a rebase.\n\n\n\n> \n>> +\tisp_->output()->queueBuffer(buffer);\n>> +}\n>> +\n>> +void RPiCameraData::ispOutputReady(Buffer *buffer)\n>> +{\n>> +\t/* Return a completed buffer from the ISP back to the sensor */\n> \n> Same comment here.\n> \n>> +\tunicam_->queueBuffer(buffer);\n>> +}\n>> +\n>> +void RPiCameraData::ispCaptureReady(Buffer *buffer)\n>> +{\n>> +\tRequest *request = buffer->request();\n>> +\n>> +\tpipe_->completeBuffer(camera_, request, buffer);\n>> +\tpipe_->completeRequest(camera_, request);\n>> +}\n>> +\n>> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi);\n>> +\n>> +} /* namespace libcamera */\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DCC8C61620\n\tfor ;\n\tFri, 9 Aug 2019 13:19:21 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0ACA3CC;\n\tFri, 9 Aug 2019 13:19:20 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565349561;\n\tbh=Fd9hm04WWt6H7GtS1Lzs1Pq1qYI6NIUkfK0d7bwXKXQ=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=HtucRXJ55zyywdbuYmu2cYc7TlN14dOJzviE6wDg9caQMxE+xSgMKZLrCHw7o9krD\n\tlZR0Ou4So5/Bh1UUYH3BYhcsSntJKLN3ItQTis0AyUdEpbeD8WKVtWg2dhvRq4W2dk\n\tcJAm3EfR7SCMW8cgoAJfk2ItkR51j2lioyoknzak=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190808151221.24254-1-kieran.bingham@ideasonboard.com>\n\t<20190808151221.24254-7-kieran.bingham@ideasonboard.com>\n\t<20190808215114.GH6055@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<304fd2c6-3929-0363-db78-2f2a06b6d5b5@ideasonboard.com>","Date":"Fri, 9 Aug 2019 12:19:17 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190808215114.GH6055@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 6/6] [PoC/RFC] libcamera: pipeline:\n\tAdd RaspberryPi handler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 09 Aug 2019 11:19:22 -0000"}},{"id":2366,"web_url":"https://patchwork.libcamera.org/comment/2366/","msgid":"<20190809181329.GC5007@pendragon.ideasonboard.com>","date":"2019-08-09T18:13:29","subject":"Re: [libcamera-devel] [PATCH 6/6] [PoC/RFC] libcamera: pipeline:\n\tAdd RaspberryPi handler","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Aug 09, 2019 at 12:19:17PM +0100, Kieran Bingham wrote:\n> On 08/08/2019 22:51, Laurent Pinchart wrote:\n> > On Thu, Aug 08, 2019 at 04:12:21PM +0100, Kieran Bingham wrote:\n> >> Utilise the CameraSensor class and construct a pipeline for a single\n> >> sensor on the Unicam, routed through the V4L2 Codec ISP interface.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> > \n> > A pipeline handler for the RPi, very nice :-)\n> \n> A \"Proof of Concept\" pipeline handler :-) - It works - but needs more\n> development and requires out of tree drivers for the kernel, and\n> out-of-driver patches for the out of tree drivers :S\n> \n> We'll get there :-D\n> \n> Thanks for the review comments. I've pretty much taken most of them in.\n> \n> >> ---\n> >> src/libcamera/pipeline/meson.build | 1 +\n> >> src/libcamera/pipeline/raspberrypi.cpp | 425 +++++++++++++++++++++++++\n> >> 2 files changed, 426 insertions(+)\n> >> create mode 100644 src/libcamera/pipeline/raspberrypi.cpp\n> >>\n> >> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> >> index 0d466225a72e..7ed7b26f3033 100644\n> >> --- a/src/libcamera/pipeline/meson.build\n> >> +++ b/src/libcamera/pipeline/meson.build\n> >> @@ -1,4 +1,5 @@\n> >> libcamera_sources += files([\n> >> + 'raspberrypi.cpp',\n> > \n> > I would expect the same level of complexity as the IPU3 and RkISP1\n> > pipeline handlers, should this get its own directory ?\n> \n> As yet, I've seen only a single file for the IPU3, and RKISP1.\n> \n> What other files are expected? The IPA code will go in a separate\n> directory right? So I'm not yet sure what will be broken out in the\n> pipeline handlers to need their own directory.\n\nI'm not sure yet. Let's leave it as-is for now.\n\n> (Except the IPU3 - That looks like it could be split to have an\n> ipu3.cpp, imgu.cpp, cio2.cpp.)\n> \n> >> 'uvcvideo.cpp',\n> >> 'vimc.cpp',\n> >> ])\n> >> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp\n> >> new file mode 100644\n> >> index 000000000000..4c4c3dea0113\n> >> --- /dev/null\n> >> +++ b/src/libcamera/pipeline/raspberrypi.cpp\n> >> @@ -0,0 +1,425 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * raspberrypi.cpp - Pipeline handler for raspberrypi devices\n> > \n> > s/raspberrypi devices/Raspberry Pi devices/ ?\n> > \n> > Should we name it Raspberry Pi, or based on the SoC ? It could be used\n> > on other systems using the same SoC (or family of SoCs).\n> \n> I expect this pipeline handler to function on all of the Raspberry Pi's\n> with a camera interface. This covers multiple SoCs.\n> \n> So lets stick with RaspberryPi as the name I think.\n\nOK.\n\n> >> + */\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \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_controls.h\"\n> >> +#include \"v4l2_videodevice.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +LOG_DEFINE_CATEGORY(RPI)\n> >> +\n> >> +class RPiCameraData : public CameraData\n> >> +{\n> >> +public:\n> >> +\tRPiCameraData(PipelineHandler *pipe)\n> >> +\t\t: CameraData(pipe), sensor_(nullptr), unicam_(nullptr),\n> >> +\t\t isp_(nullptr)\n> >> +\t{\n> >> +\t}\n> >> +\n> >> +\t~RPiCameraData()\n> >> +\t{\n> >> +\t\tbayerBuffers_.destroyBuffers();\n> > \n> > Shouldn't you also delete sensor_ ?\n> \n> Yup.\n> \n> >> +\t\tdelete unicam_;\n> >> +\t\tdelete isp_;\n> >> +\t}\n> >> +\n> >> +\tvoid sensorReady(Buffer *buffer);\n> >> +\tvoid ispOutputReady(Buffer *buffer);\n> >> +\tvoid ispCaptureReady(Buffer *buffer);\n> >> +\n> >> +\tCameraSensor *sensor_;\n> >> +\tV4L2VideoDevice *unicam_;\n> >> +\tV4L2M2MDevice *isp_;\n> >> +\tStream stream_;\n> >> +\n> >> +\tBufferPool bayerBuffers_;\n> >> +\tstd::vector> rawBuffers_;\n> >> +};\n> >> +\n> >> +class RPiCameraConfiguration : public CameraConfiguration\n> >> +{\n> >> +public:\n> >> +\tRPiCameraConfiguration();\n> >> +\n> >> +\tStatus validate() override;\n> >> +};\n> >> +\n> >> +class PipelineHandlerRPi : public PipelineHandler\n> >> +{\n> >> +public:\n> >> +\tPipelineHandlerRPi(CameraManager *manager);\n> >> +\t~PipelineHandlerRPi();\n> >> +\n> >> +\tCameraConfiguration *\n> >> +\tgenerateConfiguration(Camera *camera,\n> >> +\t\t\t const StreamRoles &roles) override;\n> >> +\tint configure(Camera *camera,\n> >> +\t\t CameraConfiguration *config) override;\n> >> +\n> >> +\tint allocateBuffers(Camera *camera,\n> >> +\t\t\t const std::set &streams) override;\n> >> +\tint freeBuffers(Camera *camera,\n> >> +\t\t\tconst std::set &streams) 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> >> +\tRPiCameraData *cameraData(const Camera *camera)\n> >> +\t{\n> >> +\t\treturn static_cast(\n> >> +\t\t\tPipelineHandler::cameraData(camera));\n> >> +\t}\n> >> +\n> >> +\tstd::shared_ptr unicam_;\n> >> +\tstd::shared_ptr codec_;\n> >> +};\n> >> +\n> >> +RPiCameraConfiguration::RPiCameraConfiguration()\n> >> +\t: CameraConfiguration()\n> >> +{\n> >> +}\n> >> +\n> >> +CameraConfiguration::Status RPiCameraConfiguration::validate()\n> >> +{\n> >> +\tStatus status = Valid;\n> >> +\n> >> +\tif (config_.empty())\n> >> +\t\treturn Invalid;\n> >> +\n> >> +\t/* Todo: Experiment with increased stream support through the ISP. */\n> > \n> > s/Todo:/\\todo/\n> > \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/* Todo: restrict to hardware capabilities. */\n> > \n> > I think this one should be addressed before merging the code.\n> \n> Yes, needs more development.\n> \n> >> +\n> >> +\tcfg.bufferCount = 4;\n> >> +\n> >> +\treturn status;\n> >> +}\n> >> +\n> >> +PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> >> +\t: PipelineHandler(manager), unicam_(nullptr), codec_(nullptr)\n> >> +{\n> >> +}\n> >> +\n> >> +PipelineHandlerRPi::~PipelineHandlerRPi()\n> >> +{\n> >> +\tif (unicam_)\n> >> +\t\tunicam_->release();\n> >> +\n> >> +\tif (codec_)\n> >> +\t\tcodec_->release();\n> >> +}\n> >> +\n> >> +CameraConfiguration *\n> >> +PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >> +\t\t\t\t\t const StreamRoles &roles)\n> >> +{\n> >> +\tCameraConfiguration *config = new RPiCameraConfiguration();\n> >> +\n> >> +\tif (roles.empty())\n> >> +\t\treturn config;\n> >> +\n> >> +\tStreamConfiguration cfg{};\n> >> +\tcfg.pixelFormat = V4L2_PIX_FMT_YUYV;\n> >> +\tcfg.size = { 1920, 1080 }; // data->sensor_->resolution();\n> > \n> > Let's remove commented-out code.\n> > \n> > What prevents from using the sensor resolution, and how is 1080p\n> > selected as the default resolution ?\n> \n> The RaspberryPi v2 camera defaults as a 3280x2464 output.\n> \n> The ISP has currently got an (incorrect) artificial limitation of 1080p.\n> \n> We can work through this when we get updated ISP support.\n> \n> >> +\tcfg.bufferCount = 4;\n> >> +\n> >> +\tLOG(RPI, Debug) << \"Config default as \" << cfg.toString();\n> > \n> > I think the configuration is printed in the caller already.\n> \n> Removed.\n> \n> >> +\n> >> +\tconfig->addConfiguration(cfg);\n> >> +\n> >> +\tconfig->validate();\n> >> +\n> >> +\treturn config;\n> >> +}\n> >> +\n> >> +int PipelineHandlerRPi::configure(Camera *camera,\n> >> +\t\t\t\t CameraConfiguration *config)\n> > \n> > This holds on a single line.\n> > \n> >> +{\n> >> +\tRPiCameraData *data = cameraData(camera);\n> >> +\tStreamConfiguration &cfg = config->at(0);\n> >> +\tint ret;\n> >> +\n> >> +\tSize sensorSize = { 1920, 1080 };\n> >> +\tSize outputSize = { 1920, 1088 };\n> >> +\n> >> +\tV4L2DeviceFormat format = {};\n> >> +\tformat.size = sensorSize;\n> >> +\tformat.fourcc = V4L2_PIX_FMT_SRGGB10P;\n> > \n> > This all seems to lack genericity :-) I think that at least he format\n> > should support different Bayer patterns and bpp values.\n> > \n> > Don't you need to set the format on the sensor subdev ?\n> > \n> >> +\n> >> +\tLOG(RPI, Debug) << \"Setting format to \" << format.toString();\n> >> +\n> >> +\tret = data->unicam_->setFormat(&format);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tif (format.size != sensorSize ||\n> >> +\t format.fourcc != V4L2_PIX_FMT_SRGGB10P) {\n> >> +\t\tLOG(RPI, Error)\n> >> +\t\t\t<< \"Failed to set format on Video device: \"\n> >> +\t\t\t<< format.toString();\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\tformat.size = outputSize;\n> >> +\n> >> +\tret = data->isp_->output()->setFormat(&format);\n> >> +\n> >> +\tif (format.size != outputSize ||\n> >> +\t format.fourcc != V4L2_PIX_FMT_SRGGB10P) {\n> >> +\t\tLOG(RPI, Error) << \"Failed to set format on ISP output device: \"\n> >> +\t\t\t\t<< format.toString();\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\t/* Configure the ISP based to generate the requested size and format. */\n> > \n> > Are you missing something after \"based\" ?\n> \n> s/based//\n> \n> Or it could simply be \"Configure the ISP\" ... but we have limited\n> configuration so far...\n> \n> >> +\tformat.size = cfg.size;\n> >> +\tformat.fourcc = cfg.pixelFormat;\n> >> +\n> >> +\tret = data->isp_->capture()->setFormat(&format);\n> >> +\n> >> +\tif (format.size != cfg.size ||\n> >> +\t format.fourcc != cfg.pixelFormat) {\n> >> +\t\tLOG(RPI, Error)\n> >> +\t\t\t<< \"Failed to set format on ISP capture device: \"\n> >> +\t\t\t<< format.toString();\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\tcfg.setStream(&data->stream_);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int PipelineHandlerRPi::allocateBuffers(Camera *camera,\n> >> +\t\t\t\t\tconst std::set &streams)\n> >> +{\n> >> +\tRPiCameraData *data = cameraData(camera);\n> >> +\tStream *stream = *streams.begin();\n> >> +\tconst StreamConfiguration &cfg = stream->configuration();\n> >> +\tint ret;\n> >> +\n> >> +\tLOG(RPI, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n> > \n> > I'd drop this message, or move it to Camera::allocateBuffers().\n> \n> Its stream dependent ... so I'll just drop it.\n> \n> >> +\n> >> +\t/*\n> >> +\t * Buffers are allocated on the camera, and the capture pad of the ISP\n> >> +\t * unicam -> isp.output -> isp.capture -> Application\n> >> +\t */\n> >> +\n> >> +\t/* Create a new intermediate buffer pool */\n> > \n> > s/pool/pool./ (same comment for all other comments in this file that\n> > don't end with a period)\n> \n> Do you think you can make a rule for this in checkstyle.py?\n\nI've added it to the todo list.\n\n> >> +\tdata->bayerBuffers_.createBuffers(cfg.bufferCount);\n> >> +\n> >> +\t/* Tie the unicam video buffers to the intermediate pool */\n> >> +\tret = data->unicam_->exportBuffers(&data->bayerBuffers_);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = data->isp_->output()->importBuffers(&data->bayerBuffers_);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\t/* Tie the stream buffers to the capture device of the ISP */\n> >> +\tif (stream->memoryType() == InternalMemory)\n> >> +\t\tret = data->isp_->capture()->exportBuffers(&stream->bufferPool());\n> >> +\telse\n> >> +\t\tret = data->isp_->capture()->importBuffers(&stream->bufferPool());\n> >> +\n> >> +\treturn ret;\n> >> +}\n> >> +\n> >> +int PipelineHandlerRPi::freeBuffers(Camera *camera,\n> >> +\t\t\t\t const std::set &streams)\n> >> +{\n> >> +\tRPiCameraData *data = cameraData(camera);\n> >> +\tint ret;\n> >> +\n> >> +\tret = data->unicam_->releaseBuffers();\n> >> +\tret = data->isp_->output()->releaseBuffers();\n> >> +\tret = data->isp_->capture()->releaseBuffers();\n> > \n> > You're losing the first two error values.\n> \n> Yup - quick and dirty PoC :D\n> \n> There are three cleanup operations. If one fails - should the other\n> still be attempted? Or just give up ?\n\nI think they should be attempted as it's a cleanup path.\n\n> >> +\n> >> +\tdata->bayerBuffers_.destroyBuffers();\n> >> +\n> >> +\treturn ret;\n> >> +}\n> >> +\n> >> +int PipelineHandlerRPi::start(Camera *camera)\n> >> +{\n> >> +\tRPiCameraData *data = cameraData(camera);\n> >> +\tint ret;\n> >> +\n> >> +\tdata->rawBuffers_ = data->unicam_->queueAllBuffers();\n> >> +\tif (data->rawBuffers_.empty()) {\n> >> +\t\tLOG(RPI, Debug) << \"Failed to queue unicam buffers\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\tLOG(RPI, Warning) << \"Using hard-coded exposure/gain defaults\";\n> >> +\n> >> +\tV4L2ControlList controls;\n> >> +\tcontrols.add(V4L2_CID_EXPOSURE, 1700);\n> >> +\tcontrols.add(V4L2_CID_ANALOGUE_GAIN, 180);\n> >> +\tret = data->unicam_->setControls(&controls);\n> >> +\tif (ret) {\n> >> +\t\tLOG(RPI, Error) << \"Failed to set controls\";\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tret = data->isp_->output()->streamOn();\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = data->isp_->capture()->streamOn();\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = data->unicam_->streamOn();\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> > \n> > Shouldn't you stop streaming on the successfully started streams when an\n> > error happens ?\n> > \n> >> +\n> >> +\treturn ret;\n> > \n> > \treturn 0;\n> > \n> >> +}\n> >> +\n> >> +void PipelineHandlerRPi::stop(Camera *camera)\n> >> +{\n> >> +\tRPiCameraData *data = cameraData(camera);\n> >> +\n> >> +\tdata->isp_->capture()->streamOff();\n> >> +\tdata->isp_->output()->streamOff();\n> >> +\tdata->unicam_->streamOff();\n> >> +\n> >> +\tdata->rawBuffers_.clear();\n> >> +}\n> >> +\n> >> +int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n> >> +{\n> >> +\tRPiCameraData *data = cameraData(camera);\n> >> +\tBuffer *buffer = request->findBuffer(&data->stream_);\n> >> +\tif (!buffer) {\n> >> +\t\tLOG(RPI, Error)\n> >> +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> >> +\n> >> +\t\treturn -ENOENT;\n> > \n> > Can this happen ?\n> \n> I don't know - I think this was taken from the new buffer handling bits\n> which have changed quite a lot lately.\n\nMy question was whether the Camera class had checks that would prevent\nthis from happening.\n\n> This code exactly mirrors the queueRequest in the RkISP1, and Vimc.\n> \n> >> +\t}\n> >> +\n> >> +\tint ret = data->isp_->capture()->queueBuffer(buffer);\n> >> +\tif (ret < 0)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tPipelineHandler::queueRequest(camera, request);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> >> +{\n> >> +\tDeviceMatch unicam(\"unicam\");\n> >> +\tDeviceMatch codec(\"bcm2835-codec\");\n> >> +\n> >> +\t/* We explicitly need the ISP device from the MMAL codec driver. */\n> >> +\tcodec.add(\"bcm2835-codec-isp\");\n> > \n> > Is there any subdev in the unicam device that we could match on ?\n> \n> Possibly one called \"unicam\" ... should we match \"unicam\":\"unicam\" ? or\n> is \"unicam\" sufficient...\n> \n> I'll add:\n> \n> \t/* The video node is also named unicam. */\n> \tunicam.add(\"unicam\");\n\nWorks for me.\n\n> >> +\n> >> +\tunicam_ = enumerator->search(unicam);\n> >> +\tif (!unicam_)\n> >> +\t\treturn false;\n> >> +\n> >> +\tcodec_ = enumerator->search(codec);\n> >> +\tif (!codec_)\n> >> +\t\treturn false;\n> >> +\n> >> +\tunicam_->acquire();\n> >> +\tcodec_->acquire();\n> >> +\n> >> +\tstd::unique_ptr data = utils::make_unique(this);\n> >> +\n> >> +\t/* Locate and open the unicam video node. */\n> >> +\tdata->unicam_ = new V4L2VideoDevice(unicam_->getEntityByName(\"unicam\"));\n> >> +\tif (data->unicam_->open())\n> >> +\t\treturn false;\n> >> +\n> >> +\t/* Locate the ISP M2M node */\n> >> +\tMediaEntity *isp = codec_->getEntityByName(\"bcm2835-codec-isp\");\n> >> +\tif (!isp)\n> >> +\t\treturn false;\n> >> +\n> >> +\tdata->isp_ = new V4L2M2MDevice(isp->deviceNode());\n> >> +\tif (data->isp_->status()) {\n> >> +\t\tLOG(RPI, Error) << \"Could not create the ISP device\";\n> >> +\t\treturn false;\n> >> +\t}\n> >> +\n> >> +\tdata->unicam_->bufferReady.connect(data.get(), &RPiCameraData::sensorReady);\n> >> +\tdata->isp_->output()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputReady);\n> >> +\tdata->isp_->capture()->bufferReady.connect(data.get(), &RPiCameraData::ispCaptureReady);\n> >> +\n> >> +\t/* Identify the sensor */\n> >> +\tfor (MediaEntity *entity : unicam_->entities()) {\n> >> +\t\tif (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> >> +\t\t\tdata->sensor_ = new CameraSensor(entity);\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\tif (!data->sensor_)\n> >> +\t\treturn false;\n> >> +\n> >> +\tint ret = data->sensor_->init();\n> >> +\tif (ret)\n> >> +\t\treturn false;\n> > \n> > No need for an intermediate variable, you can write\n> > \n> > \tif (data->sensor_->init())\n> > \t\treturn false;\n> \n> done\n> \n> >> +\n> >> +\t/* Create and register the camera. */\n> >> +\tstd::set streams{ &data->stream_ };\n> >> +\tstd::shared_ptr camera =\n> >> +\t\tCamera::create(this, data->sensor_->entity()->name(), streams);\n> >> +\tregisterCamera(std::move(camera), std::move(data));\n> >> +\n> >> +\treturn true;\n> >> +}\n> >> +\n> >> +void RPiCameraData::sensorReady(Buffer *buffer)\n> >> +{\n> >> +\t/* Deliver the frame from the sensor to the ISP */\n> > \n> > You should skip this when buffer->status() == Buffer::BufferCancelled\n> > (see the IPU3 pipeline handler).\n> \n> Hrm ... I had this ... I must have lost them along a rebase.\n> \n> >> +\tisp_->output()->queueBuffer(buffer);\n> >> +}\n> >> +\n> >> +void RPiCameraData::ispOutputReady(Buffer *buffer)\n> >> +{\n> >> +\t/* Return a completed buffer from the ISP back to the sensor */\n> > \n> > Same comment here.\n> > \n> >> +\tunicam_->queueBuffer(buffer);\n> >> +}\n> >> +\n> >> +void RPiCameraData::ispCaptureReady(Buffer *buffer)\n> >> +{\n> >> +\tRequest *request = buffer->request();\n> >> +\n> >> +\tpipe_->completeBuffer(camera_, request, buffer);\n> >> +\tpipe_->completeRequest(camera_, request);\n> >> +}\n> >> +\n> >> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi);\n> >> +\n> >> +} /* namespace libcamera */","headers":{"Return-Path":"","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 58D2A60E2F\n\tfor ;\n\tFri, 9 Aug 2019 20:13:32 +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 A7B23CC;\n\tFri, 9 Aug 2019 20:13:31 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565374411;\n\tbh=vWAGNuwBE+8aL3Lc/CaQNs0OyStlj57Dh4+Y79Fd46Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VipjWKSLQWiWXRbK2HTMNdR2YrABknkFI3NMPSvzEd9NzuSPWXiEEh522E1UIw9iG\n\tQwXbWWCryenadTCA2AFkRjYwn/VvwQXemhBaPjuO7WIzc78GSGx22uCJY9A9oykceg\n\tzGGGwxv1v4E1GEDh5cqSbtzJXHXr5d2SupQ6RsmA=","Date":"Fri, 9 Aug 2019 21:13:29 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190809181329.GC5007@pendragon.ideasonboard.com>","References":"<20190808151221.24254-1-kieran.bingham@ideasonboard.com>\n\t<20190808151221.24254-7-kieran.bingham@ideasonboard.com>\n\t<20190808215114.GH6055@pendragon.ideasonboard.com>\n\t<304fd2c6-3929-0363-db78-2f2a06b6d5b5@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<304fd2c6-3929-0363-db78-2f2a06b6d5b5@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 6/6] [PoC/RFC] libcamera: pipeline:\n\tAdd RaspberryPi handler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 09 Aug 2019 18:13:32 -0000"}}]