[{"id":3141,"web_url":"https://patchwork.libcamera.org/comment/3141/","msgid":"<20191127094354.b2orfwuxnjngrqvs@uno.localdomain>","date":"2019-11-27T09:43:54","subject":"Re: [libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align\n\tbookkeeping in queueRequest()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Nov 27, 2019 at 12:35:51AM +0100, Niklas Söderlund wrote:\n> Expecting pipeline handler implementations of queueRequest() to call\n> the base class queueRequest() at the correct point have lead to\n> different behaviors between the pipelines.\n>\n> Fix this by splitting queueRequest() into a base class implementation\n> which handles the bookkeeping and a new queueRequestHardware() that is\n> to be implemented by pipeline handlers and only deals with committing the\n> request to hardware.\n>\n\nJust one minor nit..\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/pipeline_handler.h | 4 ++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++--\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++---\n> src/libcamera/pipeline/uvcvideo.cpp | 6 ++--\n> src/libcamera/pipeline/vimc.cpp | 6 ++--\n> src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-------\n> 6 files changed, 37 insertions(+), 27 deletions(-)\n>\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index a02e6e77dc9ce0e7..6ce8ea1c8d31b870 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -77,7 +77,7 @@ public:\n> \tvirtual int start(Camera *camera) = 0;\n> \tvirtual void stop(Camera *camera) = 0;\n>\n> -\tvirtual int queueRequest(Camera *camera, Request *request);\n> +\tint queueRequest(Camera *camera, Request *request);\n>\n> \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> \tvoid completeRequest(Camera *camera, Request *request);\n> @@ -89,6 +89,8 @@ protected:\n> \t\t\t std::unique_ptr data);\n> \tvoid hotplugMediaDevice(MediaDevice *media);\n>\n> +\tvirtual int queueRequestHardware(Camera *camera, Request *request) = 0;\n> +\n> \tCameraData *cameraData(const Camera *camera);\n>\n> \tCameraManager *manager_;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 1c5fccf694289ae9..ad223d9101bdc6ed 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -220,7 +220,7 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n>\n> -\tint queueRequest(Camera *camera, Request *request) override;\n> +\tint queueRequestHardware(Camera *camera, Request *request) override;\n>\n> \tbool match(DeviceEnumerator *enumerator) override;\n>\n> @@ -756,7 +756,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tdata->rawBuffers_.clear();\n> }\n>\n> -int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)\n> {\n> \tint error = 0;\n>\n> @@ -769,8 +769,6 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> \t\t\terror = ret;\n> \t}\n>\n> -\tPipelineHandler::queueRequest(camera, request);\n> -\n> \treturn error;\n> }\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 4a583a7a1d7ef1fd..f75b25c6787e40df 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -184,7 +184,7 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n>\n> -\tint queueRequest(Camera *camera, Request *request) override;\n> +\tint queueRequestHardware(Camera *camera, Request *request) override;\n>\n> \tbool match(DeviceEnumerator *enumerator) override;\n>\n> @@ -810,13 +810,12 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> \tactiveCamera_ = nullptr;\n> }\n>\n> -int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandlerRkISP1::queueRequestHardware(Camera *camera,\n> +\t\t\t\t\t\tRequest *request)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> \tStream *stream = &data->stream_;\n>\n> -\tPipelineHandler::queueRequest(camera, request);\n> -\n> \tRkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,\n> \t\t\t\t\t\t\tstream);\n> \tif (!info)\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 522229241ffb9e8a..3a76653ff6dc2b5e 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -72,7 +72,7 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n>\n> -\tint queueRequest(Camera *camera, Request *request) override;\n> +\tint queueRequestHardware(Camera *camera, Request *request) override;\n>\n> \tbool match(DeviceEnumerator *enumerator) override;\n>\n> @@ -262,7 +262,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> \treturn ret;\n> }\n>\n> -int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandlerUVC::queueRequestHardware(Camera *camera, Request *request)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> \tBuffer *buffer = request->findBuffer(&data->stream_);\n> @@ -281,8 +281,6 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> \tif (ret < 0)\n> \t\treturn ret;\n>\n> -\tPipelineHandler::queueRequest(camera, request);\n> -\n> \treturn 0;\n> }\n>\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 06664fed42e796e4..f5550a1723668106 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -90,7 +90,7 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n>\n> -\tint queueRequest(Camera *camera, Request *request) override;\n> +\tint queueRequestHardware(Camera *camera, Request *request) override;\n>\n> \tbool match(DeviceEnumerator *enumerator) override;\n>\n> @@ -323,7 +323,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> \treturn ret;\n> }\n>\n> -int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandlerVimc::queueRequestHardware(Camera *camera, Request *request)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> \tBuffer *buffer = request->findBuffer(&data->stream_);\n> @@ -342,8 +342,6 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> \tif (ret < 0)\n> \t\treturn ret;\n>\n> -\tPipelineHandler::queueRequest(camera, request);\n> -\n> \treturn 0;\n> }\n>\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 4349ca8957e403fe..c9e348b98da7b736 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -345,27 +345,42 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n> * \\param[in] request The request to queue\n> *\n> * This method queues a capture request to the pipeline handler for processing.\n> - * The request contains a set of buffers associated with streams and a set of\n> - * parameters. The pipeline handler shall program the device to ensure that the\n> - * parameters will be applied to the frames captured in the buffers provided in\n> - * the request.\n> - *\n> - * Pipeline handlers shall override this method. The base implementation in the\n> - * PipelineHandler class keeps track of queued requests in order to ensure\n> - * completion of all requests when the pipeline handler is stopped with stop().\n> - * Requests completion shall be signalled by the pipeline handler using the\n> + * The method keeps track of queued requests in order to ensure completion of\n\nYou are repeating \"This method\" and \"The method\" in two consecutive\nlines.\n\nI would just s/The method/It.\n\n> + * all requests when the pipeline handler is stopped with stop(). Requests\n> + * completion shall be signalled by the pipeline handler using the\n> * completeRequest() method.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> {\n> +\tint ret;\n> +\n> \tCameraData *data = cameraData(camera);\n> \tdata->queuedRequests_.push_back(request);\n>\n> -\treturn 0;\n> +\tret = queueRequestHardware(camera, request);\nYou could declare ret here.\n\nWith these two very minor issues addressed or not:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\tif (ret)\n> +\t\tdata->queuedRequests_.remove(request);\n> +\n> +\treturn ret;\n> }\n>\n> +/**\n> + * \\fn PipelineHandler::queueRequestHardware()\n> + * \\brief Queue a request to the hardware\n> + * \\param[in] camera The camera to queue the request to\n> + * \\param[in] request The request to queue\n> + *\n> + * This method queues a capture request to the hardware for processing. The\n> + * request contains a set of buffers associated with streams and a set of\n> + * parameters. The pipeline handler shall program the device to ensure that the\n> + * parameters will be applied to the frames captured in the buffers provided in\n> + * the request.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> /**\n> * \\brief Complete a buffer for a request\n> * \\param[in] camera The camera the request belongs to\n> --\n> 2.24.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D701860BB8\n\tfor ;\n\tWed, 27 Nov 2019 10:41:48 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 57BC81BF20A;\n\tWed, 27 Nov 2019 09:41:47 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Wed, 27 Nov 2019 10:43:54 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191127094354.b2orfwuxnjngrqvs@uno.localdomain>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"og7x7oiag4kqbex2\"","Content-Disposition":"inline","In-Reply-To":"<20191126233620.1695316-2-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align\n\tbookkeeping in queueRequest()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 27 Nov 2019 09:41:49 -0000"}},{"id":3217,"web_url":"https://patchwork.libcamera.org/comment/3217/","msgid":"<20191209130406.GD4853@pendragon.ideasonboard.com>","date":"2019-12-09T13:04:06","subject":"Re: [libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align\n\tbookkeeping in queueRequest()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Wed, Nov 27, 2019 at 12:35:51AM +0100, Niklas Söderlund wrote:\n> Expecting pipeline handler implementations of queueRequest() to call\n> the base class queueRequest() at the correct point have lead to\n\ns/lead/led/\n\n> different behaviors between the pipelines.\n> \n> Fix this by splitting queueRequest() into a base class implementation\n> which handles the bookkeeping and a new queueRequestHardware() that is\n> to be implemented by pipeline handlers and only deals with committing the\n> request to hardware.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/pipeline_handler.h | 4 ++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++--\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++---\n> src/libcamera/pipeline/uvcvideo.cpp | 6 ++--\n> src/libcamera/pipeline/vimc.cpp | 6 ++--\n> src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-------\n> 6 files changed, 37 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index a02e6e77dc9ce0e7..6ce8ea1c8d31b870 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -77,7 +77,7 @@ public:\n> \tvirtual int start(Camera *camera) = 0;\n> \tvirtual void stop(Camera *camera) = 0;\n> \n> -\tvirtual int queueRequest(Camera *camera, Request *request);\n> +\tint queueRequest(Camera *camera, Request *request);\n\nI would make this method final.\n\n> \n> \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> \tvoid completeRequest(Camera *camera, Request *request);\n> @@ -89,6 +89,8 @@ protected:\n> \t\t\t std::unique_ptr data);\n> \tvoid hotplugMediaDevice(MediaDevice *media);\n> \n> +\tvirtual int queueRequestHardware(Camera *camera, Request *request) = 0;\n> +\n> \tCameraData *cameraData(const Camera *camera);\n> \n> \tCameraManager *manager_;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 1c5fccf694289ae9..ad223d9101bdc6ed 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -220,7 +220,7 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n> \n> -\tint queueRequest(Camera *camera, Request *request) override;\n> +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> \n> \tbool match(DeviceEnumerator *enumerator) override;\n> \n> @@ -756,7 +756,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tdata->rawBuffers_.clear();\n> }\n> \n> -int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)\n> {\n> \tint error = 0;\n> \n> @@ -769,8 +769,6 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> \t\t\terror = ret;\n> \t}\n> \n> -\tPipelineHandler::queueRequest(camera, request);\n> -\n> \treturn error;\n> }\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 4a583a7a1d7ef1fd..f75b25c6787e40df 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -184,7 +184,7 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n> \n> -\tint queueRequest(Camera *camera, Request *request) override;\n> +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> \n> \tbool match(DeviceEnumerator *enumerator) override;\n> \n> @@ -810,13 +810,12 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> \tactiveCamera_ = nullptr;\n> }\n> \n> -int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandlerRkISP1::queueRequestHardware(Camera *camera,\n> +\t\t\t\t\t\tRequest *request)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> \tStream *stream = &data->stream_;\n> \n> -\tPipelineHandler::queueRequest(camera, request);\n> -\n> \tRkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,\n> \t\t\t\t\t\t\tstream);\n> \tif (!info)\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 522229241ffb9e8a..3a76653ff6dc2b5e 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -72,7 +72,7 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n> \n> -\tint queueRequest(Camera *camera, Request *request) override;\n> +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> \n> \tbool match(DeviceEnumerator *enumerator) override;\n> \n> @@ -262,7 +262,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> \treturn ret;\n> }\n> \n> -int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandlerUVC::queueRequestHardware(Camera *camera, Request *request)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> \tBuffer *buffer = request->findBuffer(&data->stream_);\n> @@ -281,8 +281,6 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tPipelineHandler::queueRequest(camera, request);\n> -\n> \treturn 0;\n> }\n> \n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 06664fed42e796e4..f5550a1723668106 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -90,7 +90,7 @@ public:\n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n> \n> -\tint queueRequest(Camera *camera, Request *request) override;\n> +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> \n> \tbool match(DeviceEnumerator *enumerator) override;\n> \n> @@ -323,7 +323,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> \treturn ret;\n> }\n> \n> -int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandlerVimc::queueRequestHardware(Camera *camera, Request *request)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> \tBuffer *buffer = request->findBuffer(&data->stream_);\n> @@ -342,8 +342,6 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tPipelineHandler::queueRequest(camera, request);\n> -\n> \treturn 0;\n> }\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 4349ca8957e403fe..c9e348b98da7b736 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -345,27 +345,42 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n> * \\param[in] request The request to queue\n> *\n> * This method queues a capture request to the pipeline handler for processing.\n> - * The request contains a set of buffers associated with streams and a set of\n> - * parameters. The pipeline handler shall program the device to ensure that the\n> - * parameters will be applied to the frames captured in the buffers provided in\n> - * the request.\n> - *\n> - * Pipeline handlers shall override this method. The base implementation in the\n> - * PipelineHandler class keeps track of queued requests in order to ensure\n> - * completion of all requests when the pipeline handler is stopped with stop().\n> - * Requests completion shall be signalled by the pipeline handler using the\n> + * The method keeps track of queued requests in order to ensure completion of\n> + * all requests when the pipeline handler is stopped with stop(). Requests\n> + * completion shall be signalled by the pipeline handler using the\n> * completeRequest() method.\n\nI would mention queueRequestHardware() here:\n\n * This method queues a capture request to the pipeline handler for processing.\n * The request is first added to the internal list of queued requests, and\n * then passed to the pipeline handler with a call to queueRequestHardware().\n *\n * Keeping track of queued requests ensures automatic completion of all requests\n * when the pipeline handler is stopped with stop(). Request completion shall be\n * signalled by the pipeline handler using the completeRequest() method.\n\nIt took me a while to write this paragraph, which is I think a sign that\nthe API isn't very clean, in particular that the queueRequest() and\nqueueRequestHardware() names are confusing (the latter especially\nbecause it may queue the request to the pipeline handler first, not\nimmediately to the hardware). I think I would prefer\nqueueRequestDevice() to queueRequestHardware() (with s/hardware/device/\nwhere applicable), but even that isn't perfect. I don't have better\nnames to propose at this moment, so\n\nReviewed-by: Laurent Pinchart \n\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> {\n> +\tint ret;\n> +\n> \tCameraData *data = cameraData(camera);\n> \tdata->queuedRequests_.push_back(request);\n> \n> -\treturn 0;\n> +\tret = queueRequestHardware(camera, request);\n> +\tif (ret)\n> +\t\tdata->queuedRequests_.remove(request);\n> +\n> +\treturn ret;\n> }\n> \n> +/**\n> + * \\fn PipelineHandler::queueRequestHardware()\n> + * \\brief Queue a request to the hardware\n> + * \\param[in] camera The camera to queue the request to\n> + * \\param[in] request The request to queue\n> + *\n> + * This method queues a capture request to the hardware for processing. The\n> + * request contains a set of buffers associated with streams and a set of\n> + * parameters. The pipeline handler shall program the device to ensure that the\n> + * parameters will be applied to the frames captured in the buffers provided in\n> + * the request.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> /**\n> * \\brief Complete a buffer for a request\n> * \\param[in] camera The camera the request belongs to","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 E8F5660BC4\n\tfor ;\n\tMon, 9 Dec 2019 14:04:13 +0100 (CET)","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 532BF99A;\n\tMon, 9 Dec 2019 14:04:13 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575896653;\n\tbh=ZuB7vZHzU4G0lcaJyj0CEoFoVz+6sC6mZQ8iVeFfRfo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c+FVsSm+QEVHieyxvx4OQZ9w2gNW/gJ5S8rNkYsupQ1o8NyhC00auMGQz2PPqdYAs\n\t5S+TmRZ3ED/OajT7js3lB3Pgo67AMc4P1LStnbIzgG16WSY9NEQNfbsTIK+oZ4pgc+\n\tdv49vc+NPrusZz4Aj7YMfW2VJyfAwRGIr72N54rQ=","Date":"Mon, 9 Dec 2019 15:04:06 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191209130406.GD4853@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191126233620.1695316-2-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align\n\tbookkeeping in queueRequest()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 09 Dec 2019 13:04:14 -0000"}},{"id":3243,"web_url":"https://patchwork.libcamera.org/comment/3243/","msgid":"<20191210193124.GB6956@bigcity.dyn.berto.se>","date":"2019-12-10T19:31:24","subject":"Re: [libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align\n\tbookkeeping in queueRequest()","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2019-12-09 15:04:06 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Wed, Nov 27, 2019 at 12:35:51AM +0100, Niklas Söderlund wrote:\n> > Expecting pipeline handler implementations of queueRequest() to call\n> > the base class queueRequest() at the correct point have lead to\n> \n> s/lead/led/\n> \n> > different behaviors between the pipelines.\n> > \n> > Fix this by splitting queueRequest() into a base class implementation\n> > which handles the bookkeeping and a new queueRequestHardware() that is\n> > to be implemented by pipeline handlers and only deals with committing the\n> > request to hardware.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/include/pipeline_handler.h | 4 ++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++--\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++---\n> > src/libcamera/pipeline/uvcvideo.cpp | 6 ++--\n> > src/libcamera/pipeline/vimc.cpp | 6 ++--\n> > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-------\n> > 6 files changed, 37 insertions(+), 27 deletions(-)\n> > \n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index a02e6e77dc9ce0e7..6ce8ea1c8d31b870 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -77,7 +77,7 @@ public:\n> > \tvirtual int start(Camera *camera) = 0;\n> > \tvirtual void stop(Camera *camera) = 0;\n> > \n> > -\tvirtual int queueRequest(Camera *camera, Request *request);\n> > +\tint queueRequest(Camera *camera, Request *request);\n> \n> I would make this method final.\n\nTo mark this method as final I need to first make it virtual.\n\n virtual int queueRequest(Camera *camera, Request *request) final;\n\nLooks a but suspicious to me and we have a lot of other methods which \nshould not be overloaded by pipeline implementations. I have keep this \nas is for now. I'm holding of adding your tag tho until you had a chance \nto provide your feedback to this.\n\n> \n> > \n> > \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> > \tvoid completeRequest(Camera *camera, Request *request);\n> > @@ -89,6 +89,8 @@ protected:\n> > \t\t\t std::unique_ptr data);\n> > \tvoid hotplugMediaDevice(MediaDevice *media);\n> > \n> > +\tvirtual int queueRequestHardware(Camera *camera, Request *request) = 0;\n> > +\n> > \tCameraData *cameraData(const Camera *camera);\n> > \n> > \tCameraManager *manager_;\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 1c5fccf694289ae9..ad223d9101bdc6ed 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -220,7 +220,7 @@ public:\n> > \tint start(Camera *camera) override;\n> > \tvoid stop(Camera *camera) override;\n> > \n> > -\tint queueRequest(Camera *camera, Request *request) override;\n> > +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> > \n> > \tbool match(DeviceEnumerator *enumerator) override;\n> > \n> > @@ -756,7 +756,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > \tdata->rawBuffers_.clear();\n> > }\n> > \n> > -int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> > +int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)\n> > {\n> > \tint error = 0;\n> > \n> > @@ -769,8 +769,6 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> > \t\t\terror = ret;\n> > \t}\n> > \n> > -\tPipelineHandler::queueRequest(camera, request);\n> > -\n> > \treturn error;\n> > }\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 4a583a7a1d7ef1fd..f75b25c6787e40df 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -184,7 +184,7 @@ public:\n> > \tint start(Camera *camera) override;\n> > \tvoid stop(Camera *camera) override;\n> > \n> > -\tint queueRequest(Camera *camera, Request *request) override;\n> > +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> > \n> > \tbool match(DeviceEnumerator *enumerator) override;\n> > \n> > @@ -810,13 +810,12 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> > \tactiveCamera_ = nullptr;\n> > }\n> > \n> > -int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n> > +int PipelineHandlerRkISP1::queueRequestHardware(Camera *camera,\n> > +\t\t\t\t\t\tRequest *request)\n> > {\n> > \tRkISP1CameraData *data = cameraData(camera);\n> > \tStream *stream = &data->stream_;\n> > \n> > -\tPipelineHandler::queueRequest(camera, request);\n> > -\n> > \tRkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,\n> > \t\t\t\t\t\t\tstream);\n> > \tif (!info)\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 522229241ffb9e8a..3a76653ff6dc2b5e 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -72,7 +72,7 @@ public:\n> > \tint start(Camera *camera) override;\n> > \tvoid stop(Camera *camera) override;\n> > \n> > -\tint queueRequest(Camera *camera, Request *request) override;\n> > +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> > \n> > \tbool match(DeviceEnumerator *enumerator) override;\n> > \n> > @@ -262,7 +262,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > \treturn ret;\n> > }\n> > \n> > -int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> > +int PipelineHandlerUVC::queueRequestHardware(Camera *camera, Request *request)\n> > {\n> > \tUVCCameraData *data = cameraData(camera);\n> > \tBuffer *buffer = request->findBuffer(&data->stream_);\n> > @@ -281,8 +281,6 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > \n> > -\tPipelineHandler::queueRequest(camera, request);\n> > -\n> > \treturn 0;\n> > }\n> > \n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 06664fed42e796e4..f5550a1723668106 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -90,7 +90,7 @@ public:\n> > \tint start(Camera *camera) override;\n> > \tvoid stop(Camera *camera) override;\n> > \n> > -\tint queueRequest(Camera *camera, Request *request) override;\n> > +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> > \n> > \tbool match(DeviceEnumerator *enumerator) override;\n> > \n> > @@ -323,7 +323,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> > \treturn ret;\n> > }\n> > \n> > -int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> > +int PipelineHandlerVimc::queueRequestHardware(Camera *camera, Request *request)\n> > {\n> > \tVimcCameraData *data = cameraData(camera);\n> > \tBuffer *buffer = request->findBuffer(&data->stream_);\n> > @@ -342,8 +342,6 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > \n> > -\tPipelineHandler::queueRequest(camera, request);\n> > -\n> > \treturn 0;\n> > }\n> > \n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 4349ca8957e403fe..c9e348b98da7b736 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -345,27 +345,42 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n> > * \\param[in] request The request to queue\n> > *\n> > * This method queues a capture request to the pipeline handler for processing.\n> > - * The request contains a set of buffers associated with streams and a set of\n> > - * parameters. The pipeline handler shall program the device to ensure that the\n> > - * parameters will be applied to the frames captured in the buffers provided in\n> > - * the request.\n> > - *\n> > - * Pipeline handlers shall override this method. The base implementation in the\n> > - * PipelineHandler class keeps track of queued requests in order to ensure\n> > - * completion of all requests when the pipeline handler is stopped with stop().\n> > - * Requests completion shall be signalled by the pipeline handler using the\n> > + * The method keeps track of queued requests in order to ensure completion of\n> > + * all requests when the pipeline handler is stopped with stop(). Requests\n> > + * completion shall be signalled by the pipeline handler using the\n> > * completeRequest() method.\n> \n> I would mention queueRequestHardware() here:\n> \n> * This method queues a capture request to the pipeline handler for processing.\n> * The request is first added to the internal list of queued requests, and\n> * then passed to the pipeline handler with a call to queueRequestHardware().\n> *\n> * Keeping track of queued requests ensures automatic completion of all requests\n> * when the pipeline handler is stopped with stop(). Request completion shall be\n> * signalled by the pipeline handler using the completeRequest() method.\n> \n> It took me a while to write this paragraph, which is I think a sign that\n> the API isn't very clean, in particular that the queueRequest() and\n> queueRequestHardware() names are confusing (the latter especially\n> because it may queue the request to the pipeline handler first, not\n> immediately to the hardware). I think I would prefer\n> queueRequestDevice() to queueRequestHardware() (with s/hardware/device/\n> where applicable), but even that isn't perfect. I don't have better\n> names to propose at this moment, so\n\nTanks! I went with queueRequestDevice() as I too like it more.\n\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> > {\n> > +\tint ret;\n> > +\n> > \tCameraData *data = cameraData(camera);\n> > \tdata->queuedRequests_.push_back(request);\n> > \n> > -\treturn 0;\n> > +\tret = queueRequestHardware(camera, request);\n> > +\tif (ret)\n> > +\t\tdata->queuedRequests_.remove(request);\n> > +\n> > +\treturn ret;\n> > }\n> > \n> > +/**\n> > + * \\fn PipelineHandler::queueRequestHardware()\n> > + * \\brief Queue a request to the hardware\n> > + * \\param[in] camera The camera to queue the request to\n> > + * \\param[in] request The request to queue\n> > + *\n> > + * This method queues a capture request to the hardware for processing. The\n> > + * request contains a set of buffers associated with streams and a set of\n> > + * parameters. The pipeline handler shall program the device to ensure that the\n> > + * parameters will be applied to the frames captured in the buffers provided in\n> > + * the request.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > /**\n> > * \\brief Complete a buffer for a request\n> > * \\param[in] camera The camera the request belongs to\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C04160BDB\n\tfor ;\n\tTue, 10 Dec 2019 20:31:27 +0100 (CET)","by mail-lf1-x12a.google.com with SMTP id f15so13852919lfl.13\n\tfor ;\n\tTue, 10 Dec 2019 11:31:27 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\ta12sm2279579ljk.48.2019.12.10.11.31.25\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 10 Dec 2019 11:31:25 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=GQ/Cf4CZ5bB8imF/z0Lu1uWdNP7MaH/rc6bUhEtBuq8=;\n\tb=qPnwRSmS0N3tPOxdPU5ifOjtk/vfzQ7GRlAHzz0pjFuSkqrN2mRm+78UJyCWOuXhD0\n\taZ3ryap6zsr6XPJ/Rj2yu8mZ38BJFiXdvgh4kTWaQfAC75sMcBZZhh++gvNjHMm4W1Cr\n\tHKRXHaG8OOZlwBxlZcDM9yYgkNsBhgSA/Kmv2d1abCMYYx9esnBdwxQvFar71YZ/a/tx\n\thMSoO5TUehzx2MzxkJPa0PAmNJbQ/Ds20/saZeDzKVEaMUxvOI/9V+sm3ZDGJEW0kUo+\n\tM1PWnl3W2CS0JfKARmKftaLwS0IJXB6HNqrkCotCl7e+pPvDuUgunvQfG6lRrAFZ40de\n\t51XA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=GQ/Cf4CZ5bB8imF/z0Lu1uWdNP7MaH/rc6bUhEtBuq8=;\n\tb=iEzTGbTqY4LbXgobHKPdyTSSNTBvgNz6dA/n2WeYC/kuZtlIUIUo6x9D831cK0yJNt\n\tBcHdmiPz9PoB+FZj1FU7kk1wZBb3lpXSmgYdz7Mf5bMkppFb5HonuRMxZlTu0sZntuHf\n\tLp+7hurpv+1JDwlzY8oSLwrMkkKYxUzy8kJRy9nPfx0FqMvf8QSPrbe3I/+sZuiHa90C\n\tzZ+Dlo+wpF7NEaKr3tZd+c8rd5I5Q4buLDBdLz2vkBi4vl0Dj4WqfU2qezOJV5jMlKIe\n\t82/ZJrZrkIOM9bnmEJOLJeU82wX1/lX1yYnRRgxgCdJMzZwIHNBPIJR6yor7RitKe7y6\n\tPE3g==","X-Gm-Message-State":"APjAAAUlHIMBGll6ZaTXoY289BSMLx8sjGtewk3D9egMsNvJdBtbzup7\n\t26DXwiR7x3O+mDJL2/ZCFFrtdJop+j4=","X-Google-Smtp-Source":"APXvYqxroVTYXJRw9oz9nhJWxvLcise9obsM7otbMy1AMsEjKbe+h/+YzPUsHE1i2wL6UaBskh0ziQ==","X-Received":"by 2002:a19:5201:: with SMTP id\n\tm1mr18724575lfb.114.1576006286237; \n\tTue, 10 Dec 2019 11:31:26 -0800 (PST)","Date":"Tue, 10 Dec 2019 20:31:24 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191210193124.GB6956@bigcity.dyn.berto.se>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-2-niklas.soderlund@ragnatech.se>\n\t<20191209130406.GD4853@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191209130406.GD4853@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align\n\tbookkeeping in queueRequest()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 10 Dec 2019 19:31:27 -0000"}},{"id":3244,"web_url":"https://patchwork.libcamera.org/comment/3244/","msgid":"<20191210194122.GC5211@pendragon.ideasonboard.com>","date":"2019-12-10T19:41:22","subject":"Re: [libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align\n\tbookkeeping in queueRequest()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Dec 10, 2019 at 08:31:24PM +0100, Niklas Söderlund wrote:\n> On 2019-12-09 15:04:06 +0200, Laurent Pinchart wrote:\n> > On Wed, Nov 27, 2019 at 12:35:51AM +0100, Niklas Söderlund wrote:\n> > > Expecting pipeline handler implementations of queueRequest() to call\n> > > the base class queueRequest() at the correct point have lead to\n> > \n> > s/lead/led/\n> > \n> > > different behaviors between the pipelines.\n> > > \n> > > Fix this by splitting queueRequest() into a base class implementation\n> > > which handles the bookkeeping and a new queueRequestHardware() that is\n> > > to be implemented by pipeline handlers and only deals with committing the\n> > > request to hardware.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/include/pipeline_handler.h | 4 ++-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++--\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++---\n> > > src/libcamera/pipeline/uvcvideo.cpp | 6 ++--\n> > > src/libcamera/pipeline/vimc.cpp | 6 ++--\n> > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-------\n> > > 6 files changed, 37 insertions(+), 27 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > > index a02e6e77dc9ce0e7..6ce8ea1c8d31b870 100644\n> > > --- a/src/libcamera/include/pipeline_handler.h\n> > > +++ b/src/libcamera/include/pipeline_handler.h\n> > > @@ -77,7 +77,7 @@ public:\n> > > \tvirtual int start(Camera *camera) = 0;\n> > > \tvirtual void stop(Camera *camera) = 0;\n> > > \n> > > -\tvirtual int queueRequest(Camera *camera, Request *request);\n> > > +\tint queueRequest(Camera *camera, Request *request);\n> > \n> > I would make this method final.\n> \n> To mark this method as final I need to first make it virtual.\n> \n> virtual int queueRequest(Camera *camera, Request *request) final;\n> \n> Looks a but suspicious to me and we have a lot of other methods which \n> should not be overloaded by pipeline implementations. I have keep this \n> as is for now. I'm holding of adding your tag tho until you had a chance \n> to provide your feedback to this.\n\nMy bad, you're right.\n\n> > > \n> > > \tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> > > \tvoid completeRequest(Camera *camera, Request *request);\n> > > @@ -89,6 +89,8 @@ protected:\n> > > \t\t\t std::unique_ptr data);\n> > > \tvoid hotplugMediaDevice(MediaDevice *media);\n> > > \n> > > +\tvirtual int queueRequestHardware(Camera *camera, Request *request) = 0;\n> > > +\n> > > \tCameraData *cameraData(const Camera *camera);\n> > > \n> > > \tCameraManager *manager_;\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 1c5fccf694289ae9..ad223d9101bdc6ed 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -220,7 +220,7 @@ public:\n> > > \tint start(Camera *camera) override;\n> > > \tvoid stop(Camera *camera) override;\n> > > \n> > > -\tint queueRequest(Camera *camera, Request *request) override;\n> > > +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> > > \n> > > \tbool match(DeviceEnumerator *enumerator) override;\n> > > \n> > > @@ -756,7 +756,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > > \tdata->rawBuffers_.clear();\n> > > }\n> > > \n> > > -int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> > > +int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)\n> > > {\n> > > \tint error = 0;\n> > > \n> > > @@ -769,8 +769,6 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> > > \t\t\terror = ret;\n> > > \t}\n> > > \n> > > -\tPipelineHandler::queueRequest(camera, request);\n> > > -\n> > > \treturn error;\n> > > }\n> > > \n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 4a583a7a1d7ef1fd..f75b25c6787e40df 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -184,7 +184,7 @@ public:\n> > > \tint start(Camera *camera) override;\n> > > \tvoid stop(Camera *camera) override;\n> > > \n> > > -\tint queueRequest(Camera *camera, Request *request) override;\n> > > +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> > > \n> > > \tbool match(DeviceEnumerator *enumerator) override;\n> > > \n> > > @@ -810,13 +810,12 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> > > \tactiveCamera_ = nullptr;\n> > > }\n> > > \n> > > -int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n> > > +int PipelineHandlerRkISP1::queueRequestHardware(Camera *camera,\n> > > +\t\t\t\t\t\tRequest *request)\n> > > {\n> > > \tRkISP1CameraData *data = cameraData(camera);\n> > > \tStream *stream = &data->stream_;\n> > > \n> > > -\tPipelineHandler::queueRequest(camera, request);\n> > > -\n> > > \tRkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,\n> > > \t\t\t\t\t\t\tstream);\n> > > \tif (!info)\n> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > > index 522229241ffb9e8a..3a76653ff6dc2b5e 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > > @@ -72,7 +72,7 @@ public:\n> > > \tint start(Camera *camera) override;\n> > > \tvoid stop(Camera *camera) override;\n> > > \n> > > -\tint queueRequest(Camera *camera, Request *request) override;\n> > > +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> > > \n> > > \tbool match(DeviceEnumerator *enumerator) override;\n> > > \n> > > @@ -262,7 +262,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > > \treturn ret;\n> > > }\n> > > \n> > > -int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> > > +int PipelineHandlerUVC::queueRequestHardware(Camera *camera, Request *request)\n> > > {\n> > > \tUVCCameraData *data = cameraData(camera);\n> > > \tBuffer *buffer = request->findBuffer(&data->stream_);\n> > > @@ -281,8 +281,6 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> > > \tif (ret < 0)\n> > > \t\treturn ret;\n> > > \n> > > -\tPipelineHandler::queueRequest(camera, request);\n> > > -\n> > > \treturn 0;\n> > > }\n> > > \n> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > > index 06664fed42e796e4..f5550a1723668106 100644\n> > > --- a/src/libcamera/pipeline/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc.cpp\n> > > @@ -90,7 +90,7 @@ public:\n> > > \tint start(Camera *camera) override;\n> > > \tvoid stop(Camera *camera) override;\n> > > \n> > > -\tint queueRequest(Camera *camera, Request *request) override;\n> > > +\tint queueRequestHardware(Camera *camera, Request *request) override;\n> > > \n> > > \tbool match(DeviceEnumerator *enumerator) override;\n> > > \n> > > @@ -323,7 +323,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> > > \treturn ret;\n> > > }\n> > > \n> > > -int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> > > +int PipelineHandlerVimc::queueRequestHardware(Camera *camera, Request *request)\n> > > {\n> > > \tVimcCameraData *data = cameraData(camera);\n> > > \tBuffer *buffer = request->findBuffer(&data->stream_);\n> > > @@ -342,8 +342,6 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> > > \tif (ret < 0)\n> > > \t\treturn ret;\n> > > \n> > > -\tPipelineHandler::queueRequest(camera, request);\n> > > -\n> > > \treturn 0;\n> > > }\n> > > \n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 4349ca8957e403fe..c9e348b98da7b736 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -345,27 +345,42 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n> > > * \\param[in] request The request to queue\n> > > *\n> > > * This method queues a capture request to the pipeline handler for processing.\n> > > - * The request contains a set of buffers associated with streams and a set of\n> > > - * parameters. The pipeline handler shall program the device to ensure that the\n> > > - * parameters will be applied to the frames captured in the buffers provided in\n> > > - * the request.\n> > > - *\n> > > - * Pipeline handlers shall override this method. The base implementation in the\n> > > - * PipelineHandler class keeps track of queued requests in order to ensure\n> > > - * completion of all requests when the pipeline handler is stopped with stop().\n> > > - * Requests completion shall be signalled by the pipeline handler using the\n> > > + * The method keeps track of queued requests in order to ensure completion of\n> > > + * all requests when the pipeline handler is stopped with stop(). Requests\n> > > + * completion shall be signalled by the pipeline handler using the\n> > > * completeRequest() method.\n> > \n> > I would mention queueRequestHardware() here:\n> > \n> > * This method queues a capture request to the pipeline handler for processing.\n> > * The request is first added to the internal list of queued requests, and\n> > * then passed to the pipeline handler with a call to queueRequestHardware().\n> > *\n> > * Keeping track of queued requests ensures automatic completion of all requests\n> > * when the pipeline handler is stopped with stop(). Request completion shall be\n> > * signalled by the pipeline handler using the completeRequest() method.\n> > \n> > It took me a while to write this paragraph, which is I think a sign that\n> > the API isn't very clean, in particular that the queueRequest() and\n> > queueRequestHardware() names are confusing (the latter especially\n> > because it may queue the request to the pipeline handler first, not\n> > immediately to the hardware). I think I would prefer\n> > queueRequestDevice() to queueRequestHardware() (with s/hardware/device/\n> > where applicable), but even that isn't perfect. I don't have better\n> > names to propose at this moment, so\n> \n> Tanks! I went with queueRequestDevice() as I too like it more.\n> \n> > Reviewed-by: Laurent Pinchart \n> > \n> > > *\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > > int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> > > {\n> > > +\tint ret;\n> > > +\n> > > \tCameraData *data = cameraData(camera);\n> > > \tdata->queuedRequests_.push_back(request);\n> > > \n> > > -\treturn 0;\n> > > +\tret = queueRequestHardware(camera, request);\n> > > +\tif (ret)\n> > > +\t\tdata->queuedRequests_.remove(request);\n> > > +\n> > > +\treturn ret;\n> > > }\n> > > \n> > > +/**\n> > > + * \\fn PipelineHandler::queueRequestHardware()\n> > > + * \\brief Queue a request to the hardware\n> > > + * \\param[in] camera The camera to queue the request to\n> > > + * \\param[in] request The request to queue\n> > > + *\n> > > + * This method queues a capture request to the hardware for processing. The\n> > > + * request contains a set of buffers associated with streams and a set of\n> > > + * parameters. The pipeline handler shall program the device to ensure that the\n> > > + * parameters will be applied to the frames captured in the buffers provided in\n> > > + * the request.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Complete a buffer for a request\n> > > * \\param[in] camera The camera the request belongs to","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 6DE3F60BDB\n\tfor ;\n\tTue, 10 Dec 2019 20:41:30 +0100 (CET)","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 E040EB85;\n\tTue, 10 Dec 2019 20:41:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576006890;\n\tbh=/S98wtmunVWD4Z2cbfkEZ/OYCU5xBMdpECqo9ieWbi4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jcfO/zLiCzJW/azub4beaGgHN1lpDrWlD8sP6Se0V1tQ6T92AeZ7IOz7Lcp99Pmnm\n\t/h9/mgwik+08VanrerelQwVteACyTb5VCf7K2l9Va9sxnbxJhR4L4jJityVhbTjuE6\n\t1qbaC9J893TcjrlHIFrsSg1hvWo0HZIpmFlyJQCg=","Date":"Tue, 10 Dec 2019 21:41:22 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191210194122.GC5211@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-2-niklas.soderlund@ragnatech.se>\n\t<20191209130406.GD4853@pendragon.ideasonboard.com>\n\t<20191210193124.GB6956@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191210193124.GB6956@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align\n\tbookkeeping in queueRequest()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 10 Dec 2019 19:41:30 -0000"}}]