[{"id":18691,"web_url":"https://patchwork.libcamera.org/comment/18691/","msgid":"<20210811100339.GY2167@pyrite.rasen.tech>","date":"2021-08-11T10:03:39","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/4] pipeline: isp: The\n\tsoftware ISP-based pipeline handler","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Siyuan,\n\nOn Wed, Aug 11, 2021 at 07:12:55AM +0100, Siyuan Fan wrote:\n> From: Fan Siyuan \n> \n> Changes in V2:\n> -fix the raw and rgb data flow based queue model in pipeline handler\n> -move buffer alloc and thread to ISP class\n> -Fill metadata information in dstBuffer\n> \n> Signed-off-by: Fan Siyuan \n> ---\n> src/libcamera/pipeline/isp/isp.cpp | 306 +++++++++++++++++++++++++++++\n> 1 file changed, 306 insertions(+)\n> create mode 100644 src/libcamera/pipeline/isp/isp.cpp\n> \n> diff --git a/src/libcamera/pipeline/isp/isp.cpp b/src/libcamera/pipeline/isp/isp.cpp\n> new file mode 100644\n> index 00000000..c6b7808c\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/isp/isp.cpp\n> @@ -0,0 +1,306 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Siyuan Fan \n> + *\n> + * isp.cpp - The software ISP-based pipeline handler\n> + */\n> +\n> +#include \"../../swisp/isp.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/device_enumerator.h\"\n> +#include \"libcamera/internal/media_device.h\"\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/v4l2_videodevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(ISP)\n> +\n> +class ISPCameraData : public CameraData\n> +{\n> +public:\n> + ISPCameraData(PipelineHandler *pipe, MediaDevice *media)\n> + : CameraData(pipe), media_(media), video_(nullptr)\n> + {\n> + }\n> +\n> + ~ISPCameraData()\n> + {\n> + delete video_;\n> + }\n> +\n> + int init();\n> + void bufferReady(FrameBuffer *buffer);\n> + void ISPCompleted(FrameBuffer *buffer);\n> +\n> + Stream stream_;\n> + CPU_ISP isp_;\n> + int width_;\n> + int height_;\n> +\n> + std::vector> rawBuffers_;\n> + std::vector rawQueueBuffers_;\n> + std::vector rgbQueueBuffers_;\n\nThese are queues, so they should be std::queue\n\nAlso in english we would say rawBufferQueue.\n\n> +\n> + MediaDevice *media_;\n> + V4L2VideoDevice *video_;\n> +};\n> +\n> +\n> +class ISPCameraConfiguration : public CameraConfiguration\n> +{\n> +public:\n> + ISPCameraConfiguration();\n> +\n> + Status validate() override;\n> +};\n> +\n> +class PipelineHandlerISP : public PipelineHandler\n> +{\n> +public:\n> + PipelineHandlerISP(CameraManager *manager);\n> +\n> + CameraConfiguration *generateConfiguration(Camera *camera,\n> + const StreamRoles &roles) override;\n> + int configure(Camera *camera, CameraConfiguration *config) override;\n> +\n> + int exportFrameBuffers(Camera *camera, Stream *stream,\n> + std::vector> *buffers) override;\n> +\n> + int start(Camera *camera, const ControlList *controls) override;\n> + void stop(Camera *camera) override;\n> +\n> + int queueRequestDevice(Camera *camera, Request *request) override;\n> +\n> + bool match(DeviceEnumerator *enumerator) override;\n> +\n> +private:\n> + ISPCameraData *cameraData(const Camera *camera)\n> + {\n> + return static_cast(\n> + PipelineHandler::cameraData(camera));\n> + }\n> +};\n> +\n> +ISPCameraConfiguration::ISPCameraConfiguration()\n> + : CameraConfiguration()\n> +{\n> +}\n> +\n> +CameraConfiguration::Status ISPCameraConfiguration::validate()\n> +{\n> + Status status = Valid;\n> +\n> + return status;\n> +}\n> +\n> +PipelineHandlerISP::PipelineHandlerISP(CameraManager *manager)\n> + : PipelineHandler(manager)\n> +{\n> +}\n> +\n> +CameraConfiguration *PipelineHandlerISP::generateConfiguration(Camera *camera,\n> + const StreamRoles &roles)\n> +{\n> + ISPCameraData *data = cameraData(camera);\n> + CameraConfiguration *config = new ISPCameraConfiguration();\n> +\n> + if (roles.empty())\n> + return config;\n> +\n> + std::map> v4l2Formats =\n> + data->video_->formats();\n\nSimilar to this, I think it would be good for the ISP to report what\nformats it supports.\n\n> + std::map> deviceFormats;\n> + std::transform(v4l2Formats.begin(), v4l2Formats.end(),\n> + std::inserter(deviceFormats, deviceFormats.begin()),\n> + [&](const decltype(v4l2Formats)::value_type &format) {\n> + return decltype(deviceFormats)::value_type{\n> + format.first.toPixelFormat(),\n> + format.second\n> + };\n> + });\n> +\n> + StreamFormats formats(deviceFormats);\n> + StreamConfiguration cfg(formats);\n> +\n> + cfg.pixelFormat = formats::RGB888;\n> + cfg.size = { 640, 480 };\n> + cfg.bufferCount = 4;\n> +\n> + config->addConfiguration(cfg);\n> +\n> + config->validate();\n> +\n> + return config;\n> +}\n> +\n> +int PipelineHandlerISP::configure(Camera *camera, CameraConfiguration *config)\n> +{\n> + ISPCameraData *data = cameraData(camera);\n> + StreamConfiguration &cfg = config->at(0);\n> +\n> + V4L2VideoDevice::Formats fmts = data->video_->formats();\n> + V4L2PixelFormat v4l2Format = fmts.begin()->first;\n> +\n> + V4L2DeviceFormat format = {};\n> + format.fourcc = v4l2Format;\n> + format.size = cfg.size;\n> +\n> + data->width_ = format.size.width;\n> + data->height_ = format.size.height;\n> +\n> + int ret = data->video_->setFormat(&format);\n> + if (ret)\n> + return ret;\n\nThe ISP should have a configure() function of some sort as well.\n\n> +\n> + cfg.setStream(&data->stream_);\n> + cfg.stride = format.planes[0].bpl;\n> +\n> + return 0;\n> +}\n> +\n> +int PipelineHandlerISP::exportFrameBuffers(Camera *camera, Stream *stream,\n> + std::vector> *buffers)\n> +{\n> + unsigned int count = stream->configuration().bufferCount;\n> + ISPCameraData *data = cameraData(camera);\n> +\n> + count = data->isp_.exportBuffers(buffers, count, data->width_, data->height_);\n> +\n> + return count; \n> +\n> +}\n> +\n> +int PipelineHandlerISP::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> +{\n> + ISPCameraData *data = cameraData(camera);\n> + unsigned int count = data->stream_.configuration().bufferCount;\n> +\n> + int ret = data->video_->allocateBuffers(count, &data->rawBuffers_);\n> + if (ret < 0) {\n> + LOG(ISP, Error) << strerror(-ret);\n> + return ret;\n> + }\n> +\n> + for (unsigned int i = 0; i < count; i++) {\n> + data->rawQueueBuffers_.push_back(data->rawBuffers_[i].get());\n> + }\n> + \n> +\n> + ret = data->video_->streamOn();\n> + if (ret < 0) {\n> + data->video_->releaseBuffers();\n> + return ret;\n> + }\n> +\n> + data->isp_.startThreadISP();\n> +\n> + return 0;\n> +}\n> +\n> +void PipelineHandlerISP::stop(Camera *camera)\n> +{\n> + ISPCameraData *data = cameraData(camera);\n> +\n> + if (!(data->rawBuffers_.empty())) {\n> + data->rawBuffers_.clear();\n> + }\n> +\n> + data->isp_.stopThreadISP();\n> +\n> + data->video_->streamOff();\n> + data->video_->releaseBuffers();\n> +}\n> +\n> +int PipelineHandlerISP::queueRequestDevice(Camera *camera, Request *request)\n> +{\n> + ISPCameraData *data = cameraData(camera);\n> + FrameBuffer *rgbBuffer = request->findBuffer(&data->stream_);\n> + data->rgbQueueBuffers_.push_back(rgbBuffer);\n\nThis should go after the error check. If there's no buffer then you\nshould't queue it.\n\n> + \n> + if (!rgbBuffer) {\n> + LOG(ISP, Error) << \"Attempt to queue request with invalid stream\";\n> + return -ENOENT;\n> + }\n> +\n\nFrameBuffer *buffer = data->rawQueueBuffers_.front();\n\n> + int ret = data->video_->queueBuffer(data->rawQueueBuffers_[0]);\n\ndata->video_->queueBuffer(buffer);\n\n> + if (ret < 0)\n> + return ret;\n> + FrameBuffer *temp = data->rawQueueBuffers_[0];\n> + data->rawQueueBuffers_.erase(data->rawQueueBuffers_.begin());\n> + data->rawQueueBuffers_.push_back(std::move(temp));\n\ndata->video_->rawQueueBuffers_.pop();\n\nDon't requeue the buffer; it'll come back to you in bufferReady.\n\n> +\n> + return 0;\n> +}\n> +\n> +bool PipelineHandlerISP::match(DeviceEnumerator *enumerator)\n> +{\n> + DeviceMatch unicam(\"unicam\");\n> +\n> + unicam.add(\"unicam-embedded\");\n> + unicam.add(\"unicam-image\");\n> +\n> + MediaDevice *unicam_ = acquireMediaDevice(enumerator, unicam);\n> + if (!unicam_) {\n> + LOG(ISP, Debug) << \"unicam Device not found\";\n> + return false;\n> + }\n> +\n> + LOG(ISP, Debug) << \"unicam Device Identified\";\n> +\n> + std::unique_ptr data = std::make_unique(this, unicam_);\n> +\n> + if(data->init()) return false;\n> +\n> + std::set streams{&data->stream_};\n> + std::shared_ptr camera = Camera::create(this, data->video_->deviceName(), streams);\n> + registerCamera(std::move(camera), std::move(data));\n> +\n> + return true;\n> +}\n> +\n> +\n> +void ISPCameraData::ISPCompleted(FrameBuffer *buffer)\n> +{\n> + Request *request = buffer->request();\n\nYou need to return the raw buffer to rawQueueBuffers_.\n\n> +\n> + pipe_->completeBuffer(request, buffer);\n> + pipe_->completeRequest(request);\n> +\n> +}\n> +\n> +void ISPCameraData::bufferReady(FrameBuffer *buffer)\n> +{\n> + LOG(ISP, Debug) << rgbQueueBuffers_[0]->planes()[0].fd.fd();\n> + isp_.invokeMethod(&CPU_ISP::processing, ConnectionTypeQueued, buffer, rgbQueueBuffers_[0], width_, height_);\n\nFrameBuffer *rgbBuf = rgbQueueBuffers_.front();\n\nisp_.invokeMethod(..., buffer, rgbBuf, ...);\n\n> + rgbQueueBuffers_.erase(rgbQueueBuffers_.begin());\n> + rgbQueueBuffers_.shrink_to_fit();\n\nrgbQueueBuffers_.pop();\n\n\nAlmost there!\n\nPaul\n\n> + \n> +}\n> +\n> +int ISPCameraData::init()\n> +{\n> + video_ = new V4L2VideoDevice(media_->getEntityByName(\"unicam-image\"));\n> + if (video_->open())\n> + return -ENODEV;\n> +\n> + video_->bufferReady.connect(this, &ISPCameraData::bufferReady);\n> + isp_.ispCompleted.connect(this, &ISPCameraData::ISPCompleted);\n> +\n> + return 0;\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerISP)\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.20.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 30BA6BD87D\n\tfor ;\n\tWed, 11 Aug 2021 10:03:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 756BB6884F;\n\tWed, 11 Aug 2021 12:03:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F3CC68826\n\tfor ;\n\tWed, 11 Aug 2021 12:03:47 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C784ADD;\n\tWed, 11 Aug 2021 12:03:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QR0KemhE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628676227;\n\tbh=gsOTs/tmZEU3mIdMd0ydOAP9+80qN7Oy0zDBWflURJI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QR0KemhEoRFczPAUnxnG7jGZ7afwk3qeL+IpRGZEhH116FFR4mNjCcNOG7Q9jCsuv\n\t4bMDLMsfd7rgtLtKQuJ8fs5JR8L0Z22hSiuSCUZeWh9dcpqThX07jr3iG4apdR8Dyi\n\tLjRuwecquwWym+PtOhQVqY5ID1y2/iS6Ad2gub5U=","Date":"Wed, 11 Aug 2021 19:03:39 +0900","From":"paul.elder@ideasonboard.com","To":"Siyuan Fan ","Message-ID":"<20210811100339.GY2167@pyrite.rasen.tech>","References":"<20210811061258.7421-1-siyuan.fan@foxmail.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/4] pipeline: isp: The\n\tsoftware ISP-based pipeline handler","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]