[{"id":2552,"web_url":"https://patchwork.libcamera.org/comment/2552/","msgid":"<826201d3-8f85-7b3b-1c43-7e16c21602a0@ideasonboard.com>","date":"2019-08-29T16:28:38","subject":"Re: [libcamera-devel] [PATCH 13/13] libcamera: pipeline: rkisp1:\n\tAttach to an IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nThe final piece of the puzzle :-D\n\nOn 28/08/2019 02:17, Niklas Söderlund wrote:\n> Add the plumbing to the pipeline handler to interact with an IPA module.\n> To support this parameter and statistic buffers needs to be associated\n> with every request queued. The parameters buffer needs to be passed to\n> the IPA before any buffer in the request is queued to hardware and the\n> statistics buffer needs to be passed to the IPA for inspection as soon\n> as it's ready.\n> \n> This change makes the usage of an IPA module mandatory for the rkisp1\n> pipeline.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 263 ++++++++++++++++++++++-\n>  1 file changed, 252 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index de4ab523d0e4fe36..80d4832c49ebe78c 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -9,7 +9,7 @@\n>  #include <array>\n>  #include <iomanip>\n>  #include <memory>\n> -#include <vector>\n> +#include <queue>\n>  \n>  #include <linux/media-bus-format.h>\n>  \n> @@ -34,7 +34,7 @@ class RkISP1CameraData : public CameraData\n>  {\n>  public:\n>  \tRkISP1CameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe), sensor_(nullptr)\n> +\t\t: CameraData(pipe, 1, 1), sensor_(nullptr)\n>  \t{\n>  \t}\n>  \n> @@ -43,8 +43,21 @@ public:\n>  \t\tdelete sensor_;\n>  \t}\n>  \n> +\tint initCameraData() override;\n> +\n>  \tStream stream_;\n>  \tCameraSensor *sensor_;\n> +\n> +private:\n> +\tvoid updateSensor(V4L2ControlList controls);\n> +\tvoid queueRequestHardware(const void *cookie);\n> +};\n> +\n> +class RkISP1RequestData : public RequestData\n> +{\n> +public:\n> +\tBuffer *stat;\n> +\tBuffer *param;\n>  };\n>  \n>  class RkISP1CameraConfiguration : public CameraConfiguration\n> @@ -99,18 +112,69 @@ private:\n>  \t\t\tPipelineHandler::cameraData(camera));\n>  \t}\n>  \n> +\tfriend RkISP1CameraData;\n\nIs this necessary? Isn't almost everything in a CameraData class public?\nIf this is required - should members in CameraData be moved to\nprotected/private?\n\n> +\n>  \tint initLinks();\n>  \tint createCamera(MediaEntity *sensor);\n> +\tvoid tryCompleteRequest(Request *request);\n>  \tvoid bufferReady(Buffer *buffer);\n> +\tvoid statReady(Buffer *buffer);\n> +\tvoid paramReady(Buffer *buffer);\n>  \n>  \tMediaDevice *media_;\n>  \tV4L2Subdevice *dphy_;\n>  \tV4L2Subdevice *isp_;\n>  \tV4L2VideoDevice *video_;\n> +\tV4L2VideoDevice *stat_;\n> +\tV4L2VideoDevice *param_;\n> +\n> +\tBufferPool statPool_;\n> +\tBufferPool paramPool_;\n> +\n> +\tstd::queue<Buffer *> statBuffers_;\n> +\tstd::queue<Buffer *> paramBuffers_;\n\nWouldn't these be associated as buffers in a Request, and thus live on\nthat queue?\n\n\n>  \tCamera *activeCamera_;\n>  };\n>  \n> +int RkISP1CameraData::initCameraData()\n> +{\n> +\tipa_->updateSensor.connect(this,\n> +\t\t\t\t   &RkISP1CameraData::updateSensor);\n> +\tipa_->queueRequest.connect(this,\n> +\t\t\t\t   &RkISP1CameraData::queueRequestHardware);\n> +\treturn 0;\n> +}\n> +\n> +void RkISP1CameraData::updateSensor(V4L2ControlList controls)\n> +{\n> +\tsensor_->setControls(&controls);\n> +}\n> +\n> +void RkISP1CameraData::queueRequestHardware(const void *cookie)\n> +{\n> +\t/* Translate cookie to request. */\n> +\tRequest *request = reinterpret_cast<Request *>(const_cast<void *>(cookie));\n\nC++ casts are so ... pleasant ... :S\n\ndo you need the (const_cast<void *>.. ? Isn't cookie already a const void *?\n\n\n> +\tPipelineHandlerRkISP1 *pipe =\n> +\t\tstatic_cast<PipelineHandlerRkISP1 *>(pipe_);\n> +\tRkISP1RequestData *reqData =\n> +\t\tstatic_cast<RkISP1RequestData *>(request->data);\n> +\tBuffer *buffer = request->findBuffer(&stream_);\n> +\tint ret;\n> +\n> +\tret = pipe->param_->queueBuffer(reqData->param);\n> +\tif (ret < 0)\n> +\t\tLOG(RkISP1, Error) << \"Failed to queue paramaeters\";\n\ns/paramaeters/parameters/\n\n\n> +\n> +\tret = pipe->stat_->queueBuffer(reqData->stat);\n> +\tif (ret < 0)\n> +\t\tLOG(RkISP1, Error) << \"Failed to queue statistics\";\n\nI wonder if we should keep a counter somewhere of all 'failures' which\ncould not be returned...\n\nI guess it depends on whether we would somewhat successfully continue or\nnot without the statistics buffer queued.\n\n> +\n> +\tret = pipe->video_->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\tLOG(RkISP1, Error) << \"Failed to queue video\";\n> +}\n> +\n>  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n>  \t\t\t\t\t\t     RkISP1CameraData *data)\n>  \t: CameraConfiguration()\n> @@ -202,12 +266,14 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \n>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n>  \t: PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),\n> -\t  video_(nullptr)\n> +\t  video_(nullptr), stat_(nullptr), param_(nullptr)\n>  {\n>  }\n>  \n>  PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n>  {\n> +\tdelete param_;\n> +\tdelete stat_;\n>  \tdelete video_;\n>  \tdelete isp_;\n>  \tdelete dphy_;\n> @@ -317,6 +383,20 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> +\tV4L2DeviceFormat statFormat = {};\n> +\tstatFormat.fourcc = V4L2_META_FMT_RK_ISP1_STAT_3A;\n> +\n> +\tret = stat_->setFormat(&statFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n\nIt's fine to have it, I'm sure - but is this required?\n\nOr will a device which only supports one fourcc, not already be\nconfigured to use that fourcc?\n\nProbably good to be explicit even if it's not required though.\n\n\n> +\tV4L2DeviceFormat paramFormat = {};\n> +\tparamFormat.fourcc = V4L2_META_FMT_RK_ISP1_PARAMS;\n> +\n> +\tret = param_->setFormat(&paramFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n>  \tif (outputFormat.size != cfg.size ||\n>  \t    outputFormat.fourcc != cfg.pixelFormat) {\n>  \t\tLOG(RkISP1, Error)\n> @@ -333,30 +413,92 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n>  \t\t\t\t\t   const std::set<Stream *> &streams)\n>  {\n>  \tStream *stream = *streams.begin();\n> +\tint ret;\n>  \n>  \tif (stream->memoryType() == InternalMemory)\n> -\t\treturn video_->exportBuffers(&stream->bufferPool());\n> +\t\tret = video_->exportBuffers(&stream->bufferPool());\n>  \telse\n> -\t\treturn video_->importBuffers(&stream->bufferPool());\n> +\t\tret = video_->importBuffers(&stream->bufferPool());\n> +\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstatPool_.createBuffers(stream->configuration().bufferCount);\n> +\tret = stat_->exportBuffers(&statPool_);\n> +\tif (ret) {\n> +\t\tvideo_->releaseBuffers();\n\nCan we let these be handled by the destructor?\nor just call freeBuffers()?\n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tparamPool_.createBuffers(stream->configuration().bufferCount);\n> +\tret = param_->exportBuffers(&paramPool_);\n> +\tif (ret) {\n> +\t\tstat_->releaseBuffers();\n> +\t\tvideo_->releaseBuffers();\n\nAnd these?\n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < stream->configuration().bufferCount; i++) {\n> +\t\tstatBuffers_.push(new Buffer(i));\n> +\t\tparamBuffers_.push(new Buffer(i));\n> +\t}\n> +\n> +\treturn ret;\n>  }\n>  \n>  int PipelineHandlerRkISP1::freeBuffers(Camera *camera,\n>  \t\t\t\t       const std::set<Stream *> &streams)\n>  {\n> +\twhile (!paramBuffers_.empty())\n> +\t\tparamBuffers_.pop();\n> +\n> +\twhile (!statBuffers_.empty())\n> +\t\tstatBuffers_.pop();\n\nDoes the queue not have a .clear() operator?\nNo.\n\nHowever stackoverflow [0] informs me that you can just assign an empty\nqueue to empty the queue...\n\n  paramBuffers_ = {};\n  statBuffers_ = {};\n\nI don't know if that's better or not :D\n\n[0]\nhttps://stackoverflow.com/questions/3874624/why-stdqueue-doesnt-support-clear-function/3874743#3874743\n\nI wonder if we'll end up needing to use a deque anyway, so that we can\n'peek' into the queue to determine if we need to take any actions early\nbased on achieving per-frame-controls\n\n\n> +\n> +\tif (param_->releaseBuffers())\n> +\t\tLOG(RkISP1, Error) << \"Failed to release parameters buffers\";\n> +\n> +\tif (stat_->releaseBuffers())\n> +\t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n> +\n>  \tif (video_->releaseBuffers())\n> -\t\tLOG(RkISP1, Error) << \"Failed to release buffers\";\n> +\t\tLOG(RkISP1, Error) << \"Failed to release video buffers\";\n>  \n>  \treturn 0;\n>  }\n>  \n>  int PipelineHandlerRkISP1::start(Camera *camera)\n>  {\n> +\tRkISP1CameraData *data = cameraData(camera);\n>  \tint ret;\n>  \n> +\tret = data->ipa_->initSensor(data->sensor_->controls());\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = param_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(RkISP1, Error)\n> +\t\t\t<< \"Failed to start parameters \" << camera->name();\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = stat_->streamOn();\n> +\tif (ret) {\n> +\t\tparam_->streamOff();\n> +\t\tLOG(RkISP1, Error)\n> +\t\t\t<< \"Failed to start statisticis \" << camera->name();\n\ns/statisticis/statistics/\n\n> +\t\treturn ret;\n> +\t}\n> +\n>  \tret = video_->streamOn();\n> -\tif (ret)\n> +\tif (ret) {\n> +\t\tparam_->streamOff();\n> +\t\tstat_->streamOff();\n> +\n>  \t\tLOG(RkISP1, Error)\n>  \t\t\t<< \"Failed to start camera \" << camera->name();\n> +\t}\n>  \n>  \tactiveCamera_ = camera;\n>  \n> @@ -372,6 +514,16 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n>  \t\tLOG(RkISP1, Warning)\n>  \t\t\t<< \"Failed to stop camera \" << camera->name();\n>  \n> +\tret = stat_->streamOff();\n> +\tif (ret)\n> +\t\tLOG(RkISP1, Warning)\n> +\t\t\t<< \"Failed to stop statisticis \" << camera->name();\n\n\ns/statisticis/statistics/\n\n> +\n> +\tret = param_->streamOff();\n> +\tif (ret)\n> +\t\tLOG(RkISP1, Warning)\n> +\t\t\t<< \"Failed to stop parameters \" << camera->name();\n> +\n>  \tactiveCamera_ = nullptr;\n>  }\n>  \n> @@ -380,6 +532,16 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n>  \tRkISP1CameraData *data = cameraData(camera);\n>  \tStream *stream = &data->stream_;\n>  \n> +\tif (paramBuffers_.empty()) {\n> +\t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\tif (statBuffers_.empty()) {\n> +\t\tLOG(RkISP1, Error) << \"Statisitc buffer underrun\";\n\ns/Statisitc/Statistic/\n\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n>  \tBuffer *buffer = request->findBuffer(stream);\n>  \tif (!buffer) {\n>  \t\tLOG(RkISP1, Error)\n> @@ -387,12 +549,24 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>  \n> -\tint ret = video_->queueBuffer(buffer);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> +\tRkISP1RequestData *reqData = new RkISP1RequestData();\n> +\trequest->data = reqData;\n> +\treqData->param = paramBuffers_.front();\n> +\treqData->stat = statBuffers_.front();\n> +\n> +\tprepareInternalBuffer(reqData->param, request,\n> +\t\t\t      &paramPool_.buffers()[reqData->param->index()]);\n> +\tprepareInternalBuffer(reqData->stat, request,\n> +\t\t\t      &statPool_.buffers()[reqData->stat->index()]);\n> +\n> +\tparamBuffers_.pop();\n> +\tstatBuffers_.pop();\n>  \n>  \tPipelineHandler::queueRequest(camera, request);\n>  \n> +\tdata->ipa_->processRequest(request, request->controls(),\n> +\t\t\t\t   *reqData->param);\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -435,6 +609,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  \tstd::unique_ptr<RkISP1CameraData> data =\n>  \t\tutils::make_unique<RkISP1CameraData>(this);\n>  \n> +\tdata->controlInfo_.emplace(std::piecewise_construct,\n> +\t\t\t\t   std::forward_as_tuple(AeEnable),\n> +\t\t\t\t   std::forward_as_tuple(AeEnable, false, true));\n> +\n>  \tdata->sensor_ = new CameraSensor(sensor);\n>  \tret = data->sensor_->init();\n>  \tif (ret)\n> @@ -478,7 +656,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \tif (video_->open() < 0)\n>  \t\treturn false;\n>  \n> +\tstat_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1-statistics\");\n> +\tif (stat_->open() < 0)\n> +\t\treturn false;\n> +\n> +\tparam_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1-input-params\");\n> +\tif (param_->open() < 0)\n> +\t\treturn false;\n\nAh - good, I see these entities are already in the DeviceMatch.\n\n\n> +\n>  \tvideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> +\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> +\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>  \n>  \t/* Configure default links. */\n>  \tif (initLinks() < 0) {\n> @@ -504,13 +692,66 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>   * Buffer Handling\n>   */\n>  \n> +void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)\n> +{\n> +\tRkISP1RequestData *reqData =\n> +\t\tstatic_cast<RkISP1RequestData *>(request->data);\n> +\n> +\tif (reqData->param)\n> +\t\treturn;\n> +\n> +\tif (reqData->stat)\n> +\t\treturn;\n> +\n> +\tif (request->hasPendingBuffers())\n> +\t\treturn;\n\nHrm... for some reason I assumed these buffers would be tied into the\nhasPendingBuffers() calls ... but maybe that would require creating new\nstreams. And these are 'internal' streams ...\n\n> +\n> +\tdelete reqData;\n> +\trequest->data = nullptr;\n> +\n> +\tcompleteRequest(activeCamera_, request);\n> +}\n> +\n>  void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n>  {\n>  \tASSERT(activeCamera_);\n>  \tRequest *request = buffer->request();\n>  \n>  \tcompleteBuffer(activeCamera_, request, buffer);\n> -\tcompleteRequest(activeCamera_, request);\n> +\ttryCompleteRequest(request);\n\nI feel like somehow the param and stats buffers should be more directly\nassociated with the Request to facilitate the existing completeRequest()\nmechanism, which is what I thought it was for.\n\nIs there anything preventing that?\n\n\n> +}\n> +\n> +void PipelineHandlerRkISP1::statReady(Buffer *buffer)\n> +{\n> +\tASSERT(activeCamera_);\n> +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> +\tRequest *request = buffer->request();\n> +\tRkISP1RequestData *reqData =\n> +\t\tstatic_cast<RkISP1RequestData *>(request->data);\n> +\n> +\tdata->ipa_->updateStatistics(request, *buffer);\n> +\n> +\t/* TODO: Fetch libcamera status controls from IPA */\n\nstatus controls?\n\nDo we perhaps need to support two interactions with the IPA per request?\nOne at queue time, and one at complete?\n\n\n> +\n> +\treqData->stat = nullptr;\n> +\n> +\tstatBuffers_.push(buffer);\n> +\n> +\ttryCompleteRequest(request);\n> +}\n> +\n> +void PipelineHandlerRkISP1::paramReady(Buffer *buffer)\n> +{\n> +\tASSERT(activeCamera_);\n> +\tRequest *request = buffer->request();\n> +\tRkISP1RequestData *reqData =\n> +\t\tstatic_cast<RkISP1RequestData *>(request->data);\n> +\n> +\treqData->param = nullptr;\n> +\n> +\tparamBuffers_.push(buffer);\n> +\n> +\ttryCompleteRequest(request);\n>  }\n>  \n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n>","headers":{"Return-Path":"<kieran.bingham@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 5A3B360BE5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 29 Aug 2019 18:28:41 +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 B46CE741;\n\tThu, 29 Aug 2019 18:28:40 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567096120;\n\tbh=a8/GINQVUUQehTDdfwNPvDumL9wh3uSu1FcefKsytXk=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=onCXAbpwo2uRc54zP0mm+hgRjZO8yUah/ZDikX9NeJPGBCWpY3n07e3onOBC41jaU\n\tSGGCxiDIcjxsXTV+b+rMezwROpEIOE2XrWD1BJmLaYnHZW3kFDrBORSazmLOLGvUTG\n\tLcB7Fo9NxwgDF1T1m3p8WPUMYEAVmY8+aWy8cWKY=","Reply-To":"kieran.bingham@ideasonboard.com","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190828011710.32128-1-niklas.soderlund@ragnatech.se>\n\t<20190828011710.32128-14-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCJQQYAQoADwIbDAUCWcOUawUJ\n\tB4D+AgAKCRChHkZyEKRh/XJhEACr5iidt/0MZ0rWRMCbZFMWD7D2g6nZeOp+F2zY8CEUW+sd\n\tCDVd9BH9QX9KN5SZo6YtJzMzSzpcx45VwTvtQW0n/6Eujg9EUqblfU9xqvqDmbjEapr5d/OL\n\t21GTALb0owKhA5qDUGEcKGCphpQffKhTNo/BP99jvmJUj7IPSKH97qPypi8/ym8bAxB+uY31\n\tgHTMHf1jMJJ1pRo2tYYPeIIHGDqXBI4sp5GHHF+JcIhgR/e/A6w/dgzHYmQPl2ix5eZYEZbV\n\tTRP+gkX4NV8oHqa/lR+xPOlWElGB57viOSOoWriqxQbFy8XbG1GR8cWlkNtGBGVWaJaSoORP\n\tiowD7irXL91bCyFIqL+7BVk3Jy4uzP744PzE80KwxOp5SQAp9sPzFbgsJrLev90PZySjFHG0\n\twP144DK7nBjOj/J0g9OHVASP1JjK+nw7SDoKnETDIdRC0XmiHXk7TXzPdkvO0UkpHdEPjZUp\n\tWyuc0MqehjR/hTTPt4m/Y14XzEcy6JREIjOrFfUZVho2QpOdv9CNryGdieRTNjUtz463CIaZ\n\tdPBiw9mOMBoNffkn9FIoCjLnAaj9gUAnEHWBZOEviQ5NuyqpeP0YtzI4iaRbSUkYZHej99X3\n\tVmHrdLlMqd/ZgYYbPGSL4AN3FVACb5CxuxEHwo029VcE5U3CSjzqtCoX12tm7A==","Organization":"Ideas on Board","Message-ID":"<826201d3-8f85-7b3b-1c43-7e16c21602a0@ideasonboard.com>","Date":"Thu, 29 Aug 2019 17:28:38 +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":"<20190828011710.32128-14-niklas.soderlund@ragnatech.se>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 13/13] libcamera: pipeline: rkisp1:\n\tAttach to an IPA","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":"Thu, 29 Aug 2019 16:28:41 -0000"}},{"id":2561,"web_url":"https://patchwork.libcamera.org/comment/2561/","msgid":"<20190829224801.GI8479@bigcity.dyn.berto.se>","date":"2019-08-29T22:48:01","subject":"Re: [libcamera-devel] [PATCH 13/13] libcamera: pipeline: rkisp1:\n\tAttach to an IPA","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran,\n\nOn 2019-08-29 17:28:38 +0100, Kieran Bingham wrote:\n> Hi Niklas,\n> \n> The final piece of the puzzle :-D\n\nThanks for your final piece of feedback ;-)\n\n> \n> On 28/08/2019 02:17, Niklas Söderlund wrote:\n> > Add the plumbing to the pipeline handler to interact with an IPA module.\n> > To support this parameter and statistic buffers needs to be associated\n> > with every request queued. The parameters buffer needs to be passed to\n> > the IPA before any buffer in the request is queued to hardware and the\n> > statistics buffer needs to be passed to the IPA for inspection as soon\n> > as it's ready.\n> > \n> > This change makes the usage of an IPA module mandatory for the rkisp1\n> > pipeline.\n> > \n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 263 ++++++++++++++++++++++-\n> >  1 file changed, 252 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index de4ab523d0e4fe36..80d4832c49ebe78c 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -9,7 +9,7 @@\n> >  #include <array>\n> >  #include <iomanip>\n> >  #include <memory>\n> > -#include <vector>\n> > +#include <queue>\n> >  \n> >  #include <linux/media-bus-format.h>\n> >  \n> > @@ -34,7 +34,7 @@ class RkISP1CameraData : public CameraData\n> >  {\n> >  public:\n> >  \tRkISP1CameraData(PipelineHandler *pipe)\n> > -\t\t: CameraData(pipe), sensor_(nullptr)\n> > +\t\t: CameraData(pipe, 1, 1), sensor_(nullptr)\n> >  \t{\n> >  \t}\n> >  \n> > @@ -43,8 +43,21 @@ public:\n> >  \t\tdelete sensor_;\n> >  \t}\n> >  \n> > +\tint initCameraData() override;\n> > +\n> >  \tStream stream_;\n> >  \tCameraSensor *sensor_;\n> > +\n> > +private:\n> > +\tvoid updateSensor(V4L2ControlList controls);\n> > +\tvoid queueRequestHardware(const void *cookie);\n> > +};\n> > +\n> > +class RkISP1RequestData : public RequestData\n> > +{\n> > +public:\n> > +\tBuffer *stat;\n> > +\tBuffer *param;\n> >  };\n> >  \n> >  class RkISP1CameraConfiguration : public CameraConfiguration\n> > @@ -99,18 +112,69 @@ private:\n> >  \t\t\tPipelineHandler::cameraData(camera));\n> >  \t}\n> >  \n> > +\tfriend RkISP1CameraData;\n> \n> Is this necessary? Isn't almost everything in a CameraData class public?\n> If this is required - should members in CameraData be moved to\n> protected/private?\n\nIt's the other way around, this allows RkISP1CameraData to access the \nparam_, stat_ and video_ members of RkISP1CameraConfiguration.\n\nBut the question still holds, maybe there is little point in keeping \nthem private?\n\n> \n> > +\n> >  \tint initLinks();\n> >  \tint createCamera(MediaEntity *sensor);\n> > +\tvoid tryCompleteRequest(Request *request);\n> >  \tvoid bufferReady(Buffer *buffer);\n> > +\tvoid statReady(Buffer *buffer);\n> > +\tvoid paramReady(Buffer *buffer);\n> >  \n> >  \tMediaDevice *media_;\n> >  \tV4L2Subdevice *dphy_;\n> >  \tV4L2Subdevice *isp_;\n> >  \tV4L2VideoDevice *video_;\n> > +\tV4L2VideoDevice *stat_;\n> > +\tV4L2VideoDevice *param_;\n> > +\n> > +\tBufferPool statPool_;\n> > +\tBufferPool paramPool_;\n> > +\n> > +\tstd::queue<Buffer *> statBuffers_;\n> > +\tstd::queue<Buffer *> paramBuffers_;\n> \n> Wouldn't these be associated as buffers in a Request, and thus live on\n> that queue?\n\nThe life cycle is that the Buffer object's are create at camera start \nand put in these queues. Once a request is \"queued\" to the pipeline a \nbuffer is take of these queues and associated with the request. Before \nthe request is complete (and returned to the application) the internal \nbuffers are removed from the request and put back at the end of this \nqueue.\n\n> \n> \n> >  \tCamera *activeCamera_;\n> >  };\n> >  \n> > +int RkISP1CameraData::initCameraData()\n> > +{\n> > +\tipa_->updateSensor.connect(this,\n> > +\t\t\t\t   &RkISP1CameraData::updateSensor);\n> > +\tipa_->queueRequest.connect(this,\n> > +\t\t\t\t   &RkISP1CameraData::queueRequestHardware);\n> > +\treturn 0;\n> > +}\n> > +\n> > +void RkISP1CameraData::updateSensor(V4L2ControlList controls)\n> > +{\n> > +\tsensor_->setControls(&controls);\n> > +}\n> > +\n> > +void RkISP1CameraData::queueRequestHardware(const void *cookie)\n> > +{\n> > +\t/* Translate cookie to request. */\n> > +\tRequest *request = reinterpret_cast<Request *>(const_cast<void *>(cookie));\n> \n> C++ casts are so ... pleasant ... :S\n> \n> do you need the (const_cast<void *>.. ? Isn't cookie already a const void *?\n\nIt is, but I need it to lose to const, as i can't reinterpret_cast() \nfrom a const object o a none const object. Agree this looks dumb and \nthere might be a more clever way of doing it, but I can't find it :-(\n\n> \n> \n> > +\tPipelineHandlerRkISP1 *pipe =\n> > +\t\tstatic_cast<PipelineHandlerRkISP1 *>(pipe_);\n> > +\tRkISP1RequestData *reqData =\n> > +\t\tstatic_cast<RkISP1RequestData *>(request->data);\n> > +\tBuffer *buffer = request->findBuffer(&stream_);\n> > +\tint ret;\n> > +\n> > +\tret = pipe->param_->queueBuffer(reqData->param);\n> > +\tif (ret < 0)\n> > +\t\tLOG(RkISP1, Error) << \"Failed to queue paramaeters\";\n> \n> s/paramaeters/parameters/\n> \n> \n> > +\n> > +\tret = pipe->stat_->queueBuffer(reqData->stat);\n> > +\tif (ret < 0)\n> > +\t\tLOG(RkISP1, Error) << \"Failed to queue statistics\";\n> \n> I wonder if we should keep a counter somewhere of all 'failures' which\n> could not be returned...\n> \n> I guess it depends on whether we would somewhat successfully continue or\n> not without the statistics buffer queued.\n\nI don't think we would retain a good image quality if this fails, maybe \nthis should be made a Fatal error? It really should not happen.\n\n> \n> > +\n> > +\tret = pipe->video_->queueBuffer(buffer);\n> > +\tif (ret < 0)\n> > +\t\tLOG(RkISP1, Error) << \"Failed to queue video\";\n> > +}\n> > +\n> >  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> >  \t\t\t\t\t\t     RkISP1CameraData *data)\n> >  \t: CameraConfiguration()\n> > @@ -202,12 +266,14 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >  \n> >  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> >  \t: PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),\n> > -\t  video_(nullptr)\n> > +\t  video_(nullptr), stat_(nullptr), param_(nullptr)\n> >  {\n> >  }\n> >  \n> >  PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n> >  {\n> > +\tdelete param_;\n> > +\tdelete stat_;\n> >  \tdelete video_;\n> >  \tdelete isp_;\n> >  \tdelete dphy_;\n> > @@ -317,6 +383,20 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >  \n> > +\tV4L2DeviceFormat statFormat = {};\n> > +\tstatFormat.fourcc = V4L2_META_FMT_RK_ISP1_STAT_3A;\n> > +\n> > +\tret = stat_->setFormat(&statFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> \n> It's fine to have it, I'm sure - but is this required?\n> \n> Or will a device which only supports one fourcc, not already be\n> configured to use that fourcc?\n\nIt should.\n\n> \n> Probably good to be explicit even if it's not required though.\n\nI'm open to not check return value here but should we not then change \nthe return type of setFormat() to void?\n\n> \n> \n> > +\tV4L2DeviceFormat paramFormat = {};\n> > +\tparamFormat.fourcc = V4L2_META_FMT_RK_ISP1_PARAMS;\n> > +\n> > +\tret = param_->setFormat(&paramFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> >  \tif (outputFormat.size != cfg.size ||\n> >  \t    outputFormat.fourcc != cfg.pixelFormat) {\n> >  \t\tLOG(RkISP1, Error)\n> > @@ -333,30 +413,92 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n> >  \t\t\t\t\t   const std::set<Stream *> &streams)\n> >  {\n> >  \tStream *stream = *streams.begin();\n> > +\tint ret;\n> >  \n> >  \tif (stream->memoryType() == InternalMemory)\n> > -\t\treturn video_->exportBuffers(&stream->bufferPool());\n> > +\t\tret = video_->exportBuffers(&stream->bufferPool());\n> >  \telse\n> > -\t\treturn video_->importBuffers(&stream->bufferPool());\n> > +\t\tret = video_->importBuffers(&stream->bufferPool());\n> > +\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tstatPool_.createBuffers(stream->configuration().bufferCount);\n> > +\tret = stat_->exportBuffers(&statPool_);\n> > +\tif (ret) {\n> > +\t\tvideo_->releaseBuffers();\n> \n> Can we let these be handled by the destructor?\n> or just call freeBuffers()?\n> \n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tparamPool_.createBuffers(stream->configuration().bufferCount);\n> > +\tret = param_->exportBuffers(&paramPool_);\n> > +\tif (ret) {\n> > +\t\tstat_->releaseBuffers();\n> > +\t\tvideo_->releaseBuffers();\n> \n> And these?\n\nThe destructor is only called once the camera is destroyed right? This \nwill undo what happens at stream on, potentially allowing a second \nstream on to succeed.\n\n> \n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfor (unsigned int i = 0; i < stream->configuration().bufferCount; i++) {\n> > +\t\tstatBuffers_.push(new Buffer(i));\n> > +\t\tparamBuffers_.push(new Buffer(i));\n> > +\t}\n> > +\n> > +\treturn ret;\n> >  }\n> >  \n> >  int PipelineHandlerRkISP1::freeBuffers(Camera *camera,\n> >  \t\t\t\t       const std::set<Stream *> &streams)\n> >  {\n> > +\twhile (!paramBuffers_.empty())\n> > +\t\tparamBuffers_.pop();\n> > +\n> > +\twhile (!statBuffers_.empty())\n> > +\t\tstatBuffers_.pop();\n> \n> Does the queue not have a .clear() operator?\n> No.\n> \n> However stackoverflow [0] informs me that you can just assign an empty\n> queue to empty the queue...\n> \n>   paramBuffers_ = {};\n>   statBuffers_ = {};\n> \n> I don't know if that's better or not :D\n> \n> [0]\n> https://stackoverflow.com/questions/3874624/why-stdqueue-doesnt-support-clear-function/3874743#3874743\n> \n> I wonder if we'll end up needing to use a deque anyway, so that we can\n> 'peek' into the queue to determine if we need to take any actions early\n> based on achieving per-frame-controls\n\nI like this explicit popping better then assigning an empty queue. But I \nthink this is just a matter of taste.\n\n> \n> \n> > +\n> > +\tif (param_->releaseBuffers())\n> > +\t\tLOG(RkISP1, Error) << \"Failed to release parameters buffers\";\n> > +\n> > +\tif (stat_->releaseBuffers())\n> > +\t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n> > +\n> >  \tif (video_->releaseBuffers())\n> > -\t\tLOG(RkISP1, Error) << \"Failed to release buffers\";\n> > +\t\tLOG(RkISP1, Error) << \"Failed to release video buffers\";\n> >  \n> >  \treturn 0;\n> >  }\n> >  \n> >  int PipelineHandlerRkISP1::start(Camera *camera)\n> >  {\n> > +\tRkISP1CameraData *data = cameraData(camera);\n> >  \tint ret;\n> >  \n> > +\tret = data->ipa_->initSensor(data->sensor_->controls());\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = param_->streamOn();\n> > +\tif (ret) {\n> > +\t\tLOG(RkISP1, Error)\n> > +\t\t\t<< \"Failed to start parameters \" << camera->name();\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tret = stat_->streamOn();\n> > +\tif (ret) {\n> > +\t\tparam_->streamOff();\n> > +\t\tLOG(RkISP1, Error)\n> > +\t\t\t<< \"Failed to start statisticis \" << camera->name();\n> \n> s/statisticis/statistics/\n> \n> > +\t\treturn ret;\n> > +\t}\n> > +\n> >  \tret = video_->streamOn();\n> > -\tif (ret)\n> > +\tif (ret) {\n> > +\t\tparam_->streamOff();\n> > +\t\tstat_->streamOff();\n> > +\n> >  \t\tLOG(RkISP1, Error)\n> >  \t\t\t<< \"Failed to start camera \" << camera->name();\n> > +\t}\n> >  \n> >  \tactiveCamera_ = camera;\n> >  \n> > @@ -372,6 +514,16 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> >  \t\tLOG(RkISP1, Warning)\n> >  \t\t\t<< \"Failed to stop camera \" << camera->name();\n> >  \n> > +\tret = stat_->streamOff();\n> > +\tif (ret)\n> > +\t\tLOG(RkISP1, Warning)\n> > +\t\t\t<< \"Failed to stop statisticis \" << camera->name();\n> \n> \n> s/statisticis/statistics/\n> \n> > +\n> > +\tret = param_->streamOff();\n> > +\tif (ret)\n> > +\t\tLOG(RkISP1, Warning)\n> > +\t\t\t<< \"Failed to stop parameters \" << camera->name();\n> > +\n> >  \tactiveCamera_ = nullptr;\n> >  }\n> >  \n> > @@ -380,6 +532,16 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n> >  \tRkISP1CameraData *data = cameraData(camera);\n> >  \tStream *stream = &data->stream_;\n> >  \n> > +\tif (paramBuffers_.empty()) {\n> > +\t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\tif (statBuffers_.empty()) {\n> > +\t\tLOG(RkISP1, Error) << \"Statisitc buffer underrun\";\n> \n> s/Statisitc/Statistic/\n> \n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> >  \tBuffer *buffer = request->findBuffer(stream);\n> >  \tif (!buffer) {\n> >  \t\tLOG(RkISP1, Error)\n> > @@ -387,12 +549,24 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n> >  \t\treturn -ENOENT;\n> >  \t}\n> >  \n> > -\tint ret = video_->queueBuffer(buffer);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > +\tRkISP1RequestData *reqData = new RkISP1RequestData();\n> > +\trequest->data = reqData;\n> > +\treqData->param = paramBuffers_.front();\n> > +\treqData->stat = statBuffers_.front();\n> > +\n> > +\tprepareInternalBuffer(reqData->param, request,\n> > +\t\t\t      &paramPool_.buffers()[reqData->param->index()]);\n> > +\tprepareInternalBuffer(reqData->stat, request,\n> > +\t\t\t      &statPool_.buffers()[reqData->stat->index()]);\n> > +\n> > +\tparamBuffers_.pop();\n> > +\tstatBuffers_.pop();\n> >  \n> >  \tPipelineHandler::queueRequest(camera, request);\n> >  \n> > +\tdata->ipa_->processRequest(request, request->controls(),\n> > +\t\t\t\t   *reqData->param);\n> > +\n> >  \treturn 0;\n> >  }\n> >  \n> > @@ -435,6 +609,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >  \tstd::unique_ptr<RkISP1CameraData> data =\n> >  \t\tutils::make_unique<RkISP1CameraData>(this);\n> >  \n> > +\tdata->controlInfo_.emplace(std::piecewise_construct,\n> > +\t\t\t\t   std::forward_as_tuple(AeEnable),\n> > +\t\t\t\t   std::forward_as_tuple(AeEnable, false, true));\n> > +\n> >  \tdata->sensor_ = new CameraSensor(sensor);\n> >  \tret = data->sensor_->init();\n> >  \tif (ret)\n> > @@ -478,7 +656,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >  \tif (video_->open() < 0)\n> >  \t\treturn false;\n> >  \n> > +\tstat_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1-statistics\");\n> > +\tif (stat_->open() < 0)\n> > +\t\treturn false;\n> > +\n> > +\tparam_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1-input-params\");\n> > +\tif (param_->open() < 0)\n> > +\t\treturn false;\n> \n> Ah - good, I see these entities are already in the DeviceMatch.\n> \n> \n> > +\n> >  \tvideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > +\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > +\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> >  \n> >  \t/* Configure default links. */\n> >  \tif (initLinks() < 0) {\n> > @@ -504,13 +692,66 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >   * Buffer Handling\n> >   */\n> >  \n> > +void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)\n> > +{\n> > +\tRkISP1RequestData *reqData =\n> > +\t\tstatic_cast<RkISP1RequestData *>(request->data);\n> > +\n> > +\tif (reqData->param)\n> > +\t\treturn;\n> > +\n> > +\tif (reqData->stat)\n> > +\t\treturn;\n> > +\n> > +\tif (request->hasPendingBuffers())\n> > +\t\treturn;\n> \n> Hrm... for some reason I assumed these buffers would be tied into the\n> hasPendingBuffers() calls ... but maybe that would require creating new\n> streams. And these are 'internal' streams ...\n\nI do not view them as streams, but you are right maybe we should. But I \ndon't think adding them to hasPendingBuffers() logic is the right move, \nbut I might be wrong.\n\n> \n> > +\n> > +\tdelete reqData;\n> > +\trequest->data = nullptr;\n> > +\n> > +\tcompleteRequest(activeCamera_, request);\n> > +}\n> > +\n> >  void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> >  {\n> >  \tASSERT(activeCamera_);\n> >  \tRequest *request = buffer->request();\n> >  \n> >  \tcompleteBuffer(activeCamera_, request, buffer);\n> > -\tcompleteRequest(activeCamera_, request);\n> > +\ttryCompleteRequest(request);\n> \n> I feel like somehow the param and stats buffers should be more directly\n> associated with the Request to facilitate the existing completeRequest()\n> mechanism, which is what I thought it was for.\n> \n> Is there anything preventing that?\n\nI think tryCompleteRequest() to include more things than just buffers \n(completion of meta data, status control bellow) for example. I might be \nwrong but for now lets keep it in the pipeline handler and generalize \nlater once we have more IPA implementations.\n\n> \n> \n> > +}\n> > +\n> > +void PipelineHandlerRkISP1::statReady(Buffer *buffer)\n> > +{\n> > +\tASSERT(activeCamera_);\n> > +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> > +\tRequest *request = buffer->request();\n> > +\tRkISP1RequestData *reqData =\n> > +\t\tstatic_cast<RkISP1RequestData *>(request->data);\n> > +\n> > +\tdata->ipa_->updateStatistics(request, *buffer);\n> > +\n> > +\t/* TODO: Fetch libcamera status controls from IPA */\n> \n> status controls?\n> \n> Do we perhaps need to support two interactions with the IPA per request?\n> One at queue time, and one at complete?\n\nYes, I'm working on this on-top of this series. I think I'm going to go \nwith another signal to carry meta data out of the IPA after the ISP have \nfinished processing, but not sure yet.\n\n> \n> \n> > +\n> > +\treqData->stat = nullptr;\n> > +\n> > +\tstatBuffers_.push(buffer);\n> > +\n> > +\ttryCompleteRequest(request);\n> > +}\n> > +\n> > +void PipelineHandlerRkISP1::paramReady(Buffer *buffer)\n> > +{\n> > +\tASSERT(activeCamera_);\n> > +\tRequest *request = buffer->request();\n> > +\tRkISP1RequestData *reqData =\n> > +\t\tstatic_cast<RkISP1RequestData *>(request->data);\n> > +\n> > +\treqData->param = nullptr;\n> > +\n> > +\tparamBuffers_.push(buffer);\n> > +\n> > +\ttryCompleteRequest(request);\n> >  }\n> >  \n> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n> > \n> \n> -- \n> Regards\n> --\n> Kieran","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1AA960BE5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 30 Aug 2019 00:48:03 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id l11so3803448lfk.6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 29 Aug 2019 15:48:03 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\tt14sm109236lje.70.2019.08.29.15.48.01\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Aug 2019 15:48:02 -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=97dBzmBp1z3dforcH1OVPM1gfMjld+bsHoRNNyvnFQc=;\n\tb=HvXQM4cFD4pxwp5dXvJGZMpMnp6WX9MBG0Vbquu6SgKTWUEXIV2o96BPfxwXJ7zELq\n\tEykdT08l/UKBgl7jesDy0jm8Rho3rP2jbZFBXHSv1z1lSGh7glkgloz+JjMPt44jjpCr\n\tLiDIedOyv/WTp4HObcZd51zP505ovUjfScqLTqKUIlhAIH194jF69ljqY7q7ha/x9ICP\n\tBiNwf/gPSVPAw11bDybVP0gmgFfql5/B3LPtw4vKM3g98Xqic+p1mVCITcihqCvtXw6/\n\t/g9RxSvKMIDX3fdzI2uyoiacd7Q0JIlfDQVEbskvQcSCJlD8f7ay5It4iPP+lWcDCQUL\n\tFlKw==","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=97dBzmBp1z3dforcH1OVPM1gfMjld+bsHoRNNyvnFQc=;\n\tb=BHUxAZpwBaBKgajdm5PYEKmjUf7K9WhfFIkaTkJ+vmbpsd4DOa7/cgcw5bn9XeG0Ky\n\tn/XHY8vLxCaxifbUJ/NSfYo9Bjfms6tZn9UkCSZhYqTYrNE8m8+pH2oFDZfUSCrOwBLW\n\t3qfc808qzZU+cBhT+lQg+W1m8vmbyVLGeNhkUSXgg9+xbA1pA8XN5KNV0z6wWG2cr9FG\n\tGsBxMwazozjUSm3lcb+ujRzShP2s7utVXnxfvI5/WHPbF+3poyvi2OrRbfdE6HZpft5H\n\tgqpZJ4vYIU3eGHdmKmTChNfV3YLOd5pplGQ3KXL/fHEhUI+PV9GR5Tco66+0oc8rDbi6\n\teIJA==","X-Gm-Message-State":"APjAAAXjPpDHSjBpoqvT+xI4WOBEsBE0q1sgRoBh4dr04H/58A861C8I\n\t5CcxVOC3WMnQH+qYJ2kLXYGqLw==","X-Google-Smtp-Source":"APXvYqxEBYiX7R/ejRyWp7I9xP3SWT4yudWqf0JLisR/k52sQZgh3sqDOoqbnpolcwvvhhXZRCQhdg==","X-Received":"by 2002:a19:beca:: with SMTP id\n\to193mr7839123lff.137.1567118882984; \n\tThu, 29 Aug 2019 15:48:02 -0700 (PDT)","Date":"Fri, 30 Aug 2019 00:48:01 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829224801.GI8479@bigcity.dyn.berto.se>","References":"<20190828011710.32128-1-niklas.soderlund@ragnatech.se>\n\t<20190828011710.32128-14-niklas.soderlund@ragnatech.se>\n\t<826201d3-8f85-7b3b-1c43-7e16c21602a0@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<826201d3-8f85-7b3b-1c43-7e16c21602a0@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH 13/13] libcamera: pipeline: rkisp1:\n\tAttach to an IPA","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":"Thu, 29 Aug 2019 22:48:03 -0000"}}]