[{"id":1787,"web_url":"https://patchwork.libcamera.org/comment/1787/","msgid":"<20190607161943.GS7593@pendragon.ideasonboard.com>","date":"2019-06-07T16:19:43","subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jun 06, 2019 at 09:56:52PM +0100, Kieran Bingham wrote:\n> Pipeline handlers must implement functions to handle controls as part of\n> their interface.\n> \n> Extend the pipeline handler interface to declare this requirement and\n> implement basic control functionality in the currently supported\n> PipelineHandlers\n\nI don't think this should be exposed through the PipelineHandler API, it\nshould be handled inside each pipeline handler instead. You're calling\nthe readControls() and writeControls() methods of the pipeline handler\ninternally only in this patch, so it's effectively internal already.\n\n> ---\n> src/libcamera/include/pipeline_handler.h | 3 +\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++\n> src/libcamera/pipeline/raspberrypi.cpp | 108 ++++++++++++++++++-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++\n> src/libcamera/pipeline/uvcvideo.cpp | 127 ++++++++++++++++++++++-\n> src/libcamera/pipeline/vimc.cpp | 19 ++++\n> 6 files changed, 293 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 7da6df1ab2a0..4e306d964bff 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -72,6 +72,9 @@ public:\n> \tvirtual int start(Camera *camera) = 0;\n> \tvirtual void stop(Camera *camera);\n> \n> +\tvirtual int readControls(Camera *camera, Request *request) = 0;\n> +\tvirtual int writeControls(Camera *camera, Request *request) = 0;\n> +\n> \tvirtual int queueRequest(Camera *camera, Request *request);\n> \n> \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n\n[snip]\n\n> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp\n> index d6749eaae759..337554501c82 100644\n> --- a/src/libcamera/pipeline/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi.cpp\n> @@ -15,6 +15,7 @@\n> #include \"media_device.h\"\n> #include \"pipeline_handler.h\"\n> #include \"utils.h\"\n> +#include \"v4l2_controls.h\"\n> #include \"v4l2_device.h\"\n> \n> namespace libcamera {\n> @@ -77,6 +78,9 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n> \n> +\tint writeControls(Camera *camera, Request *request);\n> +\tint readControls(Camera *camera, Request *request);\n> +\n> \tint queueRequest(Camera *camera, Request *request) override;\n> \n> \tbool match(DeviceEnumerator *enumerator) override;\n> @@ -293,6 +297,94 @@ void PipelineHandlerRPi::stop(Camera *camera)\n> \tPipelineHandler::stop(camera);\n> }\n> \n> +int PipelineHandlerRPi::writeControls(Camera *camera, Request *request)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\n> +\tstd::vector controls;\n> +\n> +\tfor (Control c : request->controls()) {\n> +\t\tif (c.value().isWrite()) {\n\nWould it make sense to split the read and write sets at the Request\nlevel ?\n\n> +\t\t\tswitch (c.id()) {\n> +\t\t\tcase ManualGain:\n> +\t\t\t\tbreak;\n> +\t\t\tcase ManualExposure:\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(RPI, Error)\n> +\t\t\t\t\t<< \"Unhandled write control\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> +\tif (!controls.empty())\n> +\t\treturn data->unicam_->setControls(controls);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/* This is becoming horrible. How can we improve this\n> + * - Iterate reqeust controls to find controls to read\n> + * - Add those to a new list\n> + * - Query the device for that list to get controls\n> + * - iterate returned list\n> + * \t- For each control, search for corresponding control in request\n> + * \t- Set control value to return value.\n> + * \t- Return list.\n> + * \t- Check for any values that were not updated?\n> + */\n> +int PipelineHandlerRPi::readControls(Camera *camera, Request *request)\n> +{\n> +\tRPiCameraData *data = cameraData(camera);\n> +\tstd::vector controlIDs;\n> +\tstd::vector controls;\n> +\n> +\tfor (Control c : request->controls()) {\n> +\t\tif (c.value().isRead()) {\n> +\t\t\tLOG(RPI, Error) << \"Read Control: \" << c;\n> +\n> +\t\t\tswitch (c.id()) {\n> +\t\t\tcase ManualGain:\n> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n> +\t\t\t\tbreak;\n> +\t\t\tcase ManualExposure:\n> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(RPI, Error)\n> +\t\t\t\t\t<< \"Unhandled write control\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> +\tif (controlIDs.empty())\n> +\t\treturn 0;\n> +\n> +\tcontrols = data->unicam_->getControls(controlIDs);\n> +\tif (controls.empty())\n> +\t\treturn -ENODATA;\n> +\n> +\tfor (V4L2Control *ctrl : controls) {\n> +\t\tswitch(ctrl->id()) {\n> +\t\tcase V4L2_CID_ANALOGUE_GAIN:\n> +\t\t\t//Control gain(ManualGain); //= request->controls().find(ManualGain);\n> +\t\t\tauto it = request->controls().find(ManualGain);\n> +\t\t\tControl gain = *it;\n> +\n> +\t\t\t//V4L2IntControl *c = static_cast(ctrl);\n> +\t\t\tgain.value().set(0x88);\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n> {\n> \tRPiCameraData *data = cameraData(camera);\n> @@ -304,7 +396,15 @@ int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n> \t\treturn -ENOENT;\n> \t}\n> \n> -\tint ret = data->isp_.capture->queueBuffer(buffer);\n> +\t/*\n> +\t * Handle 'set' controls first\n> +\t * Todo: Validate ordering and timing.\n\nHandling timing will be interesting, as V4L2 controls are synchronous,\nwhile buffers are not. I expect we'll need to evaluate when the buffer\nwill be captured, and set a timer accordingly to set controls (taking\ninto account the latencies of the capture pipeline).\n\n> +\t */\n> +\tint ret = writeControls(camera, request);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tret = data->isp_.capture->queueBuffer(buffer);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> @@ -394,6 +494,12 @@ void RPiCameraData::ispCaptureReady(Buffer *buffer)\n> \tRequest *request = queuedRequests_.front();\n> \n> \tpipe_->completeBuffer(camera_, request, buffer);\n> +\n> +\tint ret = pipe_->readControls(camera_, request);\n> +\tif (ret < 0)\n> +\t\tLOG(RPI, Error)\n> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n\nShould this be reflected in the request status ?\n\n> +\n> \tpipe_->completeRequest(camera_, request);\n> }\n> \n\n[snip]\n\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index e2d02c0dafb8..216fbe237827 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -14,6 +14,7 @@\n> #include \"media_device.h\"\n> #include \"pipeline_handler.h\"\n> #include \"utils.h\"\n> +#include \"v4l2_controls.h\"\n> #include \"v4l2_device.h\"\n> \n> namespace libcamera {\n> @@ -73,6 +74,9 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n> \n> +\tint readControls(Camera *camera, Request *request) override;\n> +\tint writeControls(Camera *camera, Request *request) override;\n> +\n> \tint queueRequest(Camera *camera, Request *request) override;\n> \n> \tbool match(DeviceEnumerator *enumerator) override;\n> @@ -252,6 +256,117 @@ void PipelineHandlerUVC::stop(Camera *camera)\n> \tPipelineHandler::stop(camera);\n> }\n> \n> +int PipelineHandlerUVC::writeControls(Camera *camera, Request *request)\n> +{\n> +\tUVCCameraData *data = cameraData(camera);\n> +\n> +\tstd::vector controls;\n> +\n> +\tfor (Control c : request->controls()) {\n> +\t\tif (c.value().isWrite()) {\n> +\t\t\tswitch (c.id()) {\n> +\t\t\tcase ManualBrightness:\n> +\t\t\t\tcontrols.emplace_back(new V4L2IntControl(V4L2_CID_BRIGHTNESS, c.value().getInt()));\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tcase IpaAwbEnable:\n> +\t\t\tcase ManualGain:\n> +\t\t\tcase ManualExposure:\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(UVC, Error)\n> +\t\t\t\t\t<< \"Unhandled write control\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> +\tif (!controls.empty()) {\n> +\t\tint ret = data->video_->setControls(controls);\n> +\n> +\t\t/* It would be nice to avoid the need to manually delete\n> +\t\t * the controls */\n\nYes, this seems like a no-go to me.\n\n> +\t\tfor (V4L2Control *c : controls)\n> +\t\t\tdelete c;\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerUVC::readControls(Camera *camera, Request *request)\n> +{\n> +\tUVCCameraData *data = cameraData(camera);\n> +\tstd::vector controlIDs;\n> +\tstd::vector controls;\n> +\n> +\tfor (Control c : request->controls()) {\n> +\t\tif (c.value().isRead()) {\n> +\t\t\tLOG(UVC, Error) << \"Read Control: \" << c;\n> +\n> +\t\t\tswitch (c.id()) {\n> +\t\t\tcase ManualBrightness:\n> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_BRIGHTNESS);\n> +\t\t\t\tbreak;\n> +\t\t\tcase ManualGain:\n> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n> +\t\t\t\tbreak;\n> +\t\t\tcase ManualExposure:\n> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(UVC, Error)\n> +\t\t\t\t\t<< \"Unhandled write control\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> +\tif (controlIDs.empty())\n> +\t\treturn 0;\n> +\n> +\tcontrols = data->video_->getControls(controlIDs);\n> +\tif (controls.empty())\n> +\t\treturn -ENODATA;\n> +\n> +\tfor (V4L2Control *ctrl : controls) {\n> +\t\tswitch (ctrl->id()) {\n> +\t\tcase V4L2_CID_BRIGHTNESS: {\n> +\t\t\tControl bright = *request->controls().find(ManualBrightness);\n> +\t\t\t/* RFC:\n> +\t\t\t * If the iterator doesn't find the control ^\n> +\t\t\t * then it causes a segfault, so this is really nasty...\n> +\t\t\t * and where just a map might be better - (ornot?) as it\n> +\t\t\t * will create the object at that key.\n> +\t\t\t * Now , that *shouldn't* happen (we should only look\n> +\t\t\t * for controls that were given to us in this\n> +\t\t\t * function ... but\n\n\t\t\tauto iter = request->controls().find(ManualBrightness);\n\t\t\tif (iter == request->controls().end())\n\t\t\t\t...\n\n> +\t\t\t */\n> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n> +\t\t\tbright.value().set(vc->value());\n> +\n> +\t\t\tLOG(UVC, Debug) << \"Setting Brightness: \" << bright;\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase V4L2_CID_ANALOGUE_GAIN: {\n> +\t\t\tControl gain = *request->controls().find(ManualGain);\n> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n> +\t\t\tgain.value().set(vc->value());\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tdefault:\n> +\t\t\tLOG(UVC, Warning) << \"Unhandled V4L2 Control ID\";\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> @@ -263,7 +378,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> \t\treturn -ENOENT;\n> \t}\n> \n> -\tint ret = data->video_->queueBuffer(buffer);\n> +\tint ret = writeControls(camera, request);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tret = data->video_->queueBuffer(buffer);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> @@ -317,6 +436,12 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n> \tRequest *request = queuedRequests_.front();\n> \n> \tpipe_->completeBuffer(camera_, request, buffer);\n> +\n> +\tint ret = pipe_->readControls(camera_, request);\n> +\tif (ret < 0)\n> +\t\tLOG(UVC, Error)\n> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n> +\n> \tpipe_->completeRequest(camera_, request);\n> }\n> \n\n[snip]","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 979AE6A219\n\tfor ;\n\tFri, 7 Jun 2019 18:19:58 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:44f0:8500:ca05:8177:199c:fed4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 22DDF334;\n\tFri, 7 Jun 2019 18:19:58 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559924398;\n\tbh=DRpssyYnDq/D4m0N9yfuzzG8kCu5qNDtdDFkR1h7HJw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WjSgwuoDnkVQdiWN0Z5nG1YXqg0OfFkkCQBcwFT7YGM/RPkPnNiBjIM7KX+W8fgsb\n\txROHa+6JZwAtFSW/wmV+7pHYITHv/C4oi3+DMtWvCxbOx/r219H2E37k/Z8hk8tm2n\n\tqPAD269GILNfqz3jeyihif5IFYsCZ6235BfVBUyE=","Date":"Fri, 7 Jun 2019 19:19:43 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190607161943.GS7593@pendragon.ideasonboard.com>","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190606205654.9311-4-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","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, 07 Jun 2019 16:19:58 -0000"}},{"id":1789,"web_url":"https://patchwork.libcamera.org/comment/1789/","msgid":"<65187dec-3d82-403f-b04f-f6b519e95c1d@ideasonboard.com>","date":"2019-06-07T21:35:15","subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for your comments, they are certainly helpful for sorting out\nsome of the design issues.\n\nOn 07/06/2019 17:19, Laurent Pinchart wrote:\n> On Thu, Jun 06, 2019 at 09:56:52PM +0100, Kieran Bingham wrote:\n>> Pipeline handlers must implement functions to handle controls as part of\n>> their interface.\n>>\n>> Extend the pipeline handler interface to declare this requirement and\n>> implement basic control functionality in the currently supported\n>> PipelineHandlers\n> \n> I don't think this should be exposed through the PipelineHandler API, it\n> should be handled inside each pipeline handler instead. You're calling\n> the readControls() and writeControls() methods of the pipeline handler\n> internally only in this patch, so it's effectively internal already.\n\nI had visions of the 'call' to read/write the controls being in a common\npoint, but as the PipelineHandlers determine when the request completes,\nand the internal timings - I think you're right. There is no need to\nenforce these functions on the handler API.\n\nIt's just up to a pipeline handler to decide how to deal with controls.\n\n\n>> ---\n>> src/libcamera/include/pipeline_handler.h | 3 +\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++\n>> src/libcamera/pipeline/raspberrypi.cpp | 108 ++++++++++++++++++-\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++\n>> src/libcamera/pipeline/uvcvideo.cpp | 127 ++++++++++++++++++++++-\n>> src/libcamera/pipeline/vimc.cpp | 19 ++++\n>> 6 files changed, 293 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n>> index 7da6df1ab2a0..4e306d964bff 100644\n>> --- a/src/libcamera/include/pipeline_handler.h\n>> +++ b/src/libcamera/include/pipeline_handler.h\n>> @@ -72,6 +72,9 @@ public:\n>> \tvirtual int start(Camera *camera) = 0;\n>> \tvirtual void stop(Camera *camera);\n>> \n>> +\tvirtual int readControls(Camera *camera, Request *request) = 0;\n>> +\tvirtual int writeControls(Camera *camera, Request *request) = 0;\n>> +\n>> \tvirtual int queueRequest(Camera *camera, Request *request);\n>> \n>> \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> \n> [snip]\n> \n>> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp\n>> index d6749eaae759..337554501c82 100644\n>> --- a/src/libcamera/pipeline/raspberrypi.cpp\n>> +++ b/src/libcamera/pipeline/raspberrypi.cpp\n>> @@ -15,6 +15,7 @@\n>> #include \"media_device.h\"\n>> #include \"pipeline_handler.h\"\n>> #include \"utils.h\"\n>> +#include \"v4l2_controls.h\"\n>> #include \"v4l2_device.h\"\n>> \n>> namespace libcamera {\n>> @@ -77,6 +78,9 @@ public:\n>> \tint start(Camera *camera) override;\n>> \tvoid stop(Camera *camera) override;\n>> \n>> +\tint writeControls(Camera *camera, Request *request);\n>> +\tint readControls(Camera *camera, Request *request);\n>> +\n>> \tint queueRequest(Camera *camera, Request *request) override;\n>> \n>> \tbool match(DeviceEnumerator *enumerator) override;\n>> @@ -293,6 +297,94 @@ void PipelineHandlerRPi::stop(Camera *camera)\n>> \tPipelineHandler::stop(camera);\n>> }\n>> \n>> +int PipelineHandlerRPi::writeControls(Camera *camera, Request *request)\n>> +{\n>> +\tRPiCameraData *data = cameraData(camera);\n>> +\n>> +\tstd::vector controls;\n>> +\n>> +\tfor (Control c : request->controls()) {\n>> +\t\tif (c.value().isWrite()) {\n> \n> Would it make sense to split the read and write sets at the Request\n> level ?\n\n\nI'd love to be able to do a python generator type syntax here, and\n'yield' to cheaply iterate the controls handling ones that are appropriate.\n\nThis might be possible by writing our own iterators, but I'm not sure\nhow elegant that would be.\n\n\nIf you are imagining that every request defines every control, how would\nyou anticipate then splitting up the distinction at the request level?\nWith multiple sets?\n\n\nWhat values would you expect to be present in the reqeust when it is\ncreated with preset controls?\n\n - All values at default?\n - All values at current / cached values?\n - Not set at all... perhaps in the request in an 'unset' state.?\n - Some other option?\n\n\n\n\n\n>> +\t\t\tswitch (c.id()) {\n>> +\t\t\tcase ManualGain:\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase ManualExposure:\n>> +\t\t\t\tbreak;\n>> +\t\t\tdefault:\n>> +\t\t\t\tLOG(RPI, Error)\n>> +\t\t\t\t\t<< \"Unhandled write control\";\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t}\n>> +\n>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n>> +\tif (!controls.empty())\n>> +\t\treturn data->unicam_->setControls(controls);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/* This is becoming horrible. How can we improve this\n>> + * - Iterate reqeust controls to find controls to read\n>> + * - Add those to a new list\n>> + * - Query the device for that list to get controls\n>> + * - iterate returned list\n>> + * \t- For each control, search for corresponding control in request\n>> + * \t- Set control value to return value.\n>> + * \t- Return list.\n>> + * \t- Check for any values that were not updated?\n>> + */\n>> +int PipelineHandlerRPi::readControls(Camera *camera, Request *request)\n>> +{\n>> +\tRPiCameraData *data = cameraData(camera);\n>> +\tstd::vector controlIDs;\n>> +\tstd::vector controls;\n>> +\n>> +\tfor (Control c : request->controls()) {\n>> +\t\tif (c.value().isRead()) {\n>> +\t\t\tLOG(RPI, Error) << \"Read Control: \" << c;\n>> +\n>> +\t\t\tswitch (c.id()) {\n>> +\t\t\tcase ManualGain:\n>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase ManualExposure:\n>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n>> +\t\t\t\tbreak;\n>> +\t\t\tdefault:\n>> +\t\t\t\tLOG(RPI, Error)\n>> +\t\t\t\t\t<< \"Unhandled write control\";\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t}\n>> +\n>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n>> +\tif (controlIDs.empty())\n>> +\t\treturn 0;\n>> +\n>> +\tcontrols = data->unicam_->getControls(controlIDs);\n>> +\tif (controls.empty())\n>> +\t\treturn -ENODATA;\n>> +\n>> +\tfor (V4L2Control *ctrl : controls) {\n>> +\t\tswitch(ctrl->id()) {\n>> +\t\tcase V4L2_CID_ANALOGUE_GAIN:\n>> +\t\t\t//Control gain(ManualGain); //= request->controls().find(ManualGain);\n>> +\t\t\tauto it = request->controls().find(ManualGain);\n>> +\t\t\tControl gain = *it;\n>> +\n>> +\t\t\t//V4L2IntControl *c = static_cast(ctrl);\n>> +\t\t\tgain.value().set(0x88);\n>> +\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n>> {\n>> \tRPiCameraData *data = cameraData(camera);\n>> @@ -304,7 +396,15 @@ int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n>> \t\treturn -ENOENT;\n>> \t}\n>> \n>> -\tint ret = data->isp_.capture->queueBuffer(buffer);\n>> +\t/*\n>> +\t * Handle 'set' controls first\n>> +\t * Todo: Validate ordering and timing.\n> \n> Handling timing will be interesting, as V4L2 controls are synchronous,\n> while buffers are not. I expect we'll need to evaluate when the buffer\n> will be captured, and set a timer accordingly to set controls (taking\n> into account the latencies of the capture pipeline).\n\nIndeed - making sure we set the value for that buffer is going to be\ninteresting. It might be some 'guess work' :-( - but we should know how\nmany buffers are queued ahead of us, and we can store the completion\ntime of each previously completed buffer so we should be able to\nestimate when things happen in some form, and calculate our expected\n'latency' for any given request when it is queued.\n\nHrm ... that sounds like it might actually be quite fun (but probably\nalso painful somehow) to implement ...\n\n\n>> +\t */\n>> +\tint ret = writeControls(camera, request);\n>> +\tif (ret < 0)\n>> +\t\treturn ret;\n>> +\n>> +\tret = data->isp_.capture->queueBuffer(buffer);\n>> \tif (ret < 0)\n>> \t\treturn ret;\n>> \n>> @@ -394,6 +494,12 @@ void RPiCameraData::ispCaptureReady(Buffer *buffer)\n>> \tRequest *request = queuedRequests_.front();\n>> \n>> \tpipe_->completeBuffer(camera_, request, buffer);\n>> +\n>> +\tint ret = pipe_->readControls(camera_, request);\n>> +\tif (ret < 0)\n>> +\t\tLOG(RPI, Error)\n>> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n> \n> Should this be reflected in the request status ?\n\nAha, yes of course. That is indeed how the status could be returned!\n\nCan we end up in a situation were we have multiple error status' to\nreturn in a request? Perhaps we might have to store a vector of Error\nstatus'?\n\n> \n>> +\n>> \tpipe_->completeRequest(camera_, request);\n>> }\n>> \n> \n> [snip]\n> \n>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n>> index e2d02c0dafb8..216fbe237827 100644\n>> --- a/src/libcamera/pipeline/uvcvideo.cpp\n>> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n>> @@ -14,6 +14,7 @@\n>> #include \"media_device.h\"\n>> #include \"pipeline_handler.h\"\n>> #include \"utils.h\"\n>> +#include \"v4l2_controls.h\"\n>> #include \"v4l2_device.h\"\n>> \n>> namespace libcamera {\n>> @@ -73,6 +74,9 @@ public:\n>> \tint start(Camera *camera) override;\n>> \tvoid stop(Camera *camera) override;\n>> \n>> +\tint readControls(Camera *camera, Request *request) override;\n>> +\tint writeControls(Camera *camera, Request *request) override;\n>> +\n>> \tint queueRequest(Camera *camera, Request *request) override;\n>> \n>> \tbool match(DeviceEnumerator *enumerator) override;\n>> @@ -252,6 +256,117 @@ void PipelineHandlerUVC::stop(Camera *camera)\n>> \tPipelineHandler::stop(camera);\n>> }\n>> \n>> +int PipelineHandlerUVC::writeControls(Camera *camera, Request *request)\n>> +{\n>> +\tUVCCameraData *data = cameraData(camera);\n>> +\n>> +\tstd::vector controls;\n>> +\n>> +\tfor (Control c : request->controls()) {\n>> +\t\tif (c.value().isWrite()) {\n>> +\t\t\tswitch (c.id()) {\n>> +\t\t\tcase ManualBrightness:\n>> +\t\t\t\tcontrols.emplace_back(new V4L2IntControl(V4L2_CID_BRIGHTNESS, c.value().getInt()));\n>> +\t\t\t\tbreak;\n>> +\n>> +\t\t\tcase IpaAwbEnable:\n>> +\t\t\tcase ManualGain:\n>> +\t\t\tcase ManualExposure:\n>> +\t\t\t\tbreak;\n>> +\t\t\tdefault:\n>> +\t\t\t\tLOG(UVC, Error)\n>> +\t\t\t\t\t<< \"Unhandled write control\";\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t}\n>> +\n>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n>> +\tif (!controls.empty()) {\n>> +\t\tint ret = data->video_->setControls(controls);\n>> +\n>> +\t\t/* It would be nice to avoid the need to manually delete\n>> +\t\t * the controls */\n> \n> Yes, this seems like a no-go to me.\n\nI think we can adapt the V4L2Control interface to accept a set or map as\nwell, and potentially even (where relevant) put in a reference to the\nControlValue supplied to prevent excess copying of what will essentially\nbe the same object type to update.\n\n\n\n> \n>> +\t\tfor (V4L2Control *c : controls)\n>> +\t\t\tdelete c;\n>> +\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +int PipelineHandlerUVC::readControls(Camera *camera, Request *request)\n>> +{\n>> +\tUVCCameraData *data = cameraData(camera);\n>> +\tstd::vector controlIDs;\n>> +\tstd::vector controls;\n>> +\n>> +\tfor (Control c : request->controls()) {\n>> +\t\tif (c.value().isRead()) {\n>> +\t\t\tLOG(UVC, Error) << \"Read Control: \" << c;\n>> +\n>> +\t\t\tswitch (c.id()) {\n>> +\t\t\tcase ManualBrightness:\n>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_BRIGHTNESS);\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase ManualGain:\n>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase ManualExposure:\n>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n>> +\t\t\t\tbreak;\n>> +\t\t\tdefault:\n>> +\t\t\t\tLOG(UVC, Error)\n>> +\t\t\t\t\t<< \"Unhandled write control\";\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t}\n>> +\n>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n>> +\tif (controlIDs.empty())\n>> +\t\treturn 0;\n>> +\n>> +\tcontrols = data->video_->getControls(controlIDs);\n>> +\tif (controls.empty())\n>> +\t\treturn -ENODATA;\n>> +\n>> +\tfor (V4L2Control *ctrl : controls) {\n>> +\t\tswitch (ctrl->id()) {\n>> +\t\tcase V4L2_CID_BRIGHTNESS: {\n>> +\t\t\tControl bright = *request->controls().find(ManualBrightness);\n>> +\t\t\t/* RFC:\n>> +\t\t\t * If the iterator doesn't find the control ^\n>> +\t\t\t * then it causes a segfault, so this is really nasty...\n>> +\t\t\t * and where just a map might be better - (ornot?) as it\n>> +\t\t\t * will create the object at that key.\n>> +\t\t\t * Now , that *shouldn't* happen (we should only look\n>> +\t\t\t * for controls that were given to us in this\n>> +\t\t\t * function ... but\n> \n> \t\t\tauto iter = request->controls().find(ManualBrightness);\n> \t\t\tif (iter == request->controls().end())\n> \t\t\t\t...\n\nIt shouldn't ever occur, as we would only look for a value we have\nrequested... but that's what made me interested in the difference for\nusing a map, as it would then create the ControlValue for use.\n\nBut then also if we are always going to have all controls in the\nrequest, then failing to find a control would become an assertion failure.\n\n\n> \n>> +\t\t\t */\n>> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n>> +\t\t\tbright.value().set(vc->value());\n>> +\n>> +\t\t\tLOG(UVC, Debug) << \"Setting Brightness: \" << bright;\n>> +\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t\tcase V4L2_CID_ANALOGUE_GAIN: {\n>> +\t\t\tControl gain = *request->controls().find(ManualGain);\n>> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n>> +\t\t\tgain.value().set(vc->value());\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t\tdefault:\n>> +\t\t\tLOG(UVC, Warning) << \"Unhandled V4L2 Control ID\";\n>> +\t\t}\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n>> {\n>> \tUVCCameraData *data = cameraData(camera);\n>> @@ -263,7 +378,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n>> \t\treturn -ENOENT;\n>> \t}\n>> \n>> -\tint ret = data->video_->queueBuffer(buffer);\n>> +\tint ret = writeControls(camera, request);\n>> +\tif (ret < 0)\n>> +\t\treturn ret;\n>> +\n>> +\tret = data->video_->queueBuffer(buffer);\n>> \tif (ret < 0)\n>> \t\treturn ret;\n>> \n>> @@ -317,6 +436,12 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n>> \tRequest *request = queuedRequests_.front();\n>> \n>> \tpipe_->completeBuffer(camera_, request, buffer);\n>> +\n>> +\tint ret = pipe_->readControls(camera_, request);\n>> +\tif (ret < 0)\n>> +\t\tLOG(UVC, Error)\n>> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n>> +\n>> \tpipe_->completeRequest(camera_, request);\n>> }\n>> \n> \n> [snip]\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 5ADB26A4FA\n\tfor ;\n\tFri, 7 Jun 2019 23:35:19 +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 ADA00334;\n\tFri, 7 Jun 2019 23:35:18 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559943318;\n\tbh=eZxtGnjB0w+/0Kj3m55sLoEMweQSQ4mgyqjjU3eX+E4=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=DiWB2BGXMmhmI7HnP9d8cWMeJbwh34dkyjPLqT9ZCf7O+fq0OWt1AKcO5ZHawbKwB\n\tNh+ZckQI3XX5OlL2pFAsCsL2Ze8NTVRz04886oR0BGfFX5soWb5qqfzzETebnP9OXd\n\t8uP5rboqVx3MP/DKl+7cF3agnMnI2/A7f7lM0hxM=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-4-kieran.bingham@ideasonboard.com>\n\t<20190607161943.GS7593@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":"<65187dec-3d82-403f-b04f-f6b519e95c1d@ideasonboard.com>","Date":"Fri, 7 Jun 2019 22:35:15 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190607161943.GS7593@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","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, 07 Jun 2019 21:35:19 -0000"}},{"id":1842,"web_url":"https://patchwork.libcamera.org/comment/1842/","msgid":"<20190611121330.GH5016@pendragon.ideasonboard.com>","date":"2019-06-11T12:13:30","subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","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, Jun 07, 2019 at 10:35:15PM +0100, Kieran Bingham wrote:\n> Hi Laurent,\n> \n> Thank you for your comments, they are certainly helpful for sorting out\n> some of the design issues.\n> \n> On 07/06/2019 17:19, Laurent Pinchart wrote:\n> > On Thu, Jun 06, 2019 at 09:56:52PM +0100, Kieran Bingham wrote:\n> >> Pipeline handlers must implement functions to handle controls as part of\n> >> their interface.\n> >>\n> >> Extend the pipeline handler interface to declare this requirement and\n> >> implement basic control functionality in the currently supported\n> >> PipelineHandlers\n> > \n> > I don't think this should be exposed through the PipelineHandler API, it\n> > should be handled inside each pipeline handler instead. You're calling\n> > the readControls() and writeControls() methods of the pipeline handler\n> > internally only in this patch, so it's effectively internal already.\n> \n> I had visions of the 'call' to read/write the controls being in a common\n> point, but as the PipelineHandlers determine when the request completes,\n> and the internal timings - I think you're right. There is no need to\n> enforce these functions on the handler API.\n> \n> It's just up to a pipeline handler to decide how to deal with controls.\n> \n> >> ---\n> >> src/libcamera/include/pipeline_handler.h | 3 +\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++\n> >> src/libcamera/pipeline/raspberrypi.cpp | 108 ++++++++++++++++++-\n> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++\n> >> src/libcamera/pipeline/uvcvideo.cpp | 127 ++++++++++++++++++++++-\n> >> src/libcamera/pipeline/vimc.cpp | 19 ++++\n> >> 6 files changed, 293 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> >> index 7da6df1ab2a0..4e306d964bff 100644\n> >> --- a/src/libcamera/include/pipeline_handler.h\n> >> +++ b/src/libcamera/include/pipeline_handler.h\n> >> @@ -72,6 +72,9 @@ public:\n> >> \tvirtual int start(Camera *camera) = 0;\n> >> \tvirtual void stop(Camera *camera);\n> >> \n> >> +\tvirtual int readControls(Camera *camera, Request *request) = 0;\n> >> +\tvirtual int writeControls(Camera *camera, Request *request) = 0;\n> >> +\n> >> \tvirtual int queueRequest(Camera *camera, Request *request);\n> >> \n> >> \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> > \n> > [snip]\n> > \n> >> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp\n> >> index d6749eaae759..337554501c82 100644\n> >> --- a/src/libcamera/pipeline/raspberrypi.cpp\n> >> +++ b/src/libcamera/pipeline/raspberrypi.cpp\n> >> @@ -15,6 +15,7 @@\n> >> #include \"media_device.h\"\n> >> #include \"pipeline_handler.h\"\n> >> #include \"utils.h\"\n> >> +#include \"v4l2_controls.h\"\n> >> #include \"v4l2_device.h\"\n> >> \n> >> namespace libcamera {\n> >> @@ -77,6 +78,9 @@ public:\n> >> \tint start(Camera *camera) override;\n> >> \tvoid stop(Camera *camera) override;\n> >> \n> >> +\tint writeControls(Camera *camera, Request *request);\n> >> +\tint readControls(Camera *camera, Request *request);\n> >> +\n> >> \tint queueRequest(Camera *camera, Request *request) override;\n> >> \n> >> \tbool match(DeviceEnumerator *enumerator) override;\n> >> @@ -293,6 +297,94 @@ void PipelineHandlerRPi::stop(Camera *camera)\n> >> \tPipelineHandler::stop(camera);\n> >> }\n> >> \n> >> +int PipelineHandlerRPi::writeControls(Camera *camera, Request *request)\n> >> +{\n> >> +\tRPiCameraData *data = cameraData(camera);\n> >> +\n> >> +\tstd::vector controls;\n> >> +\n> >> +\tfor (Control c : request->controls()) {\n> >> +\t\tif (c.value().isWrite()) {\n> > \n> > Would it make sense to split the read and write sets at the Request\n> > level ?\n> \n> I'd love to be able to do a python generator type syntax here, and\n> 'yield' to cheaply iterate the controls handling ones that are appropriate.\n> \n> This might be possible by writing our own iterators, but I'm not sure\n> how elegant that would be.\n> \n> If you are imagining that every request defines every control, how would\n> you anticipate then splitting up the distinction at the request level?\n> With multiple sets?\n\nYes, a set of controls, and a set of metadata (I'm more and more\nthinking that what we called read controls so far should be renamed\nmetadata).\n\n> What values would you expect to be present in the reqeust when it is\n> created with preset controls?\n> \n> - All values at default?\n> - All values at current / cached values?\n> - Not set at all... perhaps in the request in an 'unset' state.?\n> - Some other option?\n\nI don't expect requests to be created with \"preset controls\". Requests\nshould be created empty, and populate it by applications. This could be\ndone manually, or through a preset provided by libcamera. I would expect\napplications to create multiple instances of lists of controls, populate\nthem with the help of preset and manual settings, and then use them at\nruntime to populate requests.\n\n> >> +\t\t\tswitch (c.id()) {\n> >> +\t\t\tcase ManualGain:\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tcase ManualExposure:\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tdefault:\n> >> +\t\t\t\tLOG(RPI, Error)\n> >> +\t\t\t\t\t<< \"Unhandled write control\";\n> >> +\t\t\t\tbreak;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> >> +\tif (!controls.empty())\n> >> +\t\treturn data->unicam_->setControls(controls);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +/* This is becoming horrible. How can we improve this\n> >> + * - Iterate reqeust controls to find controls to read\n> >> + * - Add those to a new list\n> >> + * - Query the device for that list to get controls\n> >> + * - iterate returned list\n> >> + * \t- For each control, search for corresponding control in request\n> >> + * \t- Set control value to return value.\n> >> + * \t- Return list.\n> >> + * \t- Check for any values that were not updated?\n> >> + */\n> >> +int PipelineHandlerRPi::readControls(Camera *camera, Request *request)\n> >> +{\n> >> +\tRPiCameraData *data = cameraData(camera);\n> >> +\tstd::vector controlIDs;\n> >> +\tstd::vector controls;\n> >> +\n> >> +\tfor (Control c : request->controls()) {\n> >> +\t\tif (c.value().isRead()) {\n> >> +\t\t\tLOG(RPI, Error) << \"Read Control: \" << c;\n> >> +\n> >> +\t\t\tswitch (c.id()) {\n> >> +\t\t\tcase ManualGain:\n> >> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tcase ManualExposure:\n> >> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tdefault:\n> >> +\t\t\t\tLOG(RPI, Error)\n> >> +\t\t\t\t\t<< \"Unhandled write control\";\n> >> +\t\t\t\tbreak;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> >> +\tif (controlIDs.empty())\n> >> +\t\treturn 0;\n> >> +\n> >> +\tcontrols = data->unicam_->getControls(controlIDs);\n> >> +\tif (controls.empty())\n> >> +\t\treturn -ENODATA;\n> >> +\n> >> +\tfor (V4L2Control *ctrl : controls) {\n> >> +\t\tswitch(ctrl->id()) {\n> >> +\t\tcase V4L2_CID_ANALOGUE_GAIN:\n> >> +\t\t\t//Control gain(ManualGain); //= request->controls().find(ManualGain);\n> >> +\t\t\tauto it = request->controls().find(ManualGain);\n> >> +\t\t\tControl gain = *it;\n> >> +\n> >> +\t\t\t//V4L2IntControl *c = static_cast(ctrl);\n> >> +\t\t\tgain.value().set(0x88);\n> >> +\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n> >> {\n> >> \tRPiCameraData *data = cameraData(camera);\n> >> @@ -304,7 +396,15 @@ int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n> >> \t\treturn -ENOENT;\n> >> \t}\n> >> \n> >> -\tint ret = data->isp_.capture->queueBuffer(buffer);\n> >> +\t/*\n> >> +\t * Handle 'set' controls first\n> >> +\t * Todo: Validate ordering and timing.\n> > \n> > Handling timing will be interesting, as V4L2 controls are synchronous,\n> > while buffers are not. I expect we'll need to evaluate when the buffer\n> > will be captured, and set a timer accordingly to set controls (taking\n> > into account the latencies of the capture pipeline).\n> \n> Indeed - making sure we set the value for that buffer is going to be\n> interesting. It might be some 'guess work' :-( - but we should know how\n> many buffers are queued ahead of us, and we can store the completion\n> time of each previously completed buffer so we should be able to\n> estimate when things happen in some form, and calculate our expected\n> 'latency' for any given request when it is queued.\n> \n> Hrm ... that sounds like it might actually be quite fun (but probably\n> also painful somehow) to implement ...\n\nFun and painful, yes :-) I hope we'll be able to factor some of that\ncode to helpers at a later point.\n\n> >> +\t */\n> >> +\tint ret = writeControls(camera, request);\n> >> +\tif (ret < 0)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = data->isp_.capture->queueBuffer(buffer);\n> >> \tif (ret < 0)\n> >> \t\treturn ret;\n> >> \n> >> @@ -394,6 +494,12 @@ void RPiCameraData::ispCaptureReady(Buffer *buffer)\n> >> \tRequest *request = queuedRequests_.front();\n> >> \n> >> \tpipe_->completeBuffer(camera_, request, buffer);\n> >> +\n> >> +\tint ret = pipe_->readControls(camera_, request);\n> >> +\tif (ret < 0)\n> >> +\t\tLOG(RPI, Error)\n> >> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n> > \n> > Should this be reflected in the request status ?\n> \n> Aha, yes of course. That is indeed how the status could be returned!\n> \n> Can we end up in a situation were we have multiple error status' to\n> return in a request? Perhaps we might have to store a vector of Error\n> status'?\n\nIdeally we should have no error :-) When an error occurs I expect it\nwill be fatal for the application, so I don't think we need to store\nmultiple error sources.\n\n> >> +\n> >> \tpipe_->completeRequest(camera_, request);\n> >> }\n> >> \n> > \n> > [snip]\n> > \n> >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> >> index e2d02c0dafb8..216fbe237827 100644\n> >> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> >> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> >> @@ -14,6 +14,7 @@\n> >> #include \"media_device.h\"\n> >> #include \"pipeline_handler.h\"\n> >> #include \"utils.h\"\n> >> +#include \"v4l2_controls.h\"\n> >> #include \"v4l2_device.h\"\n> >> \n> >> namespace libcamera {\n> >> @@ -73,6 +74,9 @@ public:\n> >> \tint start(Camera *camera) override;\n> >> \tvoid stop(Camera *camera) override;\n> >> \n> >> +\tint readControls(Camera *camera, Request *request) override;\n> >> +\tint writeControls(Camera *camera, Request *request) override;\n> >> +\n> >> \tint queueRequest(Camera *camera, Request *request) override;\n> >> \n> >> \tbool match(DeviceEnumerator *enumerator) override;\n> >> @@ -252,6 +256,117 @@ void PipelineHandlerUVC::stop(Camera *camera)\n> >> \tPipelineHandler::stop(camera);\n> >> }\n> >> \n> >> +int PipelineHandlerUVC::writeControls(Camera *camera, Request *request)\n> >> +{\n> >> +\tUVCCameraData *data = cameraData(camera);\n> >> +\n> >> +\tstd::vector controls;\n> >> +\n> >> +\tfor (Control c : request->controls()) {\n> >> +\t\tif (c.value().isWrite()) {\n> >> +\t\t\tswitch (c.id()) {\n> >> +\t\t\tcase ManualBrightness:\n> >> +\t\t\t\tcontrols.emplace_back(new V4L2IntControl(V4L2_CID_BRIGHTNESS, c.value().getInt()));\n> >> +\t\t\t\tbreak;\n> >> +\n> >> +\t\t\tcase IpaAwbEnable:\n> >> +\t\t\tcase ManualGain:\n> >> +\t\t\tcase ManualExposure:\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tdefault:\n> >> +\t\t\t\tLOG(UVC, Error)\n> >> +\t\t\t\t\t<< \"Unhandled write control\";\n> >> +\t\t\t\tbreak;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> >> +\tif (!controls.empty()) {\n> >> +\t\tint ret = data->video_->setControls(controls);\n> >> +\n> >> +\t\t/* It would be nice to avoid the need to manually delete\n> >> +\t\t * the controls */\n> > \n> > Yes, this seems like a no-go to me.\n> \n> I think we can adapt the V4L2Control interface to accept a set or map as\n> well, and potentially even (where relevant) put in a reference to the\n> ControlValue supplied to prevent excess copying of what will essentially\n> be the same object type to update.\n> \n> >> +\t\tfor (V4L2Control *c : controls)\n> >> +\t\t\tdelete c;\n> >> +\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int PipelineHandlerUVC::readControls(Camera *camera, Request *request)\n> >> +{\n> >> +\tUVCCameraData *data = cameraData(camera);\n> >> +\tstd::vector controlIDs;\n> >> +\tstd::vector controls;\n> >> +\n> >> +\tfor (Control c : request->controls()) {\n> >> +\t\tif (c.value().isRead()) {\n> >> +\t\t\tLOG(UVC, Error) << \"Read Control: \" << c;\n> >> +\n> >> +\t\t\tswitch (c.id()) {\n> >> +\t\t\tcase ManualBrightness:\n> >> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_BRIGHTNESS);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tcase ManualGain:\n> >> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tcase ManualExposure:\n> >> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tdefault:\n> >> +\t\t\t\tLOG(UVC, Error)\n> >> +\t\t\t\t\t<< \"Unhandled write control\";\n> >> +\t\t\t\tbreak;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> >> +\tif (controlIDs.empty())\n> >> +\t\treturn 0;\n> >> +\n> >> +\tcontrols = data->video_->getControls(controlIDs);\n> >> +\tif (controls.empty())\n> >> +\t\treturn -ENODATA;\n> >> +\n> >> +\tfor (V4L2Control *ctrl : controls) {\n> >> +\t\tswitch (ctrl->id()) {\n> >> +\t\tcase V4L2_CID_BRIGHTNESS: {\n> >> +\t\t\tControl bright = *request->controls().find(ManualBrightness);\n> >> +\t\t\t/* RFC:\n> >> +\t\t\t * If the iterator doesn't find the control ^\n> >> +\t\t\t * then it causes a segfault, so this is really nasty...\n> >> +\t\t\t * and where just a map might be better - (ornot?) as it\n> >> +\t\t\t * will create the object at that key.\n> >> +\t\t\t * Now , that *shouldn't* happen (we should only look\n> >> +\t\t\t * for controls that were given to us in this\n> >> +\t\t\t * function ... but\n> > \n> > \t\t\tauto iter = request->controls().find(ManualBrightness);\n> > \t\t\tif (iter == request->controls().end())\n> > \t\t\t\t...\n> \n> It shouldn't ever occur, as we would only look for a value we have\n> requested... but that's what made me interested in the difference for\n> using a map, as it would then create the ControlValue for use.\n> \n> But then also if we are always going to have all controls in the\n> request, then failing to find a control would become an assertion failure.\n\nI don't think all requests will contain all controls, I expect them to\nbe incremental, without having to store the full state.\n\n> >> +\t\t\t */\n> >> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n> >> +\t\t\tbright.value().set(vc->value());\n> >> +\n> >> +\t\t\tLOG(UVC, Debug) << \"Setting Brightness: \" << bright;\n> >> +\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> +\t\tcase V4L2_CID_ANALOGUE_GAIN: {\n> >> +\t\t\tControl gain = *request->controls().find(ManualGain);\n> >> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n> >> +\t\t\tgain.value().set(vc->value());\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> +\t\tdefault:\n> >> +\t\t\tLOG(UVC, Warning) << \"Unhandled V4L2 Control ID\";\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> >> {\n> >> \tUVCCameraData *data = cameraData(camera);\n> >> @@ -263,7 +378,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> >> \t\treturn -ENOENT;\n> >> \t}\n> >> \n> >> -\tint ret = data->video_->queueBuffer(buffer);\n> >> +\tint ret = writeControls(camera, request);\n> >> +\tif (ret < 0)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = data->video_->queueBuffer(buffer);\n> >> \tif (ret < 0)\n> >> \t\treturn ret;\n> >> \n> >> @@ -317,6 +436,12 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n> >> \tRequest *request = queuedRequests_.front();\n> >> \n> >> \tpipe_->completeBuffer(camera_, request, buffer);\n> >> +\n> >> +\tint ret = pipe_->readControls(camera_, request);\n> >> +\tif (ret < 0)\n> >> +\t\tLOG(UVC, Error)\n> >> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n> >> +\n> >> \tpipe_->completeRequest(camera_, request);\n> >> }\n> >> \n> > \n> > [snip]","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 5070D61E1B\n\tfor ;\n\tTue, 11 Jun 2019 14:13:46 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE390FA0;\n\tTue, 11 Jun 2019 14:13:45 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560255226;\n\tbh=9kg7+pTb9peJk5abI4qSxtH8c04iGEvoEBQFQtgDk9M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bVJ9RY7PLicjpuAuzhw2qYspikA52Pfi1Im9Pf2Z8+3ZVptF9HezlIPbWBKYDSh+z\n\tYqTFwc2PS9WXLepmaFCrnur0yqrjx56/gOEhrjbcI7OhR38nHzoOAdLTv9rK094N+I\n\tPjgTUZpNx0Rp5xtAQijYcJF1mLhE6An8SOEx8zn8=","Date":"Tue, 11 Jun 2019 15:13:30 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190611121330.GH5016@pendragon.ideasonboard.com>","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-4-kieran.bingham@ideasonboard.com>\n\t<20190607161943.GS7593@pendragon.ideasonboard.com>\n\t<65187dec-3d82-403f-b04f-f6b519e95c1d@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<65187dec-3d82-403f-b04f-f6b519e95c1d@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","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":"Tue, 11 Jun 2019 12:13:46 -0000"}},{"id":1855,"web_url":"https://patchwork.libcamera.org/comment/1855/","msgid":"","date":"2019-06-11T14:16:53","subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 11/06/2019 13:13, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Fri, Jun 07, 2019 at 10:35:15PM +0100, Kieran Bingham wrote:\n>> Hi Laurent,\n>>\n>> Thank you for your comments, they are certainly helpful for sorting out\n>> some of the design issues.\n>>\n>> On 07/06/2019 17:19, Laurent Pinchart wrote:\n>>> On Thu, Jun 06, 2019 at 09:56:52PM +0100, Kieran Bingham wrote:\n>>>> Pipeline handlers must implement functions to handle controls as part of\n>>>> their interface.\n>>>>\n>>>> Extend the pipeline handler interface to declare this requirement and\n>>>> implement basic control functionality in the currently supported\n>>>> PipelineHandlers\n>>>\n>>> I don't think this should be exposed through the PipelineHandler API, it\n>>> should be handled inside each pipeline handler instead. You're calling\n>>> the readControls() and writeControls() methods of the pipeline handler\n>>> internally only in this patch, so it's effectively internal already.\n>>\n>> I had visions of the 'call' to read/write the controls being in a common\n>> point, but as the PipelineHandlers determine when the request completes,\n>> and the internal timings - I think you're right. There is no need to\n>> enforce these functions on the handler API.\n>>\n>> It's just up to a pipeline handler to decide how to deal with controls.\n>>\n>>>> ---\n>>>> src/libcamera/include/pipeline_handler.h | 3 +\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++\n>>>> src/libcamera/pipeline/raspberrypi.cpp | 108 ++++++++++++++++++-\n>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++\n>>>> src/libcamera/pipeline/uvcvideo.cpp | 127 ++++++++++++++++++++++-\n>>>> src/libcamera/pipeline/vimc.cpp | 19 ++++\n>>>> 6 files changed, 293 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n>>>> index 7da6df1ab2a0..4e306d964bff 100644\n>>>> --- a/src/libcamera/include/pipeline_handler.h\n>>>> +++ b/src/libcamera/include/pipeline_handler.h\n>>>> @@ -72,6 +72,9 @@ public:\n>>>> \tvirtual int start(Camera *camera) = 0;\n>>>> \tvirtual void stop(Camera *camera);\n>>>> \n>>>> +\tvirtual int readControls(Camera *camera, Request *request) = 0;\n>>>> +\tvirtual int writeControls(Camera *camera, Request *request) = 0;\n>>>> +\n>>>> \tvirtual int queueRequest(Camera *camera, Request *request);\n>>>> \n>>>> \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n>>>\n>>> [snip]\n>>>\n>>>> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp\n>>>> index d6749eaae759..337554501c82 100644\n>>>> --- a/src/libcamera/pipeline/raspberrypi.cpp\n>>>> +++ b/src/libcamera/pipeline/raspberrypi.cpp\n>>>> @@ -15,6 +15,7 @@\n>>>> #include \"media_device.h\"\n>>>> #include \"pipeline_handler.h\"\n>>>> #include \"utils.h\"\n>>>> +#include \"v4l2_controls.h\"\n>>>> #include \"v4l2_device.h\"\n>>>> \n>>>> namespace libcamera {\n>>>> @@ -77,6 +78,9 @@ public:\n>>>> \tint start(Camera *camera) override;\n>>>> \tvoid stop(Camera *camera) override;\n>>>> \n>>>> +\tint writeControls(Camera *camera, Request *request);\n>>>> +\tint readControls(Camera *camera, Request *request);\n>>>> +\n>>>> \tint queueRequest(Camera *camera, Request *request) override;\n>>>> \n>>>> \tbool match(DeviceEnumerator *enumerator) override;\n>>>> @@ -293,6 +297,94 @@ void PipelineHandlerRPi::stop(Camera *camera)\n>>>> \tPipelineHandler::stop(camera);\n>>>> }\n>>>> \n>>>> +int PipelineHandlerRPi::writeControls(Camera *camera, Request *request)\n>>>> +{\n>>>> +\tRPiCameraData *data = cameraData(camera);\n>>>> +\n>>>> +\tstd::vector controls;\n>>>> +\n>>>> +\tfor (Control c : request->controls()) {\n>>>> +\t\tif (c.value().isWrite()) {\n>>>\n>>> Would it make sense to split the read and write sets at the Request\n>>> level ?\n>>\n>> I'd love to be able to do a python generator type syntax here, and\n>> 'yield' to cheaply iterate the controls handling ones that are appropriate.\n>>\n>> This might be possible by writing our own iterators, but I'm not sure\n>> how elegant that would be.\n>>\n>> If you are imagining that every request defines every control, how would\n>> you anticipate then splitting up the distinction at the request level?\n>> With multiple sets?\n> \n> Yes, a set of controls, and a set of metadata (I'm more and more\n> thinking that what we called read controls so far should be renamed\n> metadata).\n\nHrm... well if all 'metadata' is read back all the time, then the\napplication doesn't need to request it specifically anyway.\n\nI'm not yet sure if this would be a separate object in the request when\nit's returned or just the 'controls' map updated.\n\nI feel like just updating the controls map seems obvious at the moment.\nLets see where it goes, and that part can easily change/adapt later.\n\n\n>> What values would you expect to be present in the reqeust when it is\n>> created with preset controls?\n>>\n>> - All values at default?\n>> - All values at current / cached values?\n>> - Not set at all... perhaps in the request in an 'unset' state.?\n>> - Some other option?\n> \n> I don't expect requests to be created with \"preset controls\". Requests\n> should be created empty, and populate it by applications. This could be\n> done manually, or through a preset provided by libcamera. I would expect\n> applications to create multiple instances of lists of controls, populate\n> them with the help of preset and manual settings, and then use them at\n> runtime to populate requests.\n> \n>>>> +\t\t\tswitch (c.id()) {\n>>>> +\t\t\tcase ManualGain:\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\tcase ManualExposure:\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\tdefault:\n>>>> +\t\t\t\tLOG(RPI, Error)\n>>>> +\t\t\t\t\t<< \"Unhandled write control\";\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\t}\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n>>>> +\tif (!controls.empty())\n>>>> +\t\treturn data->unicam_->setControls(controls);\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +/* This is becoming horrible. How can we improve this\n>>>> + * - Iterate reqeust controls to find controls to read\n>>>> + * - Add those to a new list\n>>>> + * - Query the device for that list to get controls\n>>>> + * - iterate returned list\n>>>> + * \t- For each control, search for corresponding control in request\n>>>> + * \t- Set control value to return value.\n>>>> + * \t- Return list.\n>>>> + * \t- Check for any values that were not updated?\n>>>> + */\n>>>> +int PipelineHandlerRPi::readControls(Camera *camera, Request *request)\n>>>> +{\n>>>> +\tRPiCameraData *data = cameraData(camera);\n>>>> +\tstd::vector controlIDs;\n>>>> +\tstd::vector controls;\n>>>> +\n>>>> +\tfor (Control c : request->controls()) {\n>>>> +\t\tif (c.value().isRead()) {\n>>>> +\t\t\tLOG(RPI, Error) << \"Read Control: \" << c;\n>>>> +\n>>>> +\t\t\tswitch (c.id()) {\n>>>> +\t\t\tcase ManualGain:\n>>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\tcase ManualExposure:\n>>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\tdefault:\n>>>> +\t\t\t\tLOG(RPI, Error)\n>>>> +\t\t\t\t\t<< \"Unhandled write control\";\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\t}\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n>>>> +\tif (controlIDs.empty())\n>>>> +\t\treturn 0;\n>>>> +\n>>>> +\tcontrols = data->unicam_->getControls(controlIDs);\n>>>> +\tif (controls.empty())\n>>>> +\t\treturn -ENODATA;\n>>>> +\n>>>> +\tfor (V4L2Control *ctrl : controls) {\n>>>> +\t\tswitch(ctrl->id()) {\n>>>> +\t\tcase V4L2_CID_ANALOGUE_GAIN:\n>>>> +\t\t\t//Control gain(ManualGain); //= request->controls().find(ManualGain);\n>>>> +\t\t\tauto it = request->controls().find(ManualGain);\n>>>> +\t\t\tControl gain = *it;\n>>>> +\n>>>> +\t\t\t//V4L2IntControl *c = static_cast(ctrl);\n>>>> +\t\t\tgain.value().set(0x88);\n>>>> +\n>>>> +\t\t\tbreak;\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n>>>> {\n>>>> \tRPiCameraData *data = cameraData(camera);\n>>>> @@ -304,7 +396,15 @@ int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n>>>> \t\treturn -ENOENT;\n>>>> \t}\n>>>> \n>>>> -\tint ret = data->isp_.capture->queueBuffer(buffer);\n>>>> +\t/*\n>>>> +\t * Handle 'set' controls first\n>>>> +\t * Todo: Validate ordering and timing.\n>>>\n>>> Handling timing will be interesting, as V4L2 controls are synchronous,\n>>> while buffers are not. I expect we'll need to evaluate when the buffer\n>>> will be captured, and set a timer accordingly to set controls (taking\n>>> into account the latencies of the capture pipeline).\n>>\n>> Indeed - making sure we set the value for that buffer is going to be\n>> interesting. It might be some 'guess work' :-( - but we should know how\n>> many buffers are queued ahead of us, and we can store the completion\n>> time of each previously completed buffer so we should be able to\n>> estimate when things happen in some form, and calculate our expected\n>> 'latency' for any given request when it is queued.\n>>\n>> Hrm ... that sounds like it might actually be quite fun (but probably\n>> also painful somehow) to implement ...\n> \n> Fun and painful, yes :-) I hope we'll be able to factor some of that\n> code to helpers at a later point.\n> \n>>>> +\t */\n>>>> +\tint ret = writeControls(camera, request);\n>>>> +\tif (ret < 0)\n>>>> +\t\treturn ret;\n>>>> +\n>>>> +\tret = data->isp_.capture->queueBuffer(buffer);\n>>>> \tif (ret < 0)\n>>>> \t\treturn ret;\n>>>> \n>>>> @@ -394,6 +494,12 @@ void RPiCameraData::ispCaptureReady(Buffer *buffer)\n>>>> \tRequest *request = queuedRequests_.front();\n>>>> \n>>>> \tpipe_->completeBuffer(camera_, request, buffer);\n>>>> +\n>>>> +\tint ret = pipe_->readControls(camera_, request);\n>>>> +\tif (ret < 0)\n>>>> +\t\tLOG(RPI, Error)\n>>>> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n>>>\n>>> Should this be reflected in the request status ?\n>>\n>> Aha, yes of course. That is indeed how the status could be returned!\n>>\n>> Can we end up in a situation were we have multiple error status' to\n>> return in a request? Perhaps we might have to store a vector of Error\n>> status'?\n> \n> Ideally we should have no error :-) When an error occurs I expect it\n> will be fatal for the application, so I don't think we need to store\n> multiple error sources.\n\nOk - that's fine by me then.\n\n> \n>>>> +\n>>>> \tpipe_->completeRequest(camera_, request);\n>>>> }\n>>>> \n>>>\n>>> [snip]\n>>>\n>>>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n>>>> index e2d02c0dafb8..216fbe237827 100644\n>>>> --- a/src/libcamera/pipeline/uvcvideo.cpp\n>>>> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n>>>> @@ -14,6 +14,7 @@\n>>>> #include \"media_device.h\"\n>>>> #include \"pipeline_handler.h\"\n>>>> #include \"utils.h\"\n>>>> +#include \"v4l2_controls.h\"\n>>>> #include \"v4l2_device.h\"\n>>>> \n>>>> namespace libcamera {\n>>>> @@ -73,6 +74,9 @@ public:\n>>>> \tint start(Camera *camera) override;\n>>>> \tvoid stop(Camera *camera) override;\n>>>> \n>>>> +\tint readControls(Camera *camera, Request *request) override;\n>>>> +\tint writeControls(Camera *camera, Request *request) override;\n>>>> +\n>>>> \tint queueRequest(Camera *camera, Request *request) override;\n>>>> \n>>>> \tbool match(DeviceEnumerator *enumerator) override;\n>>>> @@ -252,6 +256,117 @@ void PipelineHandlerUVC::stop(Camera *camera)\n>>>> \tPipelineHandler::stop(camera);\n>>>> }\n>>>> \n>>>> +int PipelineHandlerUVC::writeControls(Camera *camera, Request *request)\n>>>> +{\n>>>> +\tUVCCameraData *data = cameraData(camera);\n>>>> +\n>>>> +\tstd::vector controls;\n>>>> +\n>>>> +\tfor (Control c : request->controls()) {\n>>>> +\t\tif (c.value().isWrite()) {\n>>>> +\t\t\tswitch (c.id()) {\n>>>> +\t\t\tcase ManualBrightness:\n>>>> +\t\t\t\tcontrols.emplace_back(new V4L2IntControl(V4L2_CID_BRIGHTNESS, c.value().getInt()));\n>>>> +\t\t\t\tbreak;\n>>>> +\n>>>> +\t\t\tcase IpaAwbEnable:\n>>>> +\t\t\tcase ManualGain:\n>>>> +\t\t\tcase ManualExposure:\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\tdefault:\n>>>> +\t\t\t\tLOG(UVC, Error)\n>>>> +\t\t\t\t\t<< \"Unhandled write control\";\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\t}\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n>>>> +\tif (!controls.empty()) {\n>>>> +\t\tint ret = data->video_->setControls(controls);\n>>>> +\n>>>> +\t\t/* It would be nice to avoid the need to manually delete\n>>>> +\t\t * the controls */\n>>>\n>>> Yes, this seems like a no-go to me.\n>>\n>> I think we can adapt the V4L2Control interface to accept a set or map as\n>> well, and potentially even (where relevant) put in a reference to the\n>> ControlValue supplied to prevent excess copying of what will essentially\n>> be the same object type to update.\n>>\n>>>> +\t\tfor (V4L2Control *c : controls)\n>>>> +\t\t\tdelete c;\n>>>> +\n>>>> +\t\treturn ret;\n>>>> +\t}\n>>>> +\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +int PipelineHandlerUVC::readControls(Camera *camera, Request *request)\n>>>> +{\n>>>> +\tUVCCameraData *data = cameraData(camera);\n>>>> +\tstd::vector controlIDs;\n>>>> +\tstd::vector controls;\n>>>> +\n>>>> +\tfor (Control c : request->controls()) {\n>>>> +\t\tif (c.value().isRead()) {\n>>>> +\t\t\tLOG(UVC, Error) << \"Read Control: \" << c;\n>>>> +\n>>>> +\t\t\tswitch (c.id()) {\n>>>> +\t\t\tcase ManualBrightness:\n>>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_BRIGHTNESS);\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\tcase ManualGain:\n>>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\tcase ManualExposure:\n>>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\tdefault:\n>>>> +\t\t\t\tLOG(UVC, Error)\n>>>> +\t\t\t\t\t<< \"Unhandled write control\";\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\t}\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n>>>> +\tif (controlIDs.empty())\n>>>> +\t\treturn 0;\n>>>> +\n>>>> +\tcontrols = data->video_->getControls(controlIDs);\n>>>> +\tif (controls.empty())\n>>>> +\t\treturn -ENODATA;\n>>>> +\n>>>> +\tfor (V4L2Control *ctrl : controls) {\n>>>> +\t\tswitch (ctrl->id()) {\n>>>> +\t\tcase V4L2_CID_BRIGHTNESS: {\n>>>> +\t\t\tControl bright = *request->controls().find(ManualBrightness);\n>>>> +\t\t\t/* RFC:\n>>>> +\t\t\t * If the iterator doesn't find the control ^\n>>>> +\t\t\t * then it causes a segfault, so this is really nasty...\n>>>> +\t\t\t * and where just a map might be better - (ornot?) as it\n>>>> +\t\t\t * will create the object at that key.\n>>>> +\t\t\t * Now , that *shouldn't* happen (we should only look\n>>>> +\t\t\t * for controls that were given to us in this\n>>>> +\t\t\t * function ... but\n>>>\n>>> \t\t\tauto iter = request->controls().find(ManualBrightness);\n>>> \t\t\tif (iter == request->controls().end())\n>>> \t\t\t\t...\n>>\n>> It shouldn't ever occur, as we would only look for a value we have\n>> requested... but that's what made me interested in the difference for\n>> using a map, as it would then create the ControlValue for use.\n>>\n>> But then also if we are always going to have all controls in the\n>> request, then failing to find a control would become an assertion failure.\n> \n> I don't think all requests will contain all controls, I expect them to\n> be incremental, without having to store the full state.\n\nWell, actually - after our discussion on the other thread, at this point\nin the code - it could well be that we do want to read more controls\nthan were in the request, and thus a map really would be a good option.\n\nThen the pipeline handler can update any extra control/value pairs as\nnecessary - and the application will get a consistent (or rather always\nat least a minimum) set of data back\n\n\n> \n>>>> +\t\t\t */\n>>>> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n>>>> +\t\t\tbright.value().set(vc->value());\n>>>> +\n>>>> +\t\t\tLOG(UVC, Debug) << \"Setting Brightness: \" << bright;\n>>>> +\n>>>> +\t\t\tbreak;\n>>>> +\t\t}\n>>>> +\t\tcase V4L2_CID_ANALOGUE_GAIN: {\n>>>> +\t\t\tControl gain = *request->controls().find(ManualGain);\n>>>> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n>>>> +\t\t\tgain.value().set(vc->value());\n>>>> +\t\t\tbreak;\n>>>> +\t\t}\n>>>> +\t\tdefault:\n>>>> +\t\t\tLOG(UVC, Warning) << \"Unhandled V4L2 Control ID\";\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n>>>> {\n>>>> \tUVCCameraData *data = cameraData(camera);\n>>>> @@ -263,7 +378,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n>>>> \t\treturn -ENOENT;\n>>>> \t}\n>>>> \n>>>> -\tint ret = data->video_->queueBuffer(buffer);\n>>>> +\tint ret = writeControls(camera, request);\n>>>> +\tif (ret < 0)\n>>>> +\t\treturn ret;\n>>>> +\n>>>> +\tret = data->video_->queueBuffer(buffer);\n>>>> \tif (ret < 0)\n>>>> \t\treturn ret;\n>>>> \n>>>> @@ -317,6 +436,12 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n>>>> \tRequest *request = queuedRequests_.front();\n>>>> \n>>>> \tpipe_->completeBuffer(camera_, request, buffer);\n>>>> +\n>>>> +\tint ret = pipe_->readControls(camera_, request);\n>>>> +\tif (ret < 0)\n>>>> +\t\tLOG(UVC, Error)\n>>>> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n>>>> +\n>>>> \tpipe_->completeRequest(camera_, request);\n>>>> }\n>>>> \n>>>\n>>> [snip]\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 69A6F62F75\n\tfor ;\n\tTue, 11 Jun 2019 16:16:56 +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 D0673FA0;\n\tTue, 11 Jun 2019 16:16:55 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560262616;\n\tbh=h//NXIMYofumvSa1mQ6vs8egVC+l6zrHXYo9chZuQII=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=A+2Zy+Z8K3J0ieU9Xvs7ihaGPuSBsOsxGw6SmZM1uaas29BW1D+/WQnh+WHuQdddw\n\tPeC4bstREI2UAqE5FmjCfaKPWEdxOo6vyjvYEZdc2kj4trhN3ur38WDzFNibiGEtVV\n\tS/ACy9pA2Xb//5BTPOkupf43fIi4fBSTgPsXDpPg=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-4-kieran.bingham@ideasonboard.com>\n\t<20190607161943.GS7593@pendragon.ideasonboard.com>\n\t<65187dec-3d82-403f-b04f-f6b519e95c1d@ideasonboard.com>\n\t<20190611121330.GH5016@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":"","Date":"Tue, 11 Jun 2019 15:16:53 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190611121330.GH5016@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","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":"Tue, 11 Jun 2019 14:16:56 -0000"}},{"id":1857,"web_url":"https://patchwork.libcamera.org/comment/1857/","msgid":"<20190611145157.GN5016@pendragon.ideasonboard.com>","date":"2019-06-11T14:51:57","subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Jun 11, 2019 at 03:16:53PM +0100, Kieran Bingham wrote:\n> On 11/06/2019 13:13, Laurent Pinchart wrote:\n> > On Fri, Jun 07, 2019 at 10:35:15PM +0100, Kieran Bingham wrote:\n> >> Hi Laurent,\n> >>\n> >> Thank you for your comments, they are certainly helpful for sorting out\n> >> some of the design issues.\n> >>\n> >> On 07/06/2019 17:19, Laurent Pinchart wrote:\n> >>> On Thu, Jun 06, 2019 at 09:56:52PM +0100, Kieran Bingham wrote:\n> >>>> Pipeline handlers must implement functions to handle controls as part of\n> >>>> their interface.\n> >>>>\n> >>>> Extend the pipeline handler interface to declare this requirement and\n> >>>> implement basic control functionality in the currently supported\n> >>>> PipelineHandlers\n> >>>\n> >>> I don't think this should be exposed through the PipelineHandler API, it\n> >>> should be handled inside each pipeline handler instead. You're calling\n> >>> the readControls() and writeControls() methods of the pipeline handler\n> >>> internally only in this patch, so it's effectively internal already.\n> >>\n> >> I had visions of the 'call' to read/write the controls being in a common\n> >> point, but as the PipelineHandlers determine when the request completes,\n> >> and the internal timings - I think you're right. There is no need to\n> >> enforce these functions on the handler API.\n> >>\n> >> It's just up to a pipeline handler to decide how to deal with controls.\n> >>\n> >>>> ---\n> >>>> src/libcamera/include/pipeline_handler.h | 3 +\n> >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++\n> >>>> src/libcamera/pipeline/raspberrypi.cpp | 108 ++++++++++++++++++-\n> >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++\n> >>>> src/libcamera/pipeline/uvcvideo.cpp | 127 ++++++++++++++++++++++-\n> >>>> src/libcamera/pipeline/vimc.cpp | 19 ++++\n> >>>> 6 files changed, 293 insertions(+), 2 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> >>>> index 7da6df1ab2a0..4e306d964bff 100644\n> >>>> --- a/src/libcamera/include/pipeline_handler.h\n> >>>> +++ b/src/libcamera/include/pipeline_handler.h\n> >>>> @@ -72,6 +72,9 @@ public:\n> >>>> \tvirtual int start(Camera *camera) = 0;\n> >>>> \tvirtual void stop(Camera *camera);\n> >>>> \n> >>>> +\tvirtual int readControls(Camera *camera, Request *request) = 0;\n> >>>> +\tvirtual int writeControls(Camera *camera, Request *request) = 0;\n> >>>> +\n> >>>> \tvirtual int queueRequest(Camera *camera, Request *request);\n> >>>> \n> >>>> \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> >>>\n> >>> [snip]\n> >>>\n> >>>> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp\n> >>>> index d6749eaae759..337554501c82 100644\n> >>>> --- a/src/libcamera/pipeline/raspberrypi.cpp\n> >>>> +++ b/src/libcamera/pipeline/raspberrypi.cpp\n> >>>> @@ -15,6 +15,7 @@\n> >>>> #include \"media_device.h\"\n> >>>> #include \"pipeline_handler.h\"\n> >>>> #include \"utils.h\"\n> >>>> +#include \"v4l2_controls.h\"\n> >>>> #include \"v4l2_device.h\"\n> >>>> \n> >>>> namespace libcamera {\n> >>>> @@ -77,6 +78,9 @@ public:\n> >>>> \tint start(Camera *camera) override;\n> >>>> \tvoid stop(Camera *camera) override;\n> >>>> \n> >>>> +\tint writeControls(Camera *camera, Request *request);\n> >>>> +\tint readControls(Camera *camera, Request *request);\n> >>>> +\n> >>>> \tint queueRequest(Camera *camera, Request *request) override;\n> >>>> \n> >>>> \tbool match(DeviceEnumerator *enumerator) override;\n> >>>> @@ -293,6 +297,94 @@ void PipelineHandlerRPi::stop(Camera *camera)\n> >>>> \tPipelineHandler::stop(camera);\n> >>>> }\n> >>>> \n> >>>> +int PipelineHandlerRPi::writeControls(Camera *camera, Request *request)\n> >>>> +{\n> >>>> +\tRPiCameraData *data = cameraData(camera);\n> >>>> +\n> >>>> +\tstd::vector controls;\n> >>>> +\n> >>>> +\tfor (Control c : request->controls()) {\n> >>>> +\t\tif (c.value().isWrite()) {\n> >>>\n> >>> Would it make sense to split the read and write sets at the Request\n> >>> level ?\n> >>\n> >> I'd love to be able to do a python generator type syntax here, and\n> >> 'yield' to cheaply iterate the controls handling ones that are appropriate.\n> >>\n> >> This might be possible by writing our own iterators, but I'm not sure\n> >> how elegant that would be.\n> >>\n> >> If you are imagining that every request defines every control, how would\n> >> you anticipate then splitting up the distinction at the request level?\n> >> With multiple sets?\n> > \n> > Yes, a set of controls, and a set of metadata (I'm more and more\n> > thinking that what we called read controls so far should be renamed\n> > metadata).\n> \n> Hrm... well if all 'metadata' is read back all the time, then the\n> application doesn't need to request it specifically anyway.\n> \n> I'm not yet sure if this would be a separate object in the request when\n> it's returned or just the 'controls' map updated.\n> \n> I feel like just updating the controls map seems obvious at the moment.\n> Lets see where it goes, and that part can easily change/adapt later.\n\nI expect most metadata items to be read-only, and I also expect related\ncontrol and metadata items to be easier to use if handled separately.\nFor instance the control that sets the desired exposure time and the\nmetadata item that reports the actual exposure time are two different\npieces of data, and it could be useful for applications to access at\nrequest completion both the manual exposure value they have requested\nand the actual exposure time.\n\n> >> What values would you expect to be present in the reqeust when it is\n> >> created with preset controls?\n> >>\n> >> - All values at default?\n> >> - All values at current / cached values?\n> >> - Not set at all... perhaps in the request in an 'unset' state.?\n> >> - Some other option?\n> > \n> > I don't expect requests to be created with \"preset controls\". Requests\n> > should be created empty, and populate it by applications. This could be\n> > done manually, or through a preset provided by libcamera. I would expect\n> > applications to create multiple instances of lists of controls, populate\n> > them with the help of preset and manual settings, and then use them at\n> > runtime to populate requests.\n> > \n> >>>> +\t\t\tswitch (c.id()) {\n> >>>> +\t\t\tcase ManualGain:\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tcase ManualExposure:\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tdefault:\n> >>>> +\t\t\t\tLOG(RPI, Error)\n> >>>> +\t\t\t\t\t<< \"Unhandled write control\";\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\t}\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>> +\n> >>>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> >>>> +\tif (!controls.empty())\n> >>>> +\t\treturn data->unicam_->setControls(controls);\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> +}\n> >>>> +\n> >>>> +/* This is becoming horrible. How can we improve this\n> >>>> + * - Iterate reqeust controls to find controls to read\n> >>>> + * - Add those to a new list\n> >>>> + * - Query the device for that list to get controls\n> >>>> + * - iterate returned list\n> >>>> + * \t- For each control, search for corresponding control in request\n> >>>> + * \t- Set control value to return value.\n> >>>> + * \t- Return list.\n> >>>> + * \t- Check for any values that were not updated?\n> >>>> + */\n> >>>> +int PipelineHandlerRPi::readControls(Camera *camera, Request *request)\n> >>>> +{\n> >>>> +\tRPiCameraData *data = cameraData(camera);\n> >>>> +\tstd::vector controlIDs;\n> >>>> +\tstd::vector controls;\n> >>>> +\n> >>>> +\tfor (Control c : request->controls()) {\n> >>>> +\t\tif (c.value().isRead()) {\n> >>>> +\t\t\tLOG(RPI, Error) << \"Read Control: \" << c;\n> >>>> +\n> >>>> +\t\t\tswitch (c.id()) {\n> >>>> +\t\t\tcase ManualGain:\n> >>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tcase ManualExposure:\n> >>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tdefault:\n> >>>> +\t\t\t\tLOG(RPI, Error)\n> >>>> +\t\t\t\t\t<< \"Unhandled write control\";\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\t}\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>> +\n> >>>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> >>>> +\tif (controlIDs.empty())\n> >>>> +\t\treturn 0;\n> >>>> +\n> >>>> +\tcontrols = data->unicam_->getControls(controlIDs);\n> >>>> +\tif (controls.empty())\n> >>>> +\t\treturn -ENODATA;\n> >>>> +\n> >>>> +\tfor (V4L2Control *ctrl : controls) {\n> >>>> +\t\tswitch(ctrl->id()) {\n> >>>> +\t\tcase V4L2_CID_ANALOGUE_GAIN:\n> >>>> +\t\t\t//Control gain(ManualGain); //= request->controls().find(ManualGain);\n> >>>> +\t\t\tauto it = request->controls().find(ManualGain);\n> >>>> +\t\t\tControl gain = *it;\n> >>>> +\n> >>>> +\t\t\t//V4L2IntControl *c = static_cast(ctrl);\n> >>>> +\t\t\tgain.value().set(0x88);\n> >>>> +\n> >>>> +\t\t\tbreak;\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> +}\n> >>>> +\n> >>>> int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n> >>>> {\n> >>>> \tRPiCameraData *data = cameraData(camera);\n> >>>> @@ -304,7 +396,15 @@ int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)\n> >>>> \t\treturn -ENOENT;\n> >>>> \t}\n> >>>> \n> >>>> -\tint ret = data->isp_.capture->queueBuffer(buffer);\n> >>>> +\t/*\n> >>>> +\t * Handle 'set' controls first\n> >>>> +\t * Todo: Validate ordering and timing.\n> >>>\n> >>> Handling timing will be interesting, as V4L2 controls are synchronous,\n> >>> while buffers are not. I expect we'll need to evaluate when the buffer\n> >>> will be captured, and set a timer accordingly to set controls (taking\n> >>> into account the latencies of the capture pipeline).\n> >>\n> >> Indeed - making sure we set the value for that buffer is going to be\n> >> interesting. It might be some 'guess work' :-( - but we should know how\n> >> many buffers are queued ahead of us, and we can store the completion\n> >> time of each previously completed buffer so we should be able to\n> >> estimate when things happen in some form, and calculate our expected\n> >> 'latency' for any given request when it is queued.\n> >>\n> >> Hrm ... that sounds like it might actually be quite fun (but probably\n> >> also painful somehow) to implement ...\n> > \n> > Fun and painful, yes :-) I hope we'll be able to factor some of that\n> > code to helpers at a later point.\n> > \n> >>>> +\t */\n> >>>> +\tint ret = writeControls(camera, request);\n> >>>> +\tif (ret < 0)\n> >>>> +\t\treturn ret;\n> >>>> +\n> >>>> +\tret = data->isp_.capture->queueBuffer(buffer);\n> >>>> \tif (ret < 0)\n> >>>> \t\treturn ret;\n> >>>> \n> >>>> @@ -394,6 +494,12 @@ void RPiCameraData::ispCaptureReady(Buffer *buffer)\n> >>>> \tRequest *request = queuedRequests_.front();\n> >>>> \n> >>>> \tpipe_->completeBuffer(camera_, request, buffer);\n> >>>> +\n> >>>> +\tint ret = pipe_->readControls(camera_, request);\n> >>>> +\tif (ret < 0)\n> >>>> +\t\tLOG(RPI, Error)\n> >>>> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n> >>>\n> >>> Should this be reflected in the request status ?\n> >>\n> >> Aha, yes of course. That is indeed how the status could be returned!\n> >>\n> >> Can we end up in a situation were we have multiple error status' to\n> >> return in a request? Perhaps we might have to store a vector of Error\n> >> status'?\n> > \n> > Ideally we should have no error :-) When an error occurs I expect it\n> > will be fatal for the application, so I don't think we need to store\n> > multiple error sources.\n> \n> Ok - that's fine by me then.\n> \n> >>>> +\n> >>>> \tpipe_->completeRequest(camera_, request);\n> >>>> }\n> >>>> \n> >>>\n> >>> [snip]\n> >>>\n> >>>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> >>>> index e2d02c0dafb8..216fbe237827 100644\n> >>>> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> >>>> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> >>>> @@ -14,6 +14,7 @@\n> >>>> #include \"media_device.h\"\n> >>>> #include \"pipeline_handler.h\"\n> >>>> #include \"utils.h\"\n> >>>> +#include \"v4l2_controls.h\"\n> >>>> #include \"v4l2_device.h\"\n> >>>> \n> >>>> namespace libcamera {\n> >>>> @@ -73,6 +74,9 @@ public:\n> >>>> \tint start(Camera *camera) override;\n> >>>> \tvoid stop(Camera *camera) override;\n> >>>> \n> >>>> +\tint readControls(Camera *camera, Request *request) override;\n> >>>> +\tint writeControls(Camera *camera, Request *request) override;\n> >>>> +\n> >>>> \tint queueRequest(Camera *camera, Request *request) override;\n> >>>> \n> >>>> \tbool match(DeviceEnumerator *enumerator) override;\n> >>>> @@ -252,6 +256,117 @@ void PipelineHandlerUVC::stop(Camera *camera)\n> >>>> \tPipelineHandler::stop(camera);\n> >>>> }\n> >>>> \n> >>>> +int PipelineHandlerUVC::writeControls(Camera *camera, Request *request)\n> >>>> +{\n> >>>> +\tUVCCameraData *data = cameraData(camera);\n> >>>> +\n> >>>> +\tstd::vector controls;\n> >>>> +\n> >>>> +\tfor (Control c : request->controls()) {\n> >>>> +\t\tif (c.value().isWrite()) {\n> >>>> +\t\t\tswitch (c.id()) {\n> >>>> +\t\t\tcase ManualBrightness:\n> >>>> +\t\t\t\tcontrols.emplace_back(new V4L2IntControl(V4L2_CID_BRIGHTNESS, c.value().getInt()));\n> >>>> +\t\t\t\tbreak;\n> >>>> +\n> >>>> +\t\t\tcase IpaAwbEnable:\n> >>>> +\t\t\tcase ManualGain:\n> >>>> +\t\t\tcase ManualExposure:\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tdefault:\n> >>>> +\t\t\t\tLOG(UVC, Error)\n> >>>> +\t\t\t\t\t<< \"Unhandled write control\";\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\t}\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>> +\n> >>>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> >>>> +\tif (!controls.empty()) {\n> >>>> +\t\tint ret = data->video_->setControls(controls);\n> >>>> +\n> >>>> +\t\t/* It would be nice to avoid the need to manually delete\n> >>>> +\t\t * the controls */\n> >>>\n> >>> Yes, this seems like a no-go to me.\n> >>\n> >> I think we can adapt the V4L2Control interface to accept a set or map as\n> >> well, and potentially even (where relevant) put in a reference to the\n> >> ControlValue supplied to prevent excess copying of what will essentially\n> >> be the same object type to update.\n> >>\n> >>>> +\t\tfor (V4L2Control *c : controls)\n> >>>> +\t\t\tdelete c;\n> >>>> +\n> >>>> +\t\treturn ret;\n> >>>> +\t}\n> >>>> +\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> +}\n> >>>> +\n> >>>> +int PipelineHandlerUVC::readControls(Camera *camera, Request *request)\n> >>>> +{\n> >>>> +\tUVCCameraData *data = cameraData(camera);\n> >>>> +\tstd::vector controlIDs;\n> >>>> +\tstd::vector controls;\n> >>>> +\n> >>>> +\tfor (Control c : request->controls()) {\n> >>>> +\t\tif (c.value().isRead()) {\n> >>>> +\t\t\tLOG(UVC, Error) << \"Read Control: \" << c;\n> >>>> +\n> >>>> +\t\t\tswitch (c.id()) {\n> >>>> +\t\t\tcase ManualBrightness:\n> >>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_BRIGHTNESS);\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tcase ManualGain:\n> >>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_ANALOGUE_GAIN);\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tcase ManualExposure:\n> >>>> +\t\t\t\tcontrolIDs.push_back(V4L2_CID_EXPOSURE);\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tdefault:\n> >>>> +\t\t\t\tLOG(UVC, Error)\n> >>>> +\t\t\t\t\t<< \"Unhandled write control\";\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\t}\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>> +\n> >>>> +\t/* Perhaps setControls could accept an empty vector/list as success? */\n> >>>> +\tif (controlIDs.empty())\n> >>>> +\t\treturn 0;\n> >>>> +\n> >>>> +\tcontrols = data->video_->getControls(controlIDs);\n> >>>> +\tif (controls.empty())\n> >>>> +\t\treturn -ENODATA;\n> >>>> +\n> >>>> +\tfor (V4L2Control *ctrl : controls) {\n> >>>> +\t\tswitch (ctrl->id()) {\n> >>>> +\t\tcase V4L2_CID_BRIGHTNESS: {\n> >>>> +\t\t\tControl bright = *request->controls().find(ManualBrightness);\n> >>>> +\t\t\t/* RFC:\n> >>>> +\t\t\t * If the iterator doesn't find the control ^\n> >>>> +\t\t\t * then it causes a segfault, so this is really nasty...\n> >>>> +\t\t\t * and where just a map might be better - (ornot?) as it\n> >>>> +\t\t\t * will create the object at that key.\n> >>>> +\t\t\t * Now , that *shouldn't* happen (we should only look\n> >>>> +\t\t\t * for controls that were given to us in this\n> >>>> +\t\t\t * function ... but\n> >>>\n> >>> \t\t\tauto iter = request->controls().find(ManualBrightness);\n> >>> \t\t\tif (iter == request->controls().end())\n> >>> \t\t\t\t...\n> >>\n> >> It shouldn't ever occur, as we would only look for a value we have\n> >> requested... but that's what made me interested in the difference for\n> >> using a map, as it would then create the ControlValue for use.\n> >>\n> >> But then also if we are always going to have all controls in the\n> >> request, then failing to find a control would become an assertion failure.\n> > \n> > I don't think all requests will contain all controls, I expect them to\n> > be incremental, without having to store the full state.\n> \n> Well, actually - after our discussion on the other thread, at this point\n> in the code - it could well be that we do want to read more controls\n> than were in the request, and thus a map really would be a good option.\n> \n> Then the pipeline handler can update any extra control/value pairs as\n> necessary - and the application will get a consistent (or rather always\n> at least a minimum) set of data back\n\nThe only reason I can see to let the application give a list of metadata\nitems it wants to get back is to optimise metadata retrieval from the\nhardware. However, considering that the metadata isn't always\ntransmitted over slow media (e.g. CSI-2 embedded data instead of I2C\nread), and that IPAs will often need to retrieve the metadata anyway,\nI'm not sure it would be a worthwile optimisation.\n\n> >>>> +\t\t\t */\n> >>>> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n> >>>> +\t\t\tbright.value().set(vc->value());\n> >>>> +\n> >>>> +\t\t\tLOG(UVC, Debug) << \"Setting Brightness: \" << bright;\n> >>>> +\n> >>>> +\t\t\tbreak;\n> >>>> +\t\t}\n> >>>> +\t\tcase V4L2_CID_ANALOGUE_GAIN: {\n> >>>> +\t\t\tControl gain = *request->controls().find(ManualGain);\n> >>>> +\t\t\tV4L2IntControl *vc = static_cast(ctrl);\n> >>>> +\t\t\tgain.value().set(vc->value());\n> >>>> +\t\t\tbreak;\n> >>>> +\t\t}\n> >>>> +\t\tdefault:\n> >>>> +\t\t\tLOG(UVC, Warning) << \"Unhandled V4L2 Control ID\";\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> +}\n> >>>> +\n> >>>> int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> >>>> {\n> >>>> \tUVCCameraData *data = cameraData(camera);\n> >>>> @@ -263,7 +378,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> >>>> \t\treturn -ENOENT;\n> >>>> \t}\n> >>>> \n> >>>> -\tint ret = data->video_->queueBuffer(buffer);\n> >>>> +\tint ret = writeControls(camera, request);\n> >>>> +\tif (ret < 0)\n> >>>> +\t\treturn ret;\n> >>>> +\n> >>>> +\tret = data->video_->queueBuffer(buffer);\n> >>>> \tif (ret < 0)\n> >>>> \t\treturn ret;\n> >>>> \n> >>>> @@ -317,6 +436,12 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n> >>>> \tRequest *request = queuedRequests_.front();\n> >>>> \n> >>>> \tpipe_->completeBuffer(camera_, request, buffer);\n> >>>> +\n> >>>> +\tint ret = pipe_->readControls(camera_, request);\n> >>>> +\tif (ret < 0)\n> >>>> +\t\tLOG(UVC, Error)\n> >>>> +\t\t\t<< \"Failed to read controls. No way to pass error\";\n> >>>> +\n> >>>> \tpipe_->completeRequest(camera_, request);\n> >>>> }\n> >>>> \n> >>>\n> >>> [snip]","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 A062462F8E\n\tfor ;\n\tTue, 11 Jun 2019 16:52:12 +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 00E9EFA0;\n\tTue, 11 Jun 2019 16:52:11 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560264732;\n\tbh=ijzWnyhu4Icr5m6JExnGXqUPmIW8QEXSO20DQcAnXBo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IcnQfcom1C0/HE7Zn/wo1581D90AO3cuueY8JmE7GnQvcxQUsoqhQhi4Y929tVkrR\n\tVyCFi29xNnQxTXo0xN4nyEojW74emnhN+DhxHw/IwB6RChESdyWCIf9LHvyzMZUM1f\n\tUa8BLKXanWQbKjYOTJSNuu2GOhZhDwV41/yUA2Oo=","Date":"Tue, 11 Jun 2019 17:51:57 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190611145157.GN5016@pendragon.ideasonboard.com>","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-4-kieran.bingham@ideasonboard.com>\n\t<20190607161943.GS7593@pendragon.ideasonboard.com>\n\t<65187dec-3d82-403f-b04f-f6b519e95c1d@ideasonboard.com>\n\t<20190611121330.GH5016@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add\n\treadControls(), writeControl() interfaces","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":"Tue, 11 Jun 2019 14:52:12 -0000"}}]